Closed
Bug 1427292
Opened 6 years ago
Closed 6 years ago
Update display: contents to the spec regarding replaced elements / svg and such.
Categories
(Core :: Layout, enhancement)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
Assignee | ||
Updated•6 years ago
|
Component: CSS Parsing and Computation → Layout
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd7707e66a8ae4743f46c9a28c3003ba0097c80
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 3•6 years ago
|
||
mozreview-review |
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-
Comment 4•6 years ago
|
||
Also, can you make sure that we test nested <legend> somewhere? e.g. something like this should work I think.
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 7•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
(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•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68d4fd90cd47
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 12•6 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•