Closed Bug 1258657 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

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/
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: → 1259889
- 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/
Depends on: 1259889
See Also: 1259889
Assignee: nobody → tlin
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/
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)
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...
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.
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)
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/
(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+
(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>

?
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.
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/
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/
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)
Blocks: 1264533
(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.