Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering: further details/summary cleanup #4746

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented Jul 1, 2019

As is evident from https://bugs.chromium.org/p/chromium/issues/detail?id=973074 the current description still had too much from the old description and led to some inconsistencies in implementations. This makes it clear the slots have no special styling (by default) and it's the first summary element that is special, not the first slot.

(See WHATWG Working Mode: Changes for more details.)


/rendering.html ( diff )

@domenic domenic added clarification Standard could be clearer topic: rendering labels Jul 2, 2019
@Loirooriol
Copy link
Contributor

Loirooriol commented Jul 30, 2019

It seems there is no WPT about this, just an internal Chromium test. See https://chromium-review.googlesource.com/c/chromium/src/+/1724676

@annevk annevk requested a review from emilio October 14, 2019 09:00
@annevk
Copy link
Member Author

annevk commented Oct 14, 2019

@Loirooriol could you maybe turn that into a WPT test?

@Loirooriol
Copy link
Contributor

@annevk That test uses internal Chromium APIs in order to access the flat tree, since the shadow tree is closed. I don't think I can do that in WPT.

@annevk
Copy link
Member Author

annevk commented Oct 14, 2019

@Loirooriol I think you should be able to write a reftest for this, given the original bug report.

@Loirooriol
Copy link
Contributor

Yes, but it seems suboptimal. If I use display: grid on <details>, it doesn't become a grid container. If it can ignore display: grid, couldn't it theoretically ignore display: contents too? Seems an implementation detail. So I would prefer to test the flat tree like in that internal test, but it doesn't seem possible.

@annevk
Copy link
Member Author

annevk commented Oct 15, 2019

@Loirooriol interesting point, ideally that's not an implementation detail and we define what works here. #1839 is also about this a bit (though for <summary>).

cc @emilio

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible. Did we decide on the pseudo-element story yet?

@annevk
Copy link
Member Author

annevk commented Oct 7, 2020

Pseudo-elements were discussed in #3805 and the conclusion there was that not enough people cared for it to warrant changes, as far as I can tell.

@domenic can you find someone on the Chrome side to conclude this? (I see you tried a year ago, might be worth trying again.)

As is evident from https://bugs.chromium.org/p/chromium/issues/detail?id=973074 the current description still had too much from the old description and led to some inconsistencies in implementations. This makes it clear the slots have no special styling (by default) and it's the first summary element that is special, not the first slot.
@annevk annevk force-pushed the annevk/details-summary-cleanup branch from af0f27e to 8642f26 Compare October 7, 2020 12:35
@domenic
Copy link
Member

domenic commented Oct 7, 2020

I poked @mfreed7 and @bfgeek internally. It would be good if we had some web platform test for this though, to make it clear what the compat and interop risks are of such a change.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 8, 2020

We're supportive of this change. I can take care of implementation on Chromium, and adding a WPT test. My plan (subject to comments here) would be to use the test case (converted to a reftest) provided by Igalia in this comment on the Chromium bug:

<div style="display: grid; grid-template-columns: auto auto;">
  <details style="display: contents;" open>
    <summary style="background: magenta; grid-column: 1 / 3;">foo</summary>
    <div style="background: cyan; grid-column: 1 / 3;">bar</div>
  </details>
</div>

@Loirooriol
Copy link
Contributor

If it can ignore display: grid, couldn't it theoretically ignore display: contents too? Seems an implementation detail.

This seems safer now with the resolution from w3c/csswg-drafts#4065:

Blockification section should use the computed value, not the box of the element

Therefore, in

<details style="display: grid" open>
  <summary style="display: inline">foo</summary>
  <div style="display: inline">bar</span>
</details>

both the <summary> and <div> should have getComputedStyle(el).display === "block". Even if the <details> is not a grid container (but assuming it keeps display: grid as the computed value).

And same for the following, even if the <details> ends up generating boxes (but assuming it keeps display: contents as the computed value):

<div style="display: grid">
  <details style="display: contents" open>
    <summary style="display: inline">foo</summary>
    <div style="display: inline">bar</span>
  </details>
</div>

@mfreed7 The implementation should be simple, https://chromium-review.googlesource.com/c/chromium/src/+/1724676.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 8, 2020

@mfreed7 The implementation should be simple, https://chromium-review.googlesource.com/c/chromium/src/+/1724676.

Thanks! I didn't know you already had a patch up - would have saved me the time of doing the same thing. Anyway, as you point out, relatively simple fix. Thanks for all the help here.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 8, 2020
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
@annevk annevk requested a review from domenic October 9, 2020 09:13
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 9, 2020
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 9, 2020
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 9, 2020
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Oct 9, 2020
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}
@mfreed7
Copy link
Contributor

mfreed7 commented Oct 9, 2020

Alright, this is now implemented in Chromium, and tests added here.

@annevk annevk merged commit 1076db6 into master Oct 12, 2020
@annevk annevk deleted the annevk/details-summary-cleanup branch October 12, 2020 09:53
@annevk
Copy link
Member Author

annevk commented Oct 12, 2020

Safari passes the test so I didn't file a bug there. However, Firefox also passes the test and @emilio has landed the fix yet...

@Loirooriol
Copy link
Contributor

The test checks that no wrapper element is added (which is what Chromium was doing).

For Firefox the problem is that it's using custom layout instead of shadow DOM. So Firefox will need a different test, like using details::before { content: "something" } and checking that the generated content appears before the summary.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 21, 2020
…y shadow dom, a=testonly

Automatic update from web-platform-tests
Remove anonymous box from details/summary shadow dom

Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}

--

wpt-commits: 411d1bfc11c5779664101d938b290e0dd26d812d
wpt-pr: 26053
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 24, 2020
…y shadow dom, a=testonly

Automatic update from web-platform-tests
Remove anonymous box from details/summary shadow dom

Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}

--

wpt-commits: 411d1bfc11c5779664101d938b290e0dd26d812d
wpt-pr: 26053
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Previously, there was an anonymous <div> in the shadow dom for the
<summary> element, which caused display:contents to not behave in an
interoperable way. See [1] for a spec PR that clarifies the spec,
and this CL matches the new clarification.

The new WPT added here passes already on Gecko and WebKit.

[1] whatwg/html#4746

Fixed: 973074
Change-Id: Iba54616c4e54e357dd71b7de30de18bde8ff3983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462217
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815731}
GitOrigin-RevId: 57f68063f26a1015f6acc649e14135e75eca55a2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: rendering
Development

Successfully merging this pull request may close these issues.

5 participants