1427292 - Update display: contents to the spec regarding replaced elements / svg and such.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

See https://drafts.csswg.org/css-display/#unbox.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

This will pass[1] whenever the next WPT sync happens.

[1]: https://github.com/w3c/web-platform-tests/blob/master/css/css-display/display-contents-unusual-html-elements-none.html

Review commit: https://reviewboard.mozilla.org/r/209464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209464/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd7707e66a8ae4743f46c9a28c3003ba0097c80

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8939032 [details]
Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

https://reviewboard.mozilla.org/r/209464/#review216340

If I read the code correctly, this would enable display:contents on MathML elements, right?
That makes me a bit nervous as I'm not sure our MathML code is ready for that.
Also, the spec at https://drafts.csswg.org/css-display/#unbox doesn't say anything about MathML AFAICT.
Note that the final clause says "any other *HTML* element" (my emphasis).
So, I tend to think we should continue to ignore display:contents for MathML, and perhaps raise a spec issue on having that behavior specified.

r- until the MathML situation is clarified.

::: commit-message-8711a:1
(Diff revision 1)
> +Bug 1427292: Update display: contents on unusual elements to the spec. r?mats

nit: for spec-related changes like this I try to mention the relevant spec in the commit message (ala www-style).  Also, capitalizing "unusual elements" would make it clearer that it's a spec term rather than some elements that you find unusual.  Like so:

Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

::: layout/base/nsCSSFrameConstructor.h:843
(Diff revision 1)
> +                  bool* aFound = nullptr);
>  

nit: the name aFound seems slightly ambiguous (whether it means that the tag matched or a FrameConstructionData was found); perhaps aMatch or aFoundTag is clearer?
(I see that you documented it clearly, but I'd like the code to be clearer without having seen that comment)

::: layout/base/nsCSSFrameConstructor.cpp:3645
(Diff revision 1)
>  
>    return nullptr;

I'm not sure if we have a solid code convention for this, but I'd expect aFound to always be assigned when non-null.  I.e. "if (aFound) aFound = false;" before the return here.  Otherwise, a caller that declares a bool without initializing it might use an uninitialized variable.

I think I'd slightly prefer to always assign it, and not initialize the variables in the callers (and update the documentation to make it clear it's always assigned when non-null).

::: layout/base/nsCSSFrameConstructor.cpp:3740
(Diff revision 1)
>      SIMPLE_TAG_CREATE(progress, NS_NewProgressFrame),
>      SIMPLE_TAG_CREATE(meter, NS_NewMeterFrame),
>      COMPLEX_TAG_CREATE(details, &nsCSSFrameConstructor::ConstructDetailsFrame)
>    };
>  
> -  return FindDataByTag(aTag, aElement, aStyleContext, sHTMLData,
> +  bool found = false;

ditto re naming, e.g. "match" or "foundTag" here

::: layout/base/nsCSSFrameConstructor.cpp:3742
(Diff revision 1)
> +    FindDataByTag(aTag, aElement, aStyleContext, sHTMLData,
> +                  ArrayLength(sHTMLData), &found);

This doesn't handle <applet> according to spec though...
Given that we don't recognize <applet> at all, I think we should raise a spec issue rather than introduce special handling for it here.  It seems reasonable to request that for UAs that don't support <applet> at all that it should be treated as <foo>, i.e. display:contents "behave as normal" rather than display:none.

::: layout/base/nsCSSFrameConstructor.cpp:4530
(Diff revision 1)
> +  // There's no spec that says what to do for XUL, but we do the same than for
> +  // HTML for consistency.

nit: this comment can be clearer, I suggest:

There's no spec that says what display:contents means for XUL elements, but we do the same as for HTML "Unusual Elements", i.e. treat it as display:none.

::: layout/svg/nsSVGUseFrame.cpp:172
(Diff revision 1)
>  nsSVGUseFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
>  {
> -  SVGUseElement *use = static_cast<SVGUseElement*>(GetContent());
> -
> +  // FIXME(emilio): This should not be done at frame construction time, but
> +  // using Shadow DOM or something like that instead, to support properly
> +  // display: contents in <svg:use>.
> +  auto* use = static_cast<SVGUseElement*>(GetContent());

nit: I think the established code convention is:
auto var = static_cast<T*>(...);

(i.e. drop the redundant * after auto)

Comment 4

[Mats Palmgren (inactive)]

Attachment: nested <legend> test

Also, can you make sure that we test nested <legend> somewhere?
e.g. something like this should work I think.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939032 [details]
Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

https://reviewboard.mozilla.org/r/209464/#review216410

::: layout/base/nsCSSFrameConstructor.h:843
(Diff revision 1)
> +                  bool* aFound = nullptr);
>  

Sure, sounds good.

::: layout/base/nsCSSFrameConstructor.cpp:3742
(Diff revision 1)
> +    FindDataByTag(aTag, aElement, aStyleContext, sHTMLData,
> +                  ArrayLength(sHTMLData), &found);

Yeah, that's what Chromium does fwiw, I will.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939032 [details]
Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209464/diff/1-2/

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8939032 [details]
Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

https://reviewboard.mozilla.org/r/209464/#review216432

Looks good to me, thanks for fixing this!  r=mats (with the typo below fixed)

::: layout/base/nsCSSFrameConstructor.cpp:3627
(Diff revision 2)
> +                                     bool* aTagFound)
>  {
> +  if (aTagFound) {
> +    *aTagFound = true;
> +  }

s/true/false/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939032 [details]
Bug 1427292: [css-display] Update display: contents on Unusual Elements to the spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209464/diff/2-3/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #7)
> ::: layout/base/nsCSSFrameConstructor.cpp:3627
> (Diff revision 2)
> > +                                     bool* aTagFound)
> >  {
> > +  if (aTagFound) {
> > +    *aTagFound = true;
> > +  }
> 
> s/true/false/

Whoopsies. Well, try caught it at least :)

Thanks for the review!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68d4fd90cd47
[css-display] Update display: contents on Unusual Elements to the spec. r=mats

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/68d4fd90cd47

Comment 12

[Chris Mills [:cmills]]

I've documented this:

Added a note to the description of the contents value on the display reference page:
https://developer.mozilla.org/en-US/docs/Web/CSS/display#Syntax
(search for <display-box> in this section of the page)

Added a note to the Fx59 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/59#CSS

Updated the browser compat data for display to cover this stuff:
https://github.com/mdn/browser-compat-data/pull/895

Let me know if that sounds OK. Thanks!

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Looks good, thanks!