1226455 - (ship-details-summary) ship support for <details> and <summary>

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron:]

This is a bug to ship our support for the <details> and <summary> elements in HTML.

Bugs that should block shipping should block this bug.

Comment 1

[David Baron :dbaron:]

TYLin, do you know of other bugs that should be added?

Comment 2

[Ting-Yu Lin [:TYLin] (UTC-8)]

dbaron, you've found all the bugs I know of. Thanks.

Comment 4

[Matthew N. [:MattN]]

Release Note Request: See bug 591737 comment 157

Comment 5

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems to me like we have automated tests covering this new implementation, and this has been enabled in Firefox 48 since March 16th. I don't think we need manual testing, but let me know if you think otherwise.

Comment 6

[Matthew N. [:MattN]]

The feature involves UI so our automated tests aren't going to catch UX issues. This also isn't used so widely on the web yet that it probably hasn't gotten much exposure with Nightly testers. Doing sanity checks with various test cases on different OS would be useful IMO.

Comment 7

[Ting-Yu Lin [:TYLin] (UTC-8)]

The "intent to ship" is discussed here.
https://groups.google.com/forum/#!msg/mozilla.dev.platform/FCQSOydoE-8/YeAp3VZIAgAJ

Comment 8

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: MozReview Request: Bug 1226455 - Enable <details> and <summary> on all channels.

Review commit: https://reviewboard.mozilla.org/r/46969/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46969/

Comment 9

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #6)
> The feature involves UI so our automated tests aren't going to catch UX
> issues. This also isn't used so widely on the web yet that it probably
> hasn't gotten much exposure with Nightly testers. Doing sanity checks with
> various test cases on different OS would be useful IMO.

Thanks for clarifying. Setting qe-verify+ so we get some sanity checks on various OSes, in Beta at its latest (once set to Resolved this will show up in our feature monitoring lists).

Some quick examples to cover during verification:
- http://simpl.info/details/
- https://drafts.csswg.org/css-backgrounds-4/
- http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_summary

Comment 10

[Boris Zbarsky [:bzbarsky]]

What's the conclusion on the marker thing?

Comment 11

[Ting-Yu Lin [:TYLin] (UTC-8)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> What's the conclusion on the marker thing?

I'd like to unblock summary::marker (bug 1221416) from shipping because no other browsers support it currently. For the webcompat point of view, even if we support summary::marker, our usage to customize the marker will still be different from summary::-webkit-details-marker usage on Blink/Webkit, which is another reason to unblock.

Comment 12

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #9)
> Some quick examples to cover during verification:
> - http://simpl.info/details/
> - https://drafts.csswg.org/css-backgrounds-4/
> - http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_summary
additional from Karl - http://codepen.io/anon/pen/bpKGXg

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, can you clearly explain the marker situation in the intent to ship thread, please?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8742184 [details]
MozReview Request: Bug 1226455 - Enable <details> and <summary> on all channels.

https://reviewboard.mozilla.org/r/46969/#review44159

r=me assuming dbaron has signed off on the ::marker situation.

Comment 15

[Ting-Yu Lin [:TYLin] (UTC-8)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> OK, can you clearly explain the marker situation in the intent to ship
> thread, please?

OK. I'll explain that we'd like to defer the implementation of ::marker with explanations like comment 11.

NI dbaron for comment 14.

Comment 16

[David Baron :dbaron:]

What does the spec say should work for styling the marker (display, list-style-type, ::marker, etc.), and what does each engine support (out of that set or other things)?

Comment 17

[Ting-Yu Lin [:TYLin] (UTC-8)]

The html spec [1] says summary should have "display: list-item". This is a recent change in [2]. So summary should be able to use "list-style-type", "list-style-position", "list-style-image", "::marker" [2], and other things that a list-item can be used.

I had changed our implementation to match the spec in bug 1258657, so we support all the list-item properties except ::marker. For example, we can change the triangle to a circle by |summary {list-style-type: circle;}|, or omit the triangle by |summary {list-style-type: none;}|. And we do not support the summary::-webkit-details-marker.

On Blink/Webkit, the summary is still "display: block" due to the old spec. All the list-item properties do not work even if author sets summary to be "display: list-item". The only way to style the marker is via |summary::-webkit-details-marker|. For example, use |summary::-webkit-details-marker {display: none;}| to omit the triangle.

[1] https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
[2] https://github.com/whatwg/html/issues/722
[3] https://drafts.csswg.org/css-pseudo-4/#marker-pseudo

Comment 18

[David Baron :dbaron:]

Attachment: some tests of marker styling

Comment 19

[David Baron :dbaron:]

Attachment: some tests of marker styling

Comment 20

[David Baron :dbaron:]

OK, shipping this sounds good to me.

Given that Edge doesn't support details/summary yet, I'm less worried about compatibility issues with the styling, although it's possible we might have to cycle back here in the future if they don't change.  (I was hoping the answer to comment 16 would have told me that Edge didn't implement details/summary, but I found that out from writing the above testcase.)

Could you point me to a Chromium bug about implementing the current HTML spec's rules on styling summary?  Or if they don't have one, could you either file one, or needinfo? me to file one.

(I didn't test WebKit.  If they match Chromium, there should be a bug for them as well...)

Comment 21

[Ting-Yu Lin [:TYLin] (UTC-8)]

Thanks. Let's ship this.

You're right that Edge does not implement details/summary yet. Sorry for not being clear about that.

Here is the Chromium bug: "Implement <details>/<summary> disclosure triangle as a list item with ::marker"
https://bugs.chromium.org/p/chromium/issues/detail?id=590014

I don't know whether webkit has a bug for implement current HTML spec on styling the summary. Would you help file one?

Comment 22

[Boris Zbarsky [:bzbarsky]]

https://bugs.webkit.org/

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3152d79aa593

Comment 24

[David Baron :dbaron:]

Since this doesn't modify behavior on nightly or aurora (but only enables the feature on beta and release), you may as well request aurora approval as well.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3152d79aa593

Comment 26

[David Baron :dbaron:]

Filed https://bugs.webkit.org/show_bug.cgi?id=157323

Comment 27

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8742184 [details]
MozReview Request: Bug 1226455 - Enable <details> and <summary> on all channels.

Approval Request Comment
[Feature/regressing bug #]: HTML <details> and <summary>
[User impact if declined]: Without this, beta users need to wait until 49 to use this feature. Aurora 48 already enabled this in bug 1241750.
[Describe test coverage new/current, TreeHerder]: Automated tests in tree and verification in comment 9.
[Risks and why]: Low. Only preference changes. No new bug or regression since the enabling of non-release channel.
[String/UUID change made/needed]: None.

Comment 28

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #27)
> [Describe test coverage new/current, TreeHerder]: Automated tests in tree
> and verification in comment 9.

Note that this hasn't been verified yet. It was only tagged to have it on the QA team radar.

Comment 29

[David Baron :dbaron:]

I don't think verification is useful here.  If Web developers who are using it find problems, they will report them, and I don't think there's a need to gate shipping the feature on any form of QA verification (which I think is unlikely to uncover the most relevant problems).

Comment 30

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Yes, I think we can just get some sanity checks as MattN suggested in comment #6.

Comment 31

[Paul Silaghi, QA [:pauly]]

Adding a display:block in the <summary> makes the expand arrow disappear. In Chrome, it doesn't.
Thoughts? http://codepen.io/anon/pen/eZbMja

Comment 32

[David Baron :dbaron:]

(In reply to Paul Silaghi, QA [:pauly] from comment #31)
> Adding a display:block in the <summary> makes the expand arrow disappear. In
> Chrome, it doesn't.
> Thoughts? http://codepen.io/anon/pen/eZbMja

This is expected, as discussed in comment 15, comment 20, comment 21, etc., and in bug 1258657.

Comment 33

[Marco Zehe (:MarcoZ)]

Making sure accessibility API support is on the radar for this. This cannot ship without it.

Comment 34

[Sebastian Zartner [:sebo]]

As this bug is fixed now, should the related preference be removed? And should I file a new bug for that?

Sebastian

Comment 35

[Sebastian Zartner [:sebo]]

Updated the documentation:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/summary
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

And added this to the release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/49

Sebastian

Comment 36

[Ting-Yu Lin [:TYLin] (UTC-8)]

I though I had file a bug for removing the pref, but found nothing. Filed bug 1271549.

Comment 37

[David Baron :dbaron:]

(In reply to Marco Zehe (:MarcoZ) from comment #33)
> Making sure accessibility API support is on the radar for this. This cannot
> ship without it.

Agreed.  Not sure why that got removed from the dependencies, but this needs to be addressed.  (Also, perhaps keyboard accessibility and accessibility API support should be different bugs.  Though it might be useful to know what other implementations do for both.)

Comment 38

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8742184 [details]
MozReview Request: Bug 1226455 - Enable <details> and <summary> on all channels.

Remove uplift request per comment 37, so we'll still need to land the support to accessibility API in Firefox 49.

Comment 39

[Ting-Yu Lin [:TYLin] (UTC-8)]

Using keyboard key ('enter' and 'space') to toggle details element had been implemented in bug 1249556. 

The bug to support accessibility on webkit and blink was mentioned in bug 634004 comment 11 and bug 634004 comment 12. Both bugs are resolved, but I lack the knowledge to test the completeness of their implementations.

Comment 40

[Paul Silaghi, QA [:pauly]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #9)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #6)
> > The feature involves UI so our automated tests aren't going to catch UX
> > issues. This also isn't used so widely on the web yet that it probably
> > hasn't gotten much exposure with Nightly testers. Doing sanity checks with
> > various test cases on different OS would be useful IMO.
> 
> Thanks for clarifying. Setting qe-verify+ so we get some sanity checks on
> various OSes, in Beta at its latest (once set to Resolved this will show up
> in our feature monitoring lists).
> 
> Some quick examples to cover during verification:
> - http://simpl.info/details/
> - https://drafts.csswg.org/css-backgrounds-4/
> - http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_summary
Verified fixed on FX 49.0a1 (2016-05-22) Win 7, Ubuntu 14.04, OS X 10.11.

Comment 41

[Marco Zehe (:MarcoZ)]

With bug 634004 now fixed, accessibility is good to go for enabling <details> and <summary>.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

I think that the note on MDN is enough. Please ni me if you disagree.