Fix ::first-letter not to apply non-block containers

Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property. An example is to apply 'display: flex' to
<fieldset> elements.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property. The list of values of the 'display' property to be
block containers is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117
Review-Url: https://codereview.chromium.org/2650583003
Cr-Commit-Position: refs/heads/master@{#446287}
Committed: https://chromium.googlesource.com/chromium/src/+/880301a7e70e48fd5eb46dc441f3d974ec4ae6bf

[kojii, 2017-01-23 06:48:33]

Description was changed from

==========
Apply crrev.com/2373963002 to HTMLFieldSetElement

BUG=
==========

to

==========
Apply crrev.com/2373963002 to HTMLFieldSetElement

BUG=675117
==========

[kojii, 2017-01-23 06:55:19]

Description was changed from

==========
Apply crrev.com/2373963002 to HTMLFieldSetElement

BUG=675117
==========

to

==========
Fix ::first-letter not to become a block

BUG=675117
==========

[kojii, 2017-01-23 06:56:31]

kojii@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[kojii, 2017-01-23 06:56:31]

cbiesinger, could you mind to advise?

PS1 fixes HTMLFieldSetElement::createLayoutObject,
PS2 fixes StyleResolver.

[cbiesinger, 2017-01-23 23:46:55]

cbiesinger@chromium.org changed reviewers:
+ rune@opera.com

[cbiesinger, 2017-01-23 23:46:55]

You're asking which one I like better? I think we should get the input from a
style person, adding rune@.

[rune, 2017-01-24 11:03:01]

I'm not sure. The issue here is that the computed display does not reflect what
the layout object is. The question is if we intend to support display:flex/grid
for fieldset or not.

Some quick testing shows Firefox supports both rendering the fieldset with
legend and at the same time act as a flex container for its children. Also, it
means Firefox does not support first-letter rendering for fieldsets with
display:flex/grid. I have not tested IE/Edge.

Does it make sense to adjust the display type to corresponding block/inline for
grid/flex types?

PS1 means we'll no longer be rendering legend "correctly"?

[kojii, 2017-01-24 13:06:26]

+tkent@, this is related with forms.

> I'm not sure. The issue here is that the computed display does not reflect
what the layout object is.

Yeah. And some part of our code uses "display" while some relies on the layout
object. So to fix crash, we can fix either way, but the correct fix is unclear.

> The question is if we intend to support display:flex/grid for fieldset or not.
> Some quick testing shows Firefox supports both rendering the fieldset with
legend and at the same time act as a flex container for its children. Also, it
means Firefox does not support first-letter rendering for fieldsets with
display:flex/grid. I have not tested IE/Edge.

"display: flex" forces its children to be block, but "::first-letter" must be
inline. It looks like impls agree to ignore ::first-letter in this case:
http://jsbin.com/sadawi/edit?html,output

So if "fieldsets should work with flex/grid" and "flex/grid should disable
::first-letter", ps2 might be a good direction?

> Does it make sense to adjust the display type to corresponding block/inline
for grid/flex types?

Do you mean to adjust "inline-flex" to "inline-block" for fieldset?

> PS1 means we'll no longer be rendering legend "correctly"?

Yeah, if the "correct" is to render fieldset as is while being a flex container.
I guess this is undefined part, correct?

[rune, 2017-01-24 14:59:51]

On 2017/01/24 13:06:26, kojii wrote:
> +tkent@, this is related with forms.
> 
> > I'm not sure. The issue here is that the computed display does not reflect
> what the layout object is.
> 
> Yeah. And some part of our code uses "display" while some relies on the layout
> object. So to fix crash, we can fix either way, but the correct fix is
unclear.

It would be good if we could adjust computed display to match the layout box.
That would mean we'd be different from Gecko for computed style, though.

> > The question is if we intend to support display:flex/grid for fieldset or
not.
> > Some quick testing shows Firefox supports both rendering the fieldset with
> legend and at the same time act as a flex container for its children. Also, it
> means Firefox does not support first-letter rendering for fieldsets with
> display:flex/grid. I have not tested IE/Edge.

From what I tested, Gecko supports flex, but not table e.g.

FWIW, the html spec says the legend must be block, but it doesn't limit the
display types for fieldset afaics.

> "display: flex" forces its children to be block, but "::first-letter" must be
> inline. It looks like impls agree to ignore ::first-letter in this case:
> http://jsbin.com/sadawi/edit?html,output
> 
> So if "fieldsets should work with flex/grid" and "flex/grid should disable
> ::first-letter", ps2 might be a good direction?

I guess. I was hoping we could adjust the computed display or handle it where we
call behavesLikeBlockContainer() to check if we can have a first-letter pseudo.
Modifying the computed style would cause interop issues if the other browsers
keep it as specified.

> > Does it make sense to adjust the display type to corresponding block/inline
> for grid/flex types?
> 
> Do you mean to adjust "inline-flex" to "inline-block" for fieldset?

Yes. Or in general actually. When we style fieldset with inline-table in Blink,
we effectively have inline-block, table -> block, inline-flex -> inline, etc.

> > PS1 means we'll no longer be rendering legend "correctly"?
> 
> Yeah, if the "correct" is to render fieldset as is while being a flex
container.
> I guess this is undefined part, correct?

The HTML spec says the legend must be block, but there's no mention of
limitation of display for fieldset afaics. It does look underspecified, though.
Implementations push the top border down, yet the spec doesn't suggest so.

[kojii, 2017-01-26 04:38:13]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 04:38:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-01-26 04:47:16]

Description was changed from

==========
Fix ::first-letter not to become a block

BUG=675117
==========

to

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

[1] https://drafts.csswg.org/css-pseudo-4/

BUG=675117
==========

[kojii, 2017-01-26 04:48:28]

Description was changed from

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

[1] https://drafts.csswg.org/css-pseudo-4/

BUG=675117
==========

to

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

[1] https://drafts.csswg.org/css-pseudo-4/

BUG=675117
==========

[kojii, 2017-01-26 04:55:18]

rune@, PTAL.

> Yes. Or in general actually.

I hope PS4 understood this correctly, please let me know if not.

In this patch, I only changed the behavior (used value) without changing the
computed value of the 'display' property. I think this is better, and avoid
possible side effects such as when author used 'display: inherit'. From your
reply, I understood you haven't decided which is better, no?

[kojii, 2017-01-26 04:58:19]

Description was changed from

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

[1] https://drafts.csswg.org/css-pseudo-4/

BUG=675117
==========

to

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

The list of values of the 'display' property to be block containers
is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117
==========

[kojii, 2017-01-26 05:34:29]

Description was changed from

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property.

The list of values of the 'display' property to be block containers
is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117
==========

to

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property. An example is to apply 'display: flex' to
<fieldset> elements.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property. The list of values of the 'display' property to be
block containers is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117
==========

[commit-bot: I haz the power, 2017-01-26 07:24:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 07:24:02]

Dry run: This issue passed the CQ dry run.

[rune, 2017-01-26 08:55:07]

On 2017/01/26 04:55:18, kojii wrote:
> rune@, PTAL.
> 
> > Yes. Or in general actually.
> 
> I hope PS4 understood this correctly, please let me know if not.
> 
> In this patch, I only changed the behavior (used value) without changing the
> computed value of the 'display' property. I think this is better, and avoid
> possible side effects such as when author used 'display: inherit'. From your
> reply, I understood you haven't decided which is better, no?

For interoperability wrt inherit, this is probably best, yes.

[rune, 2017-01-26 09:14:06]

rune@opera.com changed reviewers:
+ mstensho@opera.com

[rune, 2017-01-26 09:14:06]

lgtm, but Morten promised to take a look with his layout glasses on.

[mstensho (USE GERRIT), 2017-01-26 09:37:05]

lgtm

[kojii, 2017-01-26 09:49:21]

Thank you all for prompt reviews!

[kojii, 2017-01-26 09:49:27]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-01-26 09:49:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 09:54:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485424167913650,
"parent_rev": "86993c062fe94ca4c417a7960391067882dcd18c", "commit_rev":
"880301a7e70e48fd5eb46dc441f3d974ec4ae6bf"}

[commit-bot: I haz the power, 2017-01-26 09:54:52]

Description was changed from

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property. An example is to apply 'display: flex' to
<fieldset> elements.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property. The list of values of the 'display' property to be
block containers is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117
==========

to

==========
Fix ::first-letter not to apply non-block containers

The CSS Pseudo-Elements[1] defines that ::first-line and ::first-letter
only applies to block containers.

Checking |isLayoutBlockFlow()| works in most cases. However, when
|Element::createLayoutObject()| creates |LayoutObject| without using
|LayoutObject::createObject()|, |isLayoutBlockFlow()| may conflict with
the CSS display property. An example is to apply 'display: flex' to
<fieldset> elements.

This patch changes to check both |isLayoutBlockFlow()| and the CSS
display property. The list of values of the 'display' property to be
block containers is defined in CSS Display[2].

[1] https://drafts.csswg.org/css-pseudo-4/
[2] https://drafts.csswg.org/css-display/

BUG=675117

Review-Url: https://codereview.chromium.org/2650583003
Cr-Commit-Position: refs/heads/master@{#446287}
Committed:
https://chromium.googlesource.com/chromium/src/+/880301a7e70e48fd5eb46dc441f3...
==========

[commit-bot: I haz the power, 2017-01-26 09:54:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/880301a7e70e48fd5eb46dc441f3...