If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Update the default style of summary element to match the html spec

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla48
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
The rendering hint of the details and summary elements in HTML spec has been updated to the following [1]:

summary {
  display: list-item;
  list-style: disclosure-closed inside;
}
details[open] > summary {
  list-style-type: disclosure-open;
}

That is, we can use the CSS to render the disclosure triangle of the summary.

Currently, we use "display:block" for summary and render the triangle by inserting a nsBulletFrame if the summary's style is not "display:list-item" as in [2]. That's the sole purpose of the existence of SummaryFrame.

So if we only change the default style for summary, everything should stay as it was including the the stylability of by using ::-moz-list-bullet.

However, if we further want to drop the SummayFrame and use standard block frame to render summary, then we have the following trade-offs:

Pros:
- The code gets simpler, and summary gets more layout abilities such as column (bug 1245036) or flex for free.

Cons: 
- If the web page uses 'display: block' for the summary, then the triangle is gone. (This might be become a pro if the page further specifies "summary::-webkit-details-marker { display:none; } and add its own marker by summary::before or summary::after like [3])

[1] https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
[2] https://dxr.mozilla.org/mozilla-central/rev/4037eb98974db1b1e0b5012c8a7f3a36428eaa11/layout/generic/SummaryFrame.cpp#39-49
[3] https://dev.windows.com/en-us/microsoft-edge/platform/faq/
(Assignee)

Updated

2 years ago
Blocks: 1226455
(Assignee)

Comment 1

2 years ago
I found at least two issues by using CSS to render the disclosure triangle for summary elements.

1) I don't know how to un-apply the CSS rule dynamically when dom.details_element.enabled is false, which make details and summary feature cannot be turned off.

2) If summary is a list-item, then in the following test cases, the "Second item" will be displayed as "3. Second item".

    <ol>
      <li>First item
        <details open>
          <summary>Summary</summary>
          <p>This is the details.</p>
        </details>
      </li>
      <li>Second item</li>
    </ol>
We can figure out #1, but #2 is a spec-level issue, right?
(In reply to Boris Zbarsky [:bz] from comment #2)
> We can figure out #1, but #2 is a spec-level issue, right?

I'd say #1 is harder. We probably need to invent a new internal-only syntax for @supports to detect the pref?

#2 is easy for the spec-level, we just need to state that, details, like ol and ul, resets the list-item counter. And the presentational hint should add something like
> details { counter-reset: list-item; }
But this happens magically in our code, as we do not expose "list-item" as a real counter to the content.

I guess we should add details to this place [1] to handle this issue.


[1] https://dxr.mozilla.org/mozilla-central/rev/d5f3da0cfe7ccf846c354014c9b059fad6ba0de5/layout/generic/nsBlockFrame.cpp#6970-6973
> We probably need to invent a new internal-only syntax for @supports to detect the pref?

Or switch to using a compile-time thing instead of a pref, or come up with a new specified value that computes based on the pref.  But yes, an @supports thing specifically for <details> support might be ok...  I'd just worry about letting that out on the web and people starting to use it.

But ok, fair.  This is a bit more work than the other..
See Also: → bug 1259889
(Assignee)

Comment 5

2 years ago
Created attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

- Not consider prefernece "dom.details_element.enabled" off yet.
- The disclosure triangle still show when summary has "display: block"

The spec for reference:
https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements

Review commit: https://reviewboard.mozilla.org/r/43385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43385/
(Assignee)

Updated

2 years ago
Depends on: 1259889
See Also: bug 1259889
(Assignee)

Updated

2 years ago
Assignee: nobody → tlin
(Assignee)

Comment 6

2 years ago
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43385/diff/1-2/
(Assignee)

Comment 7

2 years ago
Created attachment 8739042 [details]
MozReview Request: Bug 1258657 Part 2 - Remove SummaryFrame.

We can use nsBlockFrame to render summary elements.

This change undoes "Bug 591737 - Add SummaryFrame" and remove
summaryFrame usage in DetailsFrame and nsCSSFrameConstructor.

Review commit: https://reviewboard.mozilla.org/r/45021/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45021/
Attachment #8736560 - Attachment description: MozReview Request: Bug 1258657 - Update summary default style to match the spec. → MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.
Attachment #8739042 - Flags: review?(bzbarsky)
Attachment #8736560 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43385/diff/2-3/
I'm sorry for the lag here; I'm not likely to get to this before Monday.  :(
Attachment #8736560 - Flags: review?(bzbarsky)
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

https://reviewboard.mozilla.org/r/43385/#review41827

OK, I was wrong, I did manage to get to at least part of this...

::: layout/style/res/html.css:787
(Diff revision 3)
> -  details[open] > summary::-moz-list-bullet {
> +  details[open] > summary:first-of-type,
> +  details[open] > summary:-moz-native-anonymous {
>      list-style-type: disclosure-open;
>    }
> +
> +  summary :-moz-any(ol, ul, menu, dir) {

This selector seems pretty slow, and I don't see it in the spec.  Am I just missing it?  Why is it here?
Attachment #8739042 - Flags: review?(bzbarsky) → review+
Comment on attachment 8739042 [details]
MozReview Request: Bug 1258657 Part 2 - Remove SummaryFrame.

https://reviewboard.mozilla.org/r/45021/#review41829

r=me
https://reviewboard.mozilla.org/r/43385/#review41827

> This selector seems pretty slow, and I don't see it in the spec.  Am I just missing it?  Why is it here?

It seems to be something worth adding into the spec... I guess it is another argument for not using list-item on summary...
(Assignee)

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/43385/#review41827

> It seems to be something worth adding into the spec... I guess it is another argument for not using list-item on summary...

I add the rule because "list-style-position: inside" for summary will be inherited down to all li elements, which have "list-style-position: outside" by default. I raise a spec issue at https://github.com/whatwg/html/issues/1020
OK.  I think we should avoid adding this slow selector if we can...  It's possible the bloom filter stuff will make it ok, but I'd rather not rely on it.
(Assignee)

Comment 15

2 years ago
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43385/diff/3-4/
Attachment #8736560 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8739042 [details]
MozReview Request: Bug 1258657 Part 2 - Remove SummaryFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45021/diff/1-2/
(Assignee)

Comment 17

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #14)
> OK.  I think we should avoid adding this slow selector if we can...  It's
> possible the bloom filter stuff will make it ok, but I'd rather not rely on
> it.

Per discussion in irc, heycam suggests to use |details > summary:first-of-type > * { list-style-position: initial; }|.  I've updated the patch accordingly.  It should be rare that summary will contain list items in real website though ...

BTW, we had many slow selectors like this.  https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/layout/style/res/html.css#612
Status: NEW → ASSIGNED
> BTW, we had many slow selectors like this.

No, those selectors will only be evaluated for ul, menu, and dir elements, because they have a tag in the rightmost position.  Your selector would be evaluated for all elements.

There's a reason the existing rules were not written like so:

  :-moz-any(ol, ul, menu, dir) :-moz-any(ol, ul, menu, dir) :-moz-any(ul, menu, dir)

and that's because it's slower.
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

https://reviewboard.mozilla.org/r/43385/#review42407

I guess I can live with this.  Again, hopefully the Bloom filter will optimize this out in the common case...
Attachment #8736560 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #18)
> > BTW, we had many slow selectors like this.
> 
> No, those selectors will only be evaluated for ul, menu, and dir elements,
> because they have a tag in the rightmost position.  Your selector would be
> evaluated for all elements.
> 
> There's a reason the existing rules were not written like so:
> 
>   :-moz-any(ol, ul, menu, dir) :-moz-any(ol, ul, menu, dir) :-moz-any(ul,
> menu, dir)
> 
> and that's because it's slower.

Aha! Thanks for the explanation. Instead of |details > summary:first-of-type > * | I think I can make the selector more verbose but faster like:

  details > summary:first-of-type > ol,
  details > summary:first-of-type > ul,
  details > summary:first-of-type > menu,
  details > summary:first-of-type > dir {
    list-style-position: initial;
  }
Won't that fail cases like:

  <details>
    <summary>
      <div>
        <ol></ol>
      </div>
    </summary>
  </details>

?
(Assignee)

Comment 22

2 years ago
bz, you're right. The cases in comment 21 will fail if I change the selector as in comment 20. So we still need |details > summary:first-of-type > *|. BTW, I'll put the test in comment 21 to details-in-ol.html, and land the patch.
(Assignee)

Comment 23

2 years ago
Comment on attachment 8736560 [details]
MozReview Request: Bug 1258657 Part 1 - Change summary default style to "display: list-item" to match html spec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43385/diff/4-5/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8739042 [details]
MozReview Request: Bug 1258657 Part 2 - Remove SummaryFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45021/diff/2-3/

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9894253461b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c23caab8a0d
(Assignee)

Comment 26

2 years ago
This change might need a document update. 

Like comment #0 said, this bug changes the summary default display property from "display: block" to "display: list-item" to match the spec, and rendering the disclosure triangle by CSS. 

Besides, we drop SummaryFrame, so if web pages style "display: block" for summary element, the disclosure triangle will disappear.
Keywords: dev-doc-needed
The other thing we should think about, I just realized, is whether it should be "*" or "*|*" in that last part of the selector.  What should happen if that <div> in my example is replaced with <svg><foreignObject>?
Flags: needinfo?(tlin)

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9894253461b
https://hg.mozilla.org/mozilla-central/rev/1c23caab8a0d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Blocks: 1264533
(Assignee)

Comment 29

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> The other thing we should think about, I just realized, is whether it should
> be "*" or "*|*" in that last part of the selector.  What should happen if
> that <div> in my example is replaced with <svg><foreignObject>?

You've found another bug. Assume <svg><foreignObject> is in different namespace [1], then we should use "*|*". Otherwise the <ul> in <foreignObject> will still have "list-style-position: inside". Filed bug 1264533.

[1] https://developer.mozilla.org/en-US/docs/Web/SVG/Element/foreignObject
Flags: needinfo?(tlin)
You need to log in before you can comment on or make changes to this bug.