Closed Bug 1427292 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Component: CSS Parsing and Computation → Layout
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)
Attachment #8939032 - Flags: review?(mats) → review-
Attached file nested <legend> test
Also, can you make sure that we test nested <legend> somewhere? e.g. something like this should work I think.
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 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/
Attachment #8939032 - Flags: review?(mats) → review+
(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!
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/68d4fd90cd47 [css-display] Update display: contents on Unusual Elements to the spec. r=mats
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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!
Flags: needinfo?(emilio)
Looks good, thanks!
Flags: needinfo?(emilio)
Depends on: 1435015
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: