Closed
Bug 1226455
(ship-details-summary)
Opened 8 years ago
Closed 7 years ago
ship support for <details> and <summary>
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla49
People
(Reporter: dbaron, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, feature)
Attachments
(2 files, 1 obsolete file)
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•8 years ago
|
||
TYLin, do you know of other bugs that should be added?
Assignee | ||
Comment 2•8 years ago
|
||
dbaron, you've found all the bugs I know of. Thanks.
Flags: needinfo?(tlin)
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•8 years ago
|
Depends on: enable-details-summary-nightly-aurora
Assignee | ||
Updated•7 years ago
|
Blocks: details-summary
Reporter | ||
Updated•7 years ago
|
Alias: ship-details-summary
Comment 4•7 years ago
|
||
Release Note Request: See bug 591737 comment 157
relnote-firefox:
--- → ?
Keywords: feature
Comment 5•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
(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+
Assignee | ||
Comment 11•7 years 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)
Comment 12•7 years ago
|
||
(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•7 years ago
|
||
OK, can you clearly explain the marker situation in the intent to ship thread, please?
![]() |
||
Comment 14•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Reporter | ||
Comment 19•7 years ago
|
||
Attachment #8747986 -
Attachment is obsolete: true
Reporter | ||
Comment 20•7 years 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•7 years 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)
![]() |
||
Comment 22•7 years ago
|
||
https://bugs.webkit.org/
Reporter | ||
Comment 24•7 years 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•7 years ago
|
Flags: needinfo?(dbaron)
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3152d79aa593
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Comment 26•7 years ago
|
||
Filed https://bugs.webkit.org/show_bug.cgi?id=157323
Flags: needinfo?(dbaron)
Assignee | ||
Comment 27•7 years 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?
Comment 28•7 years ago
|
||
(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•7 years 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).
Comment 30•7 years ago
|
||
Yes, I think we can just get some sanity checks as MattN suggested in comment #6.
Comment 31•7 years ago
|
||
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•7 years 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)
Comment 33•7 years ago
|
||
Making sure accessibility API support is on the radar for this. This cannot ship without it.
Depends on: 634004
Comment 34•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
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•7 years ago
|
||
I though I had file a bug for removing the pref, but found nothing. Filed bug 1271549.
Flags: needinfo?(dbaron)
Reporter | ||
Comment 37•7 years 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•7 years 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•7 years 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)
Comment 40•7 years ago
|
||
(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
Comment 41•7 years ago
|
||
With bug 634004 now fixed, accessibility is good to go for enabling <details> and <summary>.
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•