Intent to implement: display: contents
230 views
Skip to first unread message
Emilio Cobos Álvarez
Oct 20, 2016, 6:53:36 AM10/20/16
to blin...@chromium.org, styl...@chromium.org
Contact emails: eco...@igalia.com, re...@igalia.com
Spec: https://drafts.csswg.org/css-display/#box-generation
Summary:
Implement support for the contents keyword value for the display
property. The contents keyword suppresses generation of layout boxes
for elements which have a computed display of contents. In contrast to
display:none, descendants of elements with display:contents will still
generate layout boxes.
Rune discussed in his draft intent[1] part of the implementation plan,
I plan to stick to most of that plan and identified TODOs.
I've made a rough intent this week just modifying
LayoutTreeBuilderTraversal, and got the expected layout in the acid
test that Gecko has[2], but that's probably not going to be enough,
given that ElementResolveContext uses LayoutTreeBuilderTraversal to
decide the style parent, so I got incorrect styling. My plan to
address this is to split LayoutTreeBuilderTraversal into
LayoutTreeBuilderTraversal and StyleTreeTraversal (better name
suggestions appreciated), making the former based on the later,
filtering display: contents elements on top of it.
Other of the other immediate concerns is where to store the element's
style (given it can't be stored in the LayoutObject). Gecko's
implementation has a HashMap[3] where they store the styles, but for
now I'm using the suggested idea of using the rare data, and it seems
to be working well, though I still need to worry about where to store
the pseudo-elements' style.
Any idea, comment or suggestion here would be very appreciated, I have
different approaches in mind but my plan is to verify them and stick
with the most elegant one.
Motivation:
The <slot> element introduced by Shadow DOM v1 should be part of the
flat tree and be rendered as display:contents by default. Since we
currently do not support display:contents, we have shipped an
implementation of Shadow DOM v1 which does not include <slot> in the
flat tree which means:
* Inheritance through <slot> does not work correctly
* You cannot display <slot> elements by setting the display property
in author style.
* Both points above will cause interop issues with other
implementations when shipped. See next section.
Also, this is a feature that allows some nice use-cases when combined
with CSS grid and flexbox (see [4], for example).
Interoperability and Compatibility Risk:
Gecko has shipped display:contents[5][6] since Firefox 37. When Gecko
ships Shadow DOM v1, they will presumably implement <slot> correctly
in this regard.
It looks like WebKit has implemented display: contents, but only for
the <slot> element ([7] enables it as CSS feature, but they already
had some hard-coded support from looking at that patch). They do want
to implement it properly, though[8].
Don't know about IE/Edge.
Ongoing technical constraints: None
Will this feature be supported on all six Blink platforms (Windows, Mac,
Linux, Chrome OS, Android, and Android WebView)?
Yes
OWP launch tracking bug: https://bugs.chromium.org/p/chromium/issues/detail?id=657748
Link to entry on the feature dashboard: https://www.chromestatus.com/feature/5663606012116992
Requesting approval to ship?
No
[1]: https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/nJz270uSkuA
[2]: http://searchfox.org/mozilla-central/source/layout/reftests/css-display/display-contents-acid.html
[3]: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/base/nsFrameManagerBase.h#58
[4]: https://rachelandrew.co.uk/archives/2016/01/29/vanishing-boxes-with-display-contents/
[5]: https://bugzil.la/907396
[6]: https://bugzil.la/1105369
[7]: https://bugs.webkit.org/show_bug.cgi?id=149439
[8]: https://bugs.webkit.org/show_bug.cgi?id=157477
Spec: https://drafts.csswg.org/css-display/#box-generation
Summary:
Implement support for the contents keyword value for the display
property. The contents keyword suppresses generation of layout boxes
for elements which have a computed display of contents. In contrast to
display:none, descendants of elements with display:contents will still
generate layout boxes.
Rune discussed in his draft intent[1] part of the implementation plan,
I plan to stick to most of that plan and identified TODOs.
I've made a rough intent this week just modifying
LayoutTreeBuilderTraversal, and got the expected layout in the acid
test that Gecko has[2], but that's probably not going to be enough,
given that ElementResolveContext uses LayoutTreeBuilderTraversal to
decide the style parent, so I got incorrect styling. My plan to
address this is to split LayoutTreeBuilderTraversal into
LayoutTreeBuilderTraversal and StyleTreeTraversal (better name
suggestions appreciated), making the former based on the later,
filtering display: contents elements on top of it.
Other of the other immediate concerns is where to store the element's
style (given it can't be stored in the LayoutObject). Gecko's
implementation has a HashMap[3] where they store the styles, but for
now I'm using the suggested idea of using the rare data, and it seems
to be working well, though I still need to worry about where to store
the pseudo-elements' style.
Any idea, comment or suggestion here would be very appreciated, I have
different approaches in mind but my plan is to verify them and stick
with the most elegant one.
Motivation:
The <slot> element introduced by Shadow DOM v1 should be part of the
flat tree and be rendered as display:contents by default. Since we
currently do not support display:contents, we have shipped an
implementation of Shadow DOM v1 which does not include <slot> in the
flat tree which means:
* Inheritance through <slot> does not work correctly
* You cannot display <slot> elements by setting the display property
in author style.
* Both points above will cause interop issues with other
implementations when shipped. See next section.
Also, this is a feature that allows some nice use-cases when combined
with CSS grid and flexbox (see [4], for example).
Interoperability and Compatibility Risk:
Gecko has shipped display:contents[5][6] since Firefox 37. When Gecko
ships Shadow DOM v1, they will presumably implement <slot> correctly
in this regard.
It looks like WebKit has implemented display: contents, but only for
the <slot> element ([7] enables it as CSS feature, but they already
had some hard-coded support from looking at that patch). They do want
to implement it properly, though[8].
Don't know about IE/Edge.
Ongoing technical constraints: None
Will this feature be supported on all six Blink platforms (Windows, Mac,
Linux, Chrome OS, Android, and Android WebView)?
Yes
OWP launch tracking bug: https://bugs.chromium.org/p/chromium/issues/detail?id=657748
Link to entry on the feature dashboard: https://www.chromestatus.com/feature/5663606012116992
Requesting approval to ship?
No
[1]: https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/nJz270uSkuA
[2]: http://searchfox.org/mozilla-central/source/layout/reftests/css-display/display-contents-acid.html
[3]: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/base/nsFrameManagerBase.h#58
[4]: https://rachelandrew.co.uk/archives/2016/01/29/vanishing-boxes-with-display-contents/
[5]: https://bugzil.la/907396
[6]: https://bugzil.la/1105369
[7]: https://bugs.webkit.org/show_bug.cgi?id=149439
[8]: https://bugs.webkit.org/show_bug.cgi?id=157477
Rune Lillesveen
Oct 20, 2016, 9:42:41 AM10/20/16
to Emilio Cobos Álvarez, blink-dev, style-dev
On Thu, Oct 20, 2016 at 12:53 PM, Emilio Cobos Álvarez
<eco...@igalia.com> wrote:
> Rune discussed in his draft intent[1] part of the implementation plan,
> I plan to stick to most of that plan and identified TODOs.
>
> I've made a rough intent this week just modifying
> LayoutTreeBuilderTraversal, and got the expected layout in the acid
> test that Gecko has[2], but that's probably not going to be enough,
I have upstreamed a few tests to the csswg suite:
<eco...@igalia.com> wrote:
> Rune discussed in his draft intent[1] part of the implementation plan,
> I plan to stick to most of that plan and identified TODOs.
>
> I've made a rough intent this week just modifying
> LayoutTreeBuilderTraversal, and got the expected layout in the acid
> test that Gecko has[2], but that's probably not going to be enough,
https://github.com/w3c/csswg-test/tree/master/css-display-3
They need to be imported into blink. As you write more tests, please upstream.
> given that ElementResolveContext uses LayoutTreeBuilderTraversal to
> decide the style parent, so I got incorrect styling. My plan to
> address this is to split LayoutTreeBuilderTraversal into
> LayoutTreeBuilderTraversal and StyleTreeTraversal (better name
> suggestions appreciated), making the former based on the later,
> filtering display: contents elements on top of it.
LayoutTreeBuilderForElement for finding m_layoutObjectParent instead.
It already has special code for first-letter and top layer.
> Other of the other immediate concerns is where to store the element's
> style (given it can't be stored in the LayoutObject). Gecko's
> implementation has a HashMap[3] where they store the styles, but for
> now I'm using the suggested idea of using the rare data, and it seems
> to be working well, though I still need to worry about where to store
> the pseudo-elements' style.
--
Rune Lillesveen
Naina Raisinghani
Oct 20, 2016, 7:46:17 PM10/20/16
to Rune Lillesveen, Emilio Cobos Álvarez, blink-dev, style-dev
Hi Emilio,
Bugs and I have been working on a change that changes the way Computed Style is stored when we hit Element::recalcOwnStyle(), we store it on a HashMap on Document called m_nonAttachedStyle (this is temporary, there will be optimization and we won't unconditionally store the ComputedStyle on the HashMap). The design document and the CL list is here. Please read through and feel free to comment with questions and we can work through to make sure our changes don't inconvenience your plans.
Regards,
Naina
Bugs and I have been working on a change that changes the way Computed Style is stored when we hit Element::recalcOwnStyle(), we store it on a HashMap on Document called m_nonAttachedStyle (this is temporary, there will be optimization and we won't unconditionally store the ComputedStyle on the HashMap). The design document and the CL list is here. Please read through and feel free to comment with questions and we can work through to make sure our changes don't inconvenience your plans.
Regards,
Naina
--
You received this message because you are subscribed to the Google Groups "style-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to style-dev+...@chromium.org.
To post to this group, send an email to styl...@chromium.org.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/style-dev/CANz6XvQEqMME9dXVpE9XSa2axeKiULx%3DKOZUH0v%2B9hBYgXuSug%40mail.gmail.com.
Emilio Cobos Álvarez
Oct 21, 2016, 5:02:55 AM10/21/16
to Rune Lillesveen, blink-dev, style-dev
Hi Rune,
On Thu, Oct 20, 2016 at 03:42:24PM +0200, Rune Lillesveen wrote:
> I have upstreamed a few tests to the csswg suite:
>
> https://github.com/w3c/csswg-test/tree/master/css-display-3
>
> They need to be imported into blink. As you write more tests, please upstream.
I'll make sure they're imported, and that mine are upstreamed. In
general I think keeping our implementation compatible with Gecko is a
requirement before shipping. Thanks for pointing at them :)
> When I looked at this, I thought about changing the implementation of
> LayoutTreeBuilderForElement for finding m_layoutObjectParent instead.
> It already has special code for first-letter and top layer.
Yeah, that might end up being simpler, so it's worth trying. I agree
that keeping LayoutTreeBuilderTraversal something well-defined is
important, so I make sure all the changes I make to it and
LayoutTreeBuilder go through your eyes :).
--Emilio
On Thu, Oct 20, 2016 at 03:42:24PM +0200, Rune Lillesveen wrote:
> I have upstreamed a few tests to the csswg suite:
>
> https://github.com/w3c/csswg-test/tree/master/css-display-3
>
> They need to be imported into blink. As you write more tests, please upstream.
general I think keeping our implementation compatible with Gecko is a
requirement before shipping. Thanks for pointing at them :)
> When I looked at this, I thought about changing the implementation of
> LayoutTreeBuilderForElement for finding m_layoutObjectParent instead.
> It already has special code for first-letter and top layer.
that keeping LayoutTreeBuilderTraversal something well-defined is
important, so I make sure all the changes I make to it and
LayoutTreeBuilder go through your eyes :).
--Emilio
Emilio Cobos Álvarez
Oct 21, 2016, 5:20:27 AM10/21/16
to Naina Raisinghani, Rune Lillesveen, blink-dev, style-dev
Hi Naina,
On Thu, Oct 20, 2016 at 11:45:59PM +0000, Naina Raisinghani wrote:
> Bugs and I have been working on a change that changes the way Computed
> Style is stored when we hit Element::recalcOwnStyle(), we store it on a
> HashMap on Document called m_nonAttachedStyle (this is temporary, there
> will be optimization and we won't unconditionally store the ComputedStyle
> on the HashMap). The design document and the CL list is here
> <https://docs.google.com/document/d/1RlEfhfUnhIEJSgV0nb8AKT4uyWbNsSY_k9uSGTUCQ2I/edit?pli=1#heading=h.vqxw428kkc64>.
I think it won't interfere a lot with my plans, though I'm still
grasping how incremental restyling works in blink.
From a quick look at the code, it seems that styles in this map aren't
persistently stored there, but cleared after layout tree construction. I
*think* I'll need the display contents style to persist across restyles,
so to use that I'd need to complicate a bit more the logic.
I think removing the style from the map when the relevant layout object
is attached might just work? We can assert after layout tree
construction that the nodes in the map don't have a LayoutObject. That
will still leave display: none styles in that map too though.
The cool part (if I'm not wrong) is that we could use that instead of
the rare data for all kind of nodes without an attached layout object
(if we use the owner document, since I think you can call
getComputedStyle for a detached node).
In any case, that would change part of your proposed design, and I'm not
sure if you want to move in that direction. I think we don't *need* to
change how that works to support display contents, and it'd need a bit
more thought if we decide to do it, but definitely let me know if you
think it's worth it, and I'll try to integrate it more tightly with your
design :)
--Emilio
> You received this message because you are subscribed to the Google Groups "blink-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
On Thu, Oct 20, 2016 at 11:45:59PM +0000, Naina Raisinghani wrote:
> Bugs and I have been working on a change that changes the way Computed
> Style is stored when we hit Element::recalcOwnStyle(), we store it on a
> HashMap on Document called m_nonAttachedStyle (this is temporary, there
> will be optimization and we won't unconditionally store the ComputedStyle
> on the HashMap). The design document and the CL list is here
> Please read through and feel free to comment with questions and we can work
> through to make sure our changes don't inconvenience your plans.
I had definitely read that, thanks for pointing at it :).
> through to make sure our changes don't inconvenience your plans.
I think it won't interfere a lot with my plans, though I'm still
grasping how incremental restyling works in blink.
From a quick look at the code, it seems that styles in this map aren't
persistently stored there, but cleared after layout tree construction. I
*think* I'll need the display contents style to persist across restyles,
so to use that I'd need to complicate a bit more the logic.
I think removing the style from the map when the relevant layout object
is attached might just work? We can assert after layout tree
construction that the nodes in the map don't have a LayoutObject. That
will still leave display: none styles in that map too though.
The cool part (if I'm not wrong) is that we could use that instead of
the rare data for all kind of nodes without an attached layout object
(if we use the owner document, since I think you can call
getComputedStyle for a detached node).
In any case, that would change part of your proposed design, and I'm not
sure if you want to move in that direction. I think we don't *need* to
change how that works to support display contents, and it'd need a bit
more thought if we decide to do it, but definitely let me know if you
think it's worth it, and I'll try to integrate it more tightly with your
design :)
--Emilio
> To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
Naina Raisinghani
Oct 25, 2016, 11:22:06 PM10/25/16
to Emilio Cobos Álvarez, Rune Lillesveen, blink-dev, style-dev, Bugs Nash
+Bugs Nash who is working on this with me.
To start with some context: Currently we are storing the ComputedStyle on Document unconditionally. However, by the end of the Squad project we intend to store it on the DataUnion (we will modify the DataUnion to allow a ComputedStyle as well) if possible and revert to the ElementRareData if that is available and only store it on the HashMap as a last resort. This is to make the solution more memory efficient as storing on the HashMap always has a lot of performance implications right now and more so when we switch to LayoutNG.
As far as I understand retaining the ComputedStyle on the HashMap in the cases of Elements that don't have a display (no LayoutObject) would have memory implications but is something that should work. Bugs can chime in here with more thoughts.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/style-dev/20161021092009.GB1386%40emilio-moz.
--
Regards,
Naina
Reply all
Reply to author
Forward
0 new messages