Closed Bug 1226455 (ship-details-summary) Opened 9 years ago Closed 8 years ago

ship support for <details> and <summary>

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox49 --- verified
relnote-firefox --- -

People

(Reporter: dbaron, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature)

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1226455 - Enable <details> and <summary> on all channels.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
some tests of marker styling
3.19 KB, text/html
Details 
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.
TYLin, do you know of other bugs that should be added?
Depends on: 591737, 634004, 1225752, 1221416, 1225412
Flags: needinfo?(tlin)
dbaron, you've found all the bugs I know of. Thanks.
Flags: needinfo?(tlin)
Depends on: 1245424
Alias: ship-details-summary
Depends on: 1246185
Depends on: 1249556
No longer depends on: 634004
No longer depends on: 1245424
Release Note Request: See bug 591737 comment 157
relnote-firefox: --- → ?
Keywords: feature
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)
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
(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)
(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+
(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)
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)
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)
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)
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)
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)
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/3152d79aa593
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
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)
(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)
I though I had file a bug for removing the pref, but found nothing. Filed bug 1271549.
Flags: needinfo?(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.)
Flags: needinfo?(tlin)
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?
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: ? → -
Blocks: 1299753
No longer blocks: 1299753
Depends on: 1299753
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: