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)
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•7 years ago
|
Component: CSS Parsing and Computation → Layout
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 3•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 12•7 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
•