Bug 1226455 (ship-details-summary)

ship support for <details> and <summary>

VERIFIED FIXED in Firefox 49

Status

()

Core
Layout
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: dbaron, Assigned: TYLin)

Tracking

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

Trunk
mozilla49
dev-doc-complete, feature
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox45 affected, firefox49 verified, relnote-firefox -)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
TYLin, do you know of other bugs that should be added?
Flags: needinfo?(tlin)
(Assignee)

Comment 2

2 years ago
dbaron, you've found all the bugs I know of. Thanks.
Flags: needinfo?(tlin)
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Depends on: 1241750
(Assignee)

Updated

2 years ago
Blocks: 1245021
(Assignee)

Updated

2 years ago
Depends on: 1245424
(Reporter)

Updated

2 years ago
Alias: ship-details-summary
Duplicate of this bug: 1245583
(Assignee)

Updated

2 years ago
Depends on: 1246185
(Assignee)

Updated

2 years ago
Depends on: 1249556

Updated

2 years ago
No longer depends on: 634004

Updated

2 years ago
No longer depends on: 1245424
Release Note Request: See bug 591737 comment 157
relnote-firefox: --- → ?
Keywords: feature
(Assignee)

Updated

a year ago
Depends on: 1258657
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.
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.
Flags: needinfo?(florin.mezei)
(Assignee)

Comment 7

a year ago
The "intent to ship" is discussed here.
https://groups.google.com/forum/#!msg/mozilla.dev.platform/FCQSOydoE-8/YeAp3VZIAgAJ
Assignee: nobody → tlin
Status: NEW → ASSIGNED
(Assignee)

Comment 8

a year ago
Created attachment 8742184 [details]
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/
Attachment #8742184 - Flags: review?(bzbarsky)
(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
Flags: needinfo?(florin.mezei) → qe-verify+
What's the conclusion on the marker thing?
Flags: needinfo?(tlin)
(Assignee)

Comment 11

a year ago
(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.
No longer depends on: 1221416
Flags: needinfo?(tlin)
(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
OK, can you clearly explain the marker situation in the intent to ship thread, please?
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.
Attachment #8742184 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

a year ago
(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.
Flags: needinfo?(dbaron)
(Reporter)

Comment 16

a year ago
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)?
Flags: needinfo?(tlin)
(Assignee)

Comment 17

a year ago
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
Flags: needinfo?(tlin)
(Reporter)

Comment 18

a year ago
Created attachment 8747986 [details]
some tests of marker styling
(Reporter)

Comment 19

a year ago
Created attachment 8747989 [details]
some tests of marker styling
Attachment #8747986 - Attachment is obsolete: true
(Reporter)

Comment 20

a year ago
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...)
Flags: needinfo?(dbaron) → needinfo?(tlin)
(Assignee)

Comment 21

a year ago
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?
Flags: needinfo?(tlin) → needinfo?(dbaron)
https://bugs.webkit.org/

Comment 23

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3152d79aa593
(Reporter)

Comment 24

a year ago
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.
Flags: needinfo?(dbaron) → needinfo?(tlin)
(Reporter)

Updated

a year ago
Flags: needinfo?(dbaron)

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3152d79aa593
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Reporter)

Comment 26

a year ago
Filed https://bugs.webkit.org/show_bug.cgi?id=157323
Flags: needinfo?(dbaron)
(Assignee)

Comment 27

a year ago
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.
Flags: needinfo?(tlin)
Attachment #8742184 - Flags: approval-mozilla-aurora?
(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.
(Reporter)

Comment 29

a year ago
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).
Yes, I think we can just get some sanity checks as MattN suggested in comment #6.
Adding a display:block in the <summary> makes the expand arrow disappear. In Chrome, it doesn't.
Thoughts? http://codepen.io/anon/pen/eZbMja
Flags: needinfo?(tlin)
(Reporter)

Comment 32

a year ago
(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.
Flags: needinfo?(tlin)
Making sure accessibility API support is on the radar for this. This cannot ship without it.
Depends on: 634004
As this bug is fixed now, should the related preference be removed? And should I file a new bug for that?

Sebastian
Flags: needinfo?(dbaron)
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 36

a year ago
I though I had file a bug for removing the pref, but found nothing. Filed bug 1271549.
Flags: needinfo?(dbaron)
(Reporter)

Comment 37

a year ago
(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.)
Flags: needinfo?(tlin)
(Assignee)

Comment 38

a year ago
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.
Attachment #8742184 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 39

a year ago
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.
Flags: needinfo?(tlin)
(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.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
With bug 634004 now fixed, accessibility is good to go for enabling <details> and <summary>.
I think that the note on MDN is enough. Please ni me if you disagree.
relnote-firefox: ? → -

Updated

a year ago
Blocks: 1299753

Updated

a year ago
No longer blocks: 1299753
Depends on: 1299753
You need to log in before you can comment on or make changes to this bug.