Open Bug 1383868 Opened 7 years ago Updated 2 years ago

stylo: Incorrect blockification of kids of display:inline-flex element that is not a flex container

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix

People

(Reporter: bzbarsky, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1383868 - Compute display of <legend> other than 'none' to 'block'.
59 bytes, text/x-review-board-request
Details 
This seems to be why layout/reftests/forms/legend/1339287.html fails in stylo: stylo is blockifying the span, which is incorrect.

Note that this may require a spec issue on CSS or HTML; per spec at https://drafts.csswg.org/css-flexbox/#box-model we have:

  A flex container is the box generated by an element with a computed display of
  flex or inline-flex. In-flow children of a flex container are called flex items
  and are laid out using the flex layout model.

and per https://drafts.csswg.org/css-flexbox/#flex-items we have:

  The display value of a flex item is blockified: if the specified display of an
  in-flow child of an element generating a flex container is an inline-level
  value, it computes to its block-level equivalent.

But <legend> is subject to https://html.spec.whatwg.org/multipage/rendering.html#the-fieldset-and-legend-elements which says:

  A fieldset element's rendered legend, if any, is expected to be rendered over
  the block-start border edge of the fieldset element as a block box (overriding
  any explicit 'display' value).

That is, for <legend> the computed and used display may not agree per HTML spec.  But the flexbox spec based blockification and more importantly flex-itemness on computed display, not used display, which gives the wrong results in this case: the kids of the <legend> should NOT be flex items here.

There are similar problems (also shown by this test) with grid.

In Gecko this is implemented by using a AutoParentDisplayBasedStyleFixupSkipper in nsCSSFrameConstructor::ProcessChildren as needed based on frame type.  This is somewhat buggy, in that a dynamic restyle of the legend child causes it to get blockified:

  <!DOCTYPE html>
  <fieldset>
    <legend style="display: inline-flex">
      <span style="border: 1px solid green" onclick="this.style.opacity=0.99">Click me: <br> do I become
      a block?"</span>
    </legend>
  </fieldset>

but Chrome/Safari consistently avoid blockification here.
Blocks: stylo-reftest
I saw this before when I was investigating inlinification. I forget whether I left any comment anywhere at that time.

Gecko's current fix in bug 1339287 isn't going to work anyway for Stylo, because that requires knowing the parent's frame type before resolving its children. We can, though, add some methods to Element to state that this element is framed according to HTML and doesn't support becoming a flex/grid container.
That's not as easy as it sounds, because <legend> does support it unless it's the rendered legend.  And the definition of "rendered legend" in Gecko depends on the parent frame the legend is getting, as well as the styles of the legend, the siblings of the legend, etc.
Oh, hmmm... we can still implement the logic of rendered legend for this, but it could be annoying for having a same thing in two places.

Maybe we can try implementing flex/grid support for legend instead, so that we don't need to worry about blockification of their children? I guess we can add another anonymous box in-between in this case, just like what we've done for fieldset?
> Maybe we can try implementing flex/grid support for legend instead

That would violate the HTML spec bits quoted above.  It must be a block, per spec.
So I think there is actually conflict between the HTML spec and the CSS spec somehow, or at least their combination can lead to confusion or undesired behavior.

The HTML spec's text, "is expected to be rendered ... as a block box" probably indicates that the used value of "display" is overridden to "block" (otherwise it should probably say that the "display" value is computed to "block" instead).

And the css-flexbox spec says:
> A flex container is the box generated by an element with a computed display of flex or inline-flex.
> In-flow children of a flex container are called flex items and are laid out using the flex layout model.
and
> The display value of a flex item is blockified

which means, stylo's current behavior is actually somehow conforming to both spec: it does render the rendered legend as block per HTML spec, while it also blockifies the children per css-flexbox spec.

I think the HTML spec should be changed so that the computed value is overridden. Making used value different than computed value can cause lots of confusion.

Also, if we can change the computed value, I guess we can solve this problem purely in the style system.
I suggest reading https://github.com/whatwg/html/issues/2756 so we don't rehash all of that discussion in here...  That discussion includes some reasons why changing the computed value on just the "rendered legend" is a bit complicated, if possible at all.
And yes, I agree that there is conflict between the specs involved here.  Getting the CSS WG to care about such things is beyond my power, so I've given up on it.
Priority: -- → P3
So, I did some investigation into what other engines do on this problem.

Blink and WebKit change the computed value of display to "block" for all <legend> which has any display value other than "none". None of "position" and "float" affects this behavior, and "display: contents" is also overridden with "block".

Edge doesn't change the computed value of display, but it gives all <legend> a block-inside used value. And this is also done regardless of the hierarchy or other properties. But interestingly, Edge never treats inline-outside legend as the render legend, which is probably a bug of them.

So, it seems to me that Gecko is the only browser that currently respects document hierarchy when handling legend.

Based on that, I suggest that we just follow the behavior of Blink and WebKit here, which is the most straightforward way forward. And I think it would also be the best way to spec, as it would eliminate the conflict between the two specs.

bz, what do you think?
Flags: needinfo?(bzbarsky)
> Blink and WebKit change the computed value of display to "block" for all <legend>

Blink is planning to change this, afaik.  See https://chromium-review.googlesource.com/c/chromium/src/+/535595 (mentioned in the above-linked <https://github.com/whatwg/html/issues/2756>).

Past that, I don't really have strong opinions.  HTML used to be trying to make non-fieldset-rendered-legend legends less-magical because they were reusing them for <figure>, but they stopped doing that.  So I would actually be OK with implementing the current webkit/blink behavior and getting the spec changed accordingly.  It would certainly make speccing things really easy, except for the part where the HTML spec would have to change how CSS display property computation works (because "none" still computes to "none" even though all other values compute to "block", so the override happens at computed value time, not specified value time....)
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> > Blink and WebKit change the computed value of display to "block" for all <legend>
> 
> Blink is planning to change this, afaik.  See
> https://chromium-review.googlesource.com/c/chromium/src/+/535595 (mentioned
> in the above-linked <https://github.com/whatwg/html/issues/2756>).

That one doesn't change the computed value overriding. It only changes whether a legend is shrink-to-fit and create block formatting context. Those are part of the layout, not in style system, and I would expect that matches what Gecko have already been doing.

This is indeed a difference between Blink and WebKit, though, that WebKit all the <legend> not only have 'block' computed display, but also shrink-to-fit, while in Blink, only the former happens now.
Assignee: nobody → xidorn+moz
Not sure whether it's worth converting Gecko's style system to use the same mechanism...
Looks like we would need to change Gecko as well...
Attachment #8908048 - Flags: review?(emilio)
Attachment #8908049 - Flags: review?(emilio)
Attachment #8908050 - Flags: review?(emilio)
Attachment #8908048 - Attachment is obsolete: true
Attachment #8908049 - Attachment is obsolete: true
Attachment #8908050 - Attachment is obsolete: true
Comment on attachment 8908421 [details]
Bug 1383868 - Compute display of <legend> other than 'none' to 'block'.

https://reviewboard.mozilla.org/r/180052/#review185220

::: servo/components/style/style_resolver.rs:576
(Diff revision 1)
>              cascade_flags.insert(PROHIBIT_DISPLAY_CONTENTS);
>          } else if self.element.is_root() {
>              cascade_flags.insert(IS_ROOT_ELEMENT);
>          }
>  
> +        if self.element.get_local_name() == &*local_name!("legend") {

I know this is not technically up for review, but this should check `pseudo.is_none()` too, otherwise you incorrectly blockify other stuff.
Fixing stylo in the way that follows Blink and WebKit regresses much more tests than it fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d1069f7ce94d0f70c1894a52f3d037e95d50e1d&filter-searchStr=reftest&filter-resultStatus=testfailed&filter-resultStatus=runnable&filter-classifiedState=unclassified

Basically this is because of the change to behavior of <legend>, although this brings our behavior closer to other browsers.

It also seems to me that it is not trivial to implement in the Gecko's style system since we don't have a general mechanism to pass element info down to style context building.

Given these, I guess we should probably accept the regression on bug 1339287 for now, and add this fixup and update the tests after the beta uplift of 57 to minimize confusion and risk of shipping stylo.
Comment on attachment 8908421 [details]
Bug 1383868 - Compute display of <legend> other than 'none' to 'block'.

https://reviewboard.mozilla.org/r/180052/#review185220

> I know this is not technically up for review, but this should check `pseudo.is_none()` too, otherwise you incorrectly blockify other stuff.

Fixed. Thanks.
See Also: → 1370718
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> Given these, I guess we should probably accept the regression on bug 1339287
> for now, and add this fixup and update the tests after the beta uplift of 57
> to minimize confusion and risk of shipping stylo.

This makes sense to me overall, specially given that this feature is pretty broken in Gecko, as Boris pointed out in the bug description.
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: affected → wontfix
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: