Closed Bug 591737 Opened 14 years ago Closed 8 years ago

Support for HTML5's <details> and <summary>

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
relnote-firefox --- 49+

People

(Reporter: teoli, Assigned: TYLin)

References

(Blocks 5 open bugs, )

Details

(Keywords: dev-doc-complete, html5, Whiteboard: [parity-webkit][parity-blink][pref=dom.details_element.enabled])

Attachments

(20 files, 9 obsolete files)

719 bytes, text/html
Details
2.12 KB, text/html
Details
96.60 KB, patch
Details | Diff | Splinter Review
71 bytes, text/plain
Details
40 bytes, text/x-review-board-request
mrbkap
: review+
Details
40 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
smaug
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
surkov
: review+
Details
40 bytes, text/x-review-board-request
Ms2ger
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b5pre) Gecko/20100827 Firefox/4.0b5pre Minefield/4.0b5pre
Build Identifier: 

A lot of sites needs to hide part of the web page and make them hide/unhide when we click on a button or a link.

HTML5 propose two new elements to allows this behaviour: <details> and <summary>.

These two elements not only conveys semantic information about the relation between a detail and its summary, but allows to show/hide the details without the need of (trivial) javascript.

For instance, things like the following could be replaced to drop JS:
<a href='#' onclick='toggleVisibility(0); return false' title='See détails (need JavaScript)'>
(This example line is used on wikipedia).


Reproducible: Always




Coupled with transition, this may also remove the need for some use of Flash (photo galleries?)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: html
Keywords: html5
In my tests using the "Try it yourself" link at http://www.w3schools.com/html5/tag_details.asp, the <details> tag contents remain visible by default using Beta 8.  According to both that page and http://www.w3.org/TR/html5/interactive-elements.html#the-details-element the contents of <details> are supposed to be invisible unless the "open" attribute is specified.  This clearly isn't how Beta 8 is behaving.  Is this tag implemented yet?  Personally, the tag seems redundant, but I don't do the specs.
(In reply to comment #2)
No, this is not supported yet. :) If it were supported, this bug would be fixed.

At this moment in time, no layout engine is going to support the details element, because it is still an unstable part of the spec. It could change at any time. It could even be completely removed from the spec soon, because there is an issue as to whether or not the element is actually needed.
@josh not anymore.

The problem has been settled you can start implementing it.
@Comment 4: Could you please cite a WHATWG/W3C discussion/resolution, a browser implementation, or other info supporting that claim? While I think that these elements should be implemented, I haven't seen anything to indicate that the status of these elements has changed.
The only question I have concerning the implementation is the scrollintoview expected/default behaviour.
The rest is really straightforward.
FWIW, Google Chrome 12.0.706.0 canary build now has a details/summary disclosure widget.
Not sure how spec-compliant it is at this point, though.
E.g., summary is block instead of inline, no 40px margin-start.
Opera has started a thread on the WhatWG mailing list about the styling of <details>. The discussion may be useful: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-April/031132.html
Someone could add [parity-webkit] on the whiteboard?
https://bugs.webkit.org/show_bug.cgi?id=50309
Whiteboard: [parity-webkit]
Version: unspecified → Trunk
Is this hard to fix? If not, could it be a [good first bug]?
(In reply to Paul Rouget [:paul] from comment #12)
> Is this hard to fix?

This is not trivial.

> If not, could it be a [good first bug]?

FWIW, [good first bugs] have been deprecated by mentored bugs.
Blocks: 783301
Blocks: 767405
<details><summary> in Firefox/SeaMonkey/KompoZer would be very useful for me -- re-formatting large web-based manuals (e.g. Linux manuals) as active outlines, like those in the sub-directories of  http://www.rodericksmith.plus.com/outlines , (until now, they used JavaScript).  

I find them so much easier to browse for information than PDF, word, man pages or standard long web pages.

I'll be adding a limb of the directory tree with those outlines in pure HTML5 quite soon -- the pure HTML5 versions work with Google Chrome and with standard Android tablet browsers.  

With <details><summary>  creating the outlines will be easier too (once I update my outline reformatting tools).
I've begun looking at this.  A few questions:

1. Do we want the disclosure widget to look appropriate for different platforms, or
   should we just draw a triangle on all platforms?  And if so, should we draw it
   with the colour of the <summary> element, as WebKit does?

2. Should just the disclosure widget be clickable, or the whole <summary> element's
   contents?

3. In WebKit, a big focus ring is displayed around the <summary>, and the element is
   focusable.  Should we be doing the same?  I don't really like the look of the
   big focus ring, but I think it's useful if you can tab to the element by default.
Assignee: nobody → cam
Status: NEW → ASSIGNED
I lean strongly toward a platform-appropriate look for the widget, because (and I'd love to see metrics on this) people switch apps more often than they switch platforms. It's true of me, anyway.
Disclosure widget: 
------------------

For outlining use, I'd prefer the option to provide one's own open/closed folder image, because that relates to older windows users' expectations of file browsers.

Additional capabilities  -- suggestions for further comment
-----------------------

onclick, onhover
----------------

Those options would reduce the JavaScript demands on more complex implementations (presenting partial details on hover, or monitoring what the end-user viewed and for how long)

Summary below details
---------------------

In making outlines, the caption of a figure or illustration (that you'd want to click for the full representation) should usually appear below the 'details' of the full figure. That's currently not possible in the Google implementation (and not mentioned in the HTML5 specification); irrespective of the placement of code in the details section, it appears below the summary display.

Alternate 'alt='
---------------

This would be useful e.g. for showing thumbprints that are replaced by full-sized images on clicking.


Now if we had <details><summary> implemented, think how much neater this comment would look!

Progress with implementation
----------------------------

I'm pleased to see some-one's assigned to the task now.  I (and possibly others of the commentators) would be willing beta-testers, and would appreciate news of progress as it happens!
(In reply to Rod Smith from comment #17)
> Disclosure widget: 
> ------------------
> 
> For outlining use, I'd prefer the option to provide one's own open/closed
> folder image, because that relates to older windows users' expectations of
> file browsers.

This sounds like a nice idea, but would need some spec work done.  There are many different ways that the disclosure triangle bit of the element is exposed to CSS.

> Additional capabilities  -- suggestions for further comment
> -----------------------
> 
> onclick, onhover
> ----------------
> 
> Those options would reduce the JavaScript demands on more complex
> implementations (presenting partial details on hover, or monitoring what the
> end-user viewed and for how long)

click, mouseover, etc. are are dispatched to all elements.  Or do you mean specifically to the disclosure triangle, and being able to detect that separately from the rest of the element?

> Summary below details
> ---------------------
> 
> In making outlines, the caption of a figure or illustration (that you'd want
> to click for the full representation) should usually appear below the
> 'details' of the full figure. That's currently not possible in the Google
> implementation (and not mentioned in the HTML5 specification); irrespective
> of the placement of code in the details section, it appears below the
> summary display.

Maybe; this wouldn't be hard to implement.  It could make interaction with the element difficult, though, as the <summary> bit and its disclosure triangle would move up and down in the page when you open or closer it.

> Alternate 'alt='
> ---------------
> 
> This would be useful e.g. for showing thumbprints that are replaced by
> full-sized images on clicking.

I don't really understand this one.

> Progress with implementation
> ----------------------------
> 
> I'm pleased to see some-one's assigned to the task now.  I (and possibly
> others of the commentators) would be willing beta-testers, and would
> appreciate news of progress as it happens!

I'll post try builds once I have something functional.
Thanks for the links, Steve.(In reply to steve faulkner from comment #18)
> The HTML to Platform Accessibility APIs Implementation Guide provides
> recommendations on keyboard support and accessibility API mappings for the
> details/summary:

Thanks for those links.
Attached patch WIP (v0.1) (obsolete) — Splinter Review
This pretty much works.  Outstanding issues:
  * the accessibility mapping hasn't been done yet
  * a plain triangle is used as the disclosure widget on all platforms
  * clicking the triangle or <summary> contents can result in text selection,
    which looks a bit odd
  * the triangle is implemented with UA style rules on the <summary> making it
    display:list-item and with new -moz-disclosure-{open,closed} values for
    list-style-type, which is probably not the right solution
  * I haven't implemented anything in nsCSSFrameConstructor::CreateContinuingFrame
    for <details>, so it probably doesn't handle :first-letter properly
(In reply to Cameron McCormack (:heycam) from comment #21)
> This pretty much works.  Outstanding issues:

Oh, and no tests written yet. :)
There seems to be a bug in the handling of nested <details>. Try this document:

https://gist.github.com/4316774

Click the triangle for the top-level details elements, then click on the triangle for the first nested details element.

What happens in my environment at least is, that details element you clicked on will then unexpectedly disappear (instead of expanding as expected to show its summary content).

Then click on the next two nested details and they'll disappear as well.

But if you click on the top-level triangle again, to unexpand it, and then click again to re-expand it, they'll all be back.

Also some nits: The disclosure triangle seems too big, and I wonder if it should be gray instead of black. At least the convention for disclosure triangles I've seen across most UIs is to make the gray.
There is an issue with webkit implementation related to validation of required fields in details element https://twitter.com/getsetbro/status/280348413827104768

there is an example: http://jsfiddle.net/getsetbro/wtK4s/
Attached patch WIP (v0.2) (obsolete) — Splinter Review
(In reply to Michael[tm] Smith from comment #24)
> There seems to be a bug in the handling of nested <details>. Try this
> document:

Thanks, that should be fixed now.

> Also some nits: The disclosure triangle seems too big, and I wonder if it
> should be gray instead of black. At least the convention for disclosure
> triangles I've seen across most UIs is to make the gray.

I've made it a bit smaller (I was miscalculating the size before).  If we do platform specific rendering, then I'll make it grey (on Mac at least) then.


New builds:

http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-c071c7b6a382/try-linux/firefox-20.0a1.en-US.linux-i686.tar.bz2
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-c071c7b6a382/try-macosx64/firefox-20.0a1.en-US.mac.dmg
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-c071c7b6a382/try-win32/firefox-20.0a1.en-US.win32.zip
Attachment #692862 - Attachment is obsolete: true
(In reply to steve faulkner from comment #25)
> There is an issue with webkit implementation related to validation of
> required fields in details element
> https://twitter.com/getsetbro/status/280348413827104768
> 
> there is an example: http://jsfiddle.net/getsetbro/wtK4s/

Whether the <details> should automatically open to show the required field is an interesting question.  With my patch, you still get the "You need to fill out this field" popup, but it isn't positioned correctly -- it's placed outside the whole browser window.

You've got the same issue with any display:none form field that requires validation, by the way: http://jsfiddle.net/VsdHf/  At least with <details>, it seems reasonable to open them up to show the field that is failing validation.  For arbitrarily display:none styled form elements, we can't just show them.
(In reply to Cameron McCormack (:heycam) from comment #26)
> (In reply to Michael[tm] Smith from comment #24)
> > There seems to be a bug in the handling of nested <details>. Try this
> > document:
> 
> Thanks, that should be fixed now.

Thanks, yeah, working as expected now.

> > Also some nits: The disclosure triangle seems too big, and I wonder if it
> > should be gray instead of black. At least the convention for disclosure
> > triangles I've seen across most UIs is to make the gray.
> 
> I've made it a bit smaller (I was miscalculating the size before). 

Yeah, looks better now.

> If we do
> platform specific rendering, then I'll make it grey (on Mac at least) then.

OK

One other thing: Mathias Byens has a details+summary demo page at http://mathiasbynens.be/demo/html5-details-jquery It implements a fallback mechanism for browsers that don't have native support rendering those elements yet. It correctly detects that the Gecko build I'm using does have native support, and works as expected -- except that it doesn't display the disclosure triangles. But if I disable his CSS stylesheet, the triangles are displayed as expected. It could be that there's something WebKit-specific in his CSS (I haven't checked yet but I will), but if not that, maybe something that your implementation doesn't expect and doesn't handle right. I guess it's more likely that his CSS is borked, since he couldn't test it with Gecko back when he created that page.
(In reply to Michael[tm] Smith from comment #28)
> One other thing: Mathias Byens has a details+summary demo page at
> http://mathiasbynens.be/demo/html5-details-jquery ...
> ... it doesn't display the
> disclosure triangles.

The cause seems to be just that he has an explicit "details, summary { display: block }" in his stylesheet. Clearly it's not needed for browsers that have native support, but it seems like it at least should not prevent the triangle from being displayed in the case where some document does specify it for whatever reason.
Yeah, that's because I have

  details > summary:first-of-type { display: list-item }

in the UA style sheet, and using the list bullet to render the disclosure triangle.  I'll resurrect the thread mentioned in comment 9 to discuss how exactly it should be exposed to CSS.
(In reply to Cameron McCormack (:heycam) from comment #15)
> 1. Do we want the disclosure widget to look appropriate for different
> platforms, or
>    should we just draw a triangle on all platforms?  And if so, should we
> draw it
>    with the colour of the <summary> element, as WebKit does?

We probably should draw the platform disclosure triangle. That would mean we'd want a pseudo-element where we can set -moz-appearance:XYZ.

We can draw animated themed widgets, so we might want to consider animating this too, although probably not in the first patch.

> 2. Should just the disclosure widget be clickable, or the whole <summary>
> element's
>    contents?

All the contents, although preventDefault() on the mouse event should stop that.

> 3. In WebKit, a big focus ring is displayed around the <summary>, and the
> element is
>    focusable.  Should we be doing the same?  I don't really like the look of
> the
>    big focus ring, but I think it's useful if you can tab to the element by
> default.

For buttons, we draw the focus ring if you tabbed into it with the keyboard, but not if you click with the mouse. You probably want to do the same thing here.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> We probably should draw the platform disclosure triangle. That would mean
> we'd want a pseudo-element where we can set -moz-appearance:XYZ.

What should we use for this frame?  Currently it's an nsBulletFrame that nsBlockFrame creates just due to the normal display:list-item behaviour.  I'd like to re-use the layout that gets done for nsBulletFrames -- is it OK to have it be an actual nsBulletFrame, if I can get that to paint properly when it has a -moz-appearance applied to it?

And should we use display:list-item as the actual way for the disclosure widget to be created?  It's really the only way (through CSS) to opt in to a marker frame being created.

If this is OK, then we could have this in the UA style sheet:

  details > summary:first-of-type {
    display: list-item;
  }

  details > summary:first-of-type::-moz-list-bullet {
    -moz-appearance: treetwisty; /* or something new if that's inappropriate */
    list-style-type: -moz-disclosure-closed;
  }

  details[open] > summary:first-of-type::-moz-list-bullet {
    list-style-type: -moz-disclosure-open;
  }

and then if the authors uses -moz-appearance:none they'll still get a plain looking triangle that they could then override.  They would need to know that it's a list-style-type for overriding, though.

(Eventually this should be ::marker rather than ::-moz-list-bullet I guess.)

> We can draw animated themed widgets, so we might want to consider animating
> this too, although probably not in the first patch.
> 
> > 2. Should just the disclosure widget be clickable, or the whole <summary>
> > element's
> >    contents?
> 
> All the contents, although preventDefault() on the mouse event should stop
> that.

I've done this, so it should be working.

Should we be someone stopping text selection from these mouse events too?  Double clicking on a <summary> results in its text being selected in addition to the details opening and then closing.  Clicking the disclosure widget also selects the contents of the <summary>, just like when you click a regular bullet.  What would be the proper way to inhibit this?


> > 3. In WebKit, a big focus ring is displayed around the <summary>, and the
> > element is
> >    focusable.  Should we be doing the same?  I don't really like the look of
> > the
> >    big focus ring, but I think it's useful if you can tab to the element by
> > default.
> 
> For buttons, we draw the focus ring if you tabbed into it with the keyboard,
> but not if you click with the mouse. You probably want to do the same thing
> here.

OK, that should be working too.  We get a dotted outline focus ring when focused rather than a big honking blue ring, which is good.
(In reply to Cameron McCormack (:heycam) from comment #32)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> > We probably should draw the platform disclosure triangle. That would mean
> > we'd want a pseudo-element where we can set -moz-appearance:XYZ.
> 
> What should we use for this frame?  Currently it's an nsBulletFrame that
> nsBlockFrame creates just due to the normal display:list-item behaviour. 
> I'd like to re-use the layout that gets done for nsBulletFrames -- is it OK
> to have it be an actual nsBulletFrame, if I can get that to paint properly
> when it has a -moz-appearance applied to it?
> 
> And should we use display:list-item as the actual way for the disclosure
> widget to be created?  It's really the only way (through CSS) to opt in to a
> marker frame being created.
> 
> If this is OK, then we could have this in the UA style sheet:
> 
>   details > summary:first-of-type {
>     display: list-item;
>   }
> 
>   details > summary:first-of-type::-moz-list-bullet {
>     -moz-appearance: treetwisty; /* or something new if that's inappropriate
> */
>     list-style-type: -moz-disclosure-closed;
>   }
> 
>   details[open] > summary:first-of-type::-moz-list-bullet {
>     list-style-type: -moz-disclosure-open;
>   }

Please don't. Make it a block and not a list item. It also does not seem to be a list item. I don't know the limitations the native ff js & CSS has but I think there should be a way of doing it without using the display as list-item. Besides, the display should be consistent. By default, all <summary> should display the same way.
A playground for <details><summary>
-----------------------------------

I've started a playground program to evaluate different aspects of <details><summary, that allows you to edit in wysiwyg and save the result to local disk.  There are just a few issues already outlined in a <details><summary> document there.

There are a few extra considerations in contentEditable mode, but mostly it's very very useful as it is.

Could any-one spare time to give it a try?  Follow the link to 

http://www.rodericksmith.plus.com/outlines/detailsSummary/

Feed-back welcomed, and information on whether the issues are bugs or my inadequate implementation!
I have two key problems with copy/paste of contenteditable disclosure items, 
----------------------------------------------------------------------------
that web authors will experience.

-- can they be solved by the implementation?


Problem 1: Nested hidden item-trees are silently corrupted by copy/paste operations, 
  leading to unexpected and unseen corruption of the author's edits. (unnested is OK)
  
Problem 2: selection without encapsulating the disclosure (e.g. in a table element)
  is imprecise, leading to corruption e.g. by inserting a style tag between 
  details> and <summary while using a wysiwyg editor to style components  Could it
  show extent markers like 'table' does on selection?

They violate the 'principle of least surprise' and could generate negative comments. 

Is there some way around these issues, or can they be solved by modifying the implementation?
  (Could sizing handles appear on selection, like for the table element for problem 2?)

You can exercise Problem 1 by setting <body> to contenteditable and 
 copy-pasting the widget in the HTML code that follows:

--------------------------

<html ><head><meta charset=utf-8><title>DS issue: nested copy-paste</title></head>
<body  contenteditable="true" >

prior text0
<details><summary>summary0</summary>details0

prior text1
<details><summary>summary1</summary>details1</details>
following text1

</details>
following text0
</body></html>

--------------------------
More explanation under 
http://www.rodericksmith.plus.com/outlines/detailsSummary/issues/
(In reply to Rod Smith from comment #35)
> Problem 1: Nested hidden item-trees are silently corrupted by copy/paste
> operations, 
>   leading to unexpected and unseen corruption of the author's edits.
> (unnested is OK)

I am not terribly familiar with the behaviour that the editor should have with regards to new elements like this.  I'll look into it.

> Problem 2: selection without encapsulating the disclosure (e.g. in a table
> element)
>   is imprecise, leading to corruption e.g. by inserting a style tag between 
>   details> and <summary while using a wysiwyg editor to style components 
> Could it
>   show extent markers like 'table' does on selection?

I don't understand this, sorry.  Do you have a problem selecting content within the <details> or its <summary>?
Attached patch WIP (v0.3) (obsolete) — Splinter Review
Another WIP patch.  Now we have native rendering of the disclosure widget on Mac, Windows and Linux.  (On Mac and Linux it's the same as a treetwisty, but on Windows it uses a more specific "expando" button.)  This is done with a -moz-appearance: disclosure-{closed,open} on the bullet frame for the <summary> element (which still is required to remain display:list-item for the moment).

On other platforms, and if you turn the appearance off with

  summary::-moz-list-bullet { -moz-appearance: none; }

you get the hand draw triangles from the previous patch.

Interaction is a little better -- clicking on the disclosure widget now does not select the <summary>'s contents.  However, the state of the widget (hovered, active) is the same as the <summary> element itself.  So if you hover or click on the contents of the <summary>, the widget will change appearance.  This is fine for Windows, at least, where this is the expected behaviour.  But it would be nice to only respond to clicks on the triangle on Mac, where this is the native behaviour you'd expect.  It's a bit tricky while I'm still using a bullet frame and don't have any content for the widget that can have separate state from the <summary> itself.

New builds:

Windows: http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-7b7ab8532c80/try-win32/firefox-20.0a1.en-US.win32.zip
Linux: http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-7b7ab8532c80/try-linux/firefox-20.0a1.en-US.linux-i686.tar.bz2
Mac: http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/cmccormack@mozilla.com-7b7ab8532c80/try-macosx64/firefox-20.0a1.en-US.mac.dmg
Attachment #693226 - Attachment is obsolete: true
Attached file test
Here's a small example of customising the appearance of the disclosure widget.
By the way, the native disclosure widgets are fixed size (on Windows they scale horribly, on Mac the triangle doesn't grow, just the padding around it does).  So it makes for slightly unexpected behaviour when you zoom the page in and out.
(In reply to Cameron McCormack (:heycam) from comment #36)
> (In reply to Rod Smith from comment #35)
> > Problem 1: Nested hidden item-trees are silently corrupted by copy/paste
> > operations, 
> >   leading to unexpected and unseen corruption of the author's edits.
> > (unnested is OK)
> 
> I am not terribly familiar with the behaviour that the editor should have
> with regards to new elements like this.  I'll look into it.
> 

------------------------------------

When copy/pasting nested disclosure items, it is much better, from a web page editor's
standpoint, to preserve the nested disclosure items, their state and and their contents,
even when they are 'hidden'.  Otherwise, the whole 'nest' has to be re-typed.

I hope you may be able to make that happen . . .  thanks!

-------------------------------------


> > Problem 2: selection without encapsulating the disclosure (e.g. in a table
> > element)
> >   is imprecise, leading to corruption e.g. by inserting a style tag between 
> >   details> and <summary while using a wysiwyg editor to style components 
> > Could it
> >   show extent markers like 'table' does on selection?
> 
> I don't understand this, sorry.  Do you have a problem selecting content
> within the <details> or its <summary>?

----------------------------------

I can select content OK, but the boundary of the selected region sometimes does and other
times doesn't include the <details> or <summary> tag.  For example, if the details text is
selected in an editor add-on, and the 'make bold' button is pressed, the '<b>' tag sometimes
appears between the summary> and <details> tags in the middle of the disclosure item, and
other times (where it ought to be) after the <details> tag at the start of the details text.

The former causes some corruption of the disclosure item.  

It can be worked-around by enclosing the summary text content and (separately) the details 
text content in a table cell element -- the selection then snaps to the right place.  So it
would be nice to have a fix, but not essential.

I'll put up an example tomorrow (I'm out today).

Many thanks for investigating . . .
------------------------------------
(In reply to Cameron McCormack (:heycam) from comment #39)
> By the way, the native disclosure widgets are fixed size (on Windows they
> scale horribly, on Mac the triangle doesn't grow, just the padding around it
> does).  So it makes for slightly unexpected behaviour when you zoom the page
> in and out.

What do you mean by fixed size? They have a fixed width/height even if the author asked for something else? I feel like we usually don't do that. At least, for form elements, on Mac, we have the same problem (there are usually 2 or 3 sizes for each form control on MacOS) but instead of having a fixed size, we scale to the desired size. We should definitely use a default rendering (non-native) when the size would be too ugly on the platform, like a very huge button, but we don't do that yet.

IMO, we should make sure the default size renders well and whether scale or fallback to a default rendering for other sizes.
(In reply to Mounir Lamouri (:mounir) from comment #41)
> What do you mean by fixed size? They have a fixed width/height even if the
> author asked for something else?

Yes, and also the fixed size is not relative to the font size.  Still for the author to request a different size, they would have to write:

  summary::-moz-list-bullet { width: 64px; height: 64px; }

for example.

> I feel like we usually don't do that. At
> least, for form elements, on Mac, we have the same problem (there are
> usually 2 or 3 sizes for each form control on MacOS) but instead of having a
> fixed size, we scale to the desired size. We should definitely use a default
> rendering (non-native) when the size would be too ugly on the platform, like
> a very huge button, but we don't do that yet.
> 
> IMO, we should make sure the default size renders well and whether scale or
> fallback to a default rendering for other sizes.

On Mac, I don't think I've ever seen a disclosure triangle rendered larger than the default size.  It might look OK if we scale before rendering it, but I wonder whether we should be doing that given that it would never appear larger than its usual size in native applications.  At least for Windows it seems to be painting a bitmap, so it's not going to look good at any size other than its usual one.

I guess the question then is how to decide when to fall back to the non-themed rendering.  For example, should this:

  <details>
    <summary style="font-size: 48px">summary</summary>
    details
  </details>

?  The native disclosure widget will look small next to the text.  We could decide for example that if the height of the text is greater than twice the height of the disclosure widget, then we switch to the non-themed rendering.
(In reply to Rod Smith from comment #40)
> (In reply to Cameron McCormack (:heycam) from comment #36)
> > (In reply to Rod Smith from comment #35)

. . . . .



As promised, I've posted an 'active' example of the selection issue with an explanation at:

http://www.rodericksmith.plus.com/outlines/detailsSummary/issues/disclosureSelectionIssue.html
Thanks for the explanation, Rod.  It looks like the editor needs to be taught that summary (and details too I imagine) are block elements and so inline formatting elements shouldn't go around them.
Uhm… How can I add this patch?
(In reply to sjw from comment #45)
> Uhm… How can I add this patch?

If you are compiling your own Firefox builds, then you can apply the patch with:

  $ patch -p1 < thepatchfile

at the command line.  If not, you can try the builds from comment 37 to experiment with the <details> support so far.
(In reply to Cameron McCormack (:heycam) from comment #46)
> (In reply to sjw from comment #45)
> > Uhm… How can I add this patch?
> 
> If you are compiling your own Firefox builds, then you can apply the patch
> with:
> 
>   $ patch -p1 < thepatchfile
> 
> at the command line.  If not, you can try the builds from comment 37 to
> experiment with the <details> support so far.

Thanks!
Attachment #698429 - Attachment mime type: text/plain → text/html
(In reply to Cameron McCormack (:heycam) from comment #42)
> > I feel like we usually don't do that. At
> > least, for form elements, on Mac, we have the same problem (there are
> > usually 2 or 3 sizes for each form control on MacOS) but instead of having a
> > fixed size, we scale to the desired size. We should definitely use a default
> > rendering (non-native) when the size would be too ugly on the platform, like
> > a very huge button, but we don't do that yet.
> > 
> > IMO, we should make sure the default size renders well and whether scale or
> > fallback to a default rendering for other sizes.
> 
> On Mac, I don't think I've ever seen a disclosure triangle rendered larger
> than the default size.  It might look OK if we scale before rendering it,
> but I wonder whether we should be doing that given that it would never
> appear larger than its usual size in native applications.

It is exactly what happens for <progress>, <meter>, <input> and <button>: they all have at least one dimension restricted on MacOS X but we allow them to be rendered at any size. The output is sometimes ugly but we should fix that by falling back to the non-native rendering.

> At least for
> Windows it seems to be painting a bitmap, so it's not going to look good at
> any size other than its usual one.

Then maybe we should fallback to the non-native rendering as soon as we try to paint it to the non-default size?

> I guess the question then is how to decide when to fall back to the
> non-themed rendering.  For example, should this:
> 
>   <details>
>     <summary style="font-size: 48px">summary</summary>
>     details
>   </details>
> 
> ?  The native disclosure widget will look small next to the text.  We could
> decide for example that if the height of the text is greater than twice the
> height of the disclosure widget, then we switch to the non-themed rendering.

This rule or any other. We should prevent text with huge fonts to have a very small disclosure triangle because we only support one size.
(In reply to sjw from comment #48)
> Created attachment 698429 [details]
> Possible bugs in the <details> and <summary> patch v0.3

Thanks for the comemnts, sjw.

For #1, you are right, the special styling should not be applied to the <summary> within the <buggy> element.

For #2 and #3 (which are the same?), I think it has the right behaviour.  An empty value is a valid way to specify a boolean attribute like open="".

For #4, something funny is going on there with frame construction...

For #5, I don't think that is incorrect behaviour, or anything specific to details/summary.  For example you could get the same word wrapping with deeply nested <ul><li> elements: http://mcc.id.au/temp/ul.html
>
> For #2 and #3 (which are the same?), I think it has the right behaviour.  An
> empty value is a valid way to specify a boolean attribute like open="".
> 
Thanks for your feedback.

#2 & #3: I don't know why, but there was missed something...

#2 the attribut shoud be the following: open=" ". My english is verry bad, but I read the W3C working draft and I think this means, that if the value is empty it schould be hidden.

"When the element is created, if the attribute is absent, the details should be hidden; if the attribute is absent, the details should be shown. Subsequently, if the attribute is removed, then the details should be hidden; if the attribute is added, the details should be shown."
http://www.w3.org/TR/2011/WD-html5-20110525/interactive-elements.html#the-details-element

#3 the attribut shoud be the following: open="buggy". This is a invalid value, so the details should be hidden.
Attachment #698429 - Attachment is obsolete: true
Attachment #699162 - Attachment mime type: text/plain → text/html
minor bug fixing
Attachment #699162 - Attachment is obsolete: true
Attachment #699167 - Attachment mime type: text/plain → text/html
Added two more bugs ("tabindex-bug" & "editable-bug").
Attachment #699167 - Attachment is obsolete: true
Attachment #699259 - Attachment mime type: text/plain → text/html
(In reply to sjw from comment #52)
> #2 & #3: I don't know why, but there was missed something...
> 
> #2 the attribut shoud be the following: open=" ". My english is verry bad,
> but I read the W3C working draft and I think this means, that if the value
> is empty it schould be hidden.

No, if the attribute is present -- regardless of whether it has a non-empty value -- then the <details> element should be open.  This is the way boolean attributes in HTML work:

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#boolean-attributes
Attachment #699259 - Attachment is obsolete: true
Attachment #734368 - Attachment mime type: text/plain → text/html
(In reply to Cameron McCormack (:heycam) from comment #57)
> No, if the attribute is present -- regardless of whether it has a non-empty
> value -- then the <details> element should be open.  This is the way boolean
> attributes in HTML work:
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/common-
> microsyntaxes.html#boolean-attributes


Okay, sorry about that. I hope the other bugs are a real help.
Any news on this one?
I should be getting back to it soon; other work has priority at the moment.
Does anyone know when the <details><summary> capabilities in the snapshots above can be incorporated into the mainstream Firefox releases (so I can provide my outlined manuals for anyone to use)?

(Having to use a development snapshot for a long period causes me problems because some updated plug-ins are no longer compatible)

Thanks
Other work has still got in front of this work, sorry.  Current thing ahead of this bug for me are bug 773296 and bug 731271.
Attached patch WIP (v0.5)Splinter Review
Rebased.
Attachment #698866 - Attachment is obsolete: true
FYI, in case you haven't noticed it yet: http://html5.org/r/8251

Hixie: aja, heycam|away: let me know if you have feedback on that, i didn't have much to go on despite asking around :-)
Hixie: just that several people wanted it
Hixie: and that there was no good way to do it before
Hixie: (short of mutation observers)
I'm relieved to see that this has been actively developed and isn't one of those "keep the ball in the air" type feature requests where a developer is always working on something "a little more important" etc. A few thoughts:

1. The issue with form validation -- is this an issue with native form validation or custom validators? If it's a custom validator, I think the burden of expanding the relevant summary elements should be on the developer of the custom code, since they will know which non-visible fields require visibility and since the additional code to do this should be trivial (basically "for all invalid input ancestors of type summary with attribute "open" set to false, set to true).  However if it's native validation (such as an input of type "email" with invalid data or an empty input with "required" attribute) would it make sense to trigger the appropriate event (I think it would be 'click' for the "options" element) on the non-valid input and set the event to bubble? If you think of the "click" event (or "touch", or equivalent input-device-neutral event) as "poke", it would make sense that each invalid form input would get poked on submit and that if it bubbled up, this would mean any nested "summary" elements would open, as well as any custom interactive scripting.

The only downside to the bubbling idea is that the "invalid poke" event + open attribute value would need to be logically OR so that an already expanded summary element would not close.

Second:

Again, I am incredibly impressed with the amount of thought that has gone into this feature. Much more than I would have initially though (form data being corrupted, nested summaries, block vs list item, etc). But at the risk of sounding pushy, this feature needs to go out the front door ASAP. Reasons for increased priority:

http://www.w3.org/TR/html5/ -- The following features are at risk and may be removed due to lack of implementation. ( summary and details, high on the list).

When this bug was submitted 2 years ago, no layout engines had implemented this part of the spec. Now, there are only two  :

http://html5test.com/compare/feature/elements-interactive-details.html
http://caniuse.com/#feat=details

History has proven time and again that if IE drags its heels but the other "warriors" have adopted a drafted feature, the feature will survive to final recommendation, but if IE + another major browser lack the feature, it will circle the drain with only techno-political chances of survival.  If this feature isn't rolled out in a public release of Gecko soon, bumping the browser adoption percentage over 70 and getting true adoption confidence started, not only will this very useful and lightweight feature wither before it had a chance in the wild, the 2 years of effort above will have been for nothing.

I, for one, would really like to see this feature stick around, for no other reason than it's already in place conceptually on so many sites (wikipedia's mobile site being a big example) so it clearly fills a need and is not just  a neat idea. (Side note, I will make similar passionate pleas for features that are at this point just a neat idea as well: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23589 )
Blocks: 966107
Please don't add non-constructive "me too" comments, they will not influence the speed of development at all (at least not positively). Only comment on this bug if you have patches for implementation or tests that add value.
No longer blocks: 966107
Depends on: 982355
Whiteboard: [parity-webkit] → [parity-webkit][parity-blink]
<details> and <summary> have been adopted as part of Drupal 8, so I do really hope that this gets brought into FF.  Florian Bender, I do appreciate the "me too" concerns, but it's a significant piece of the Internet at the moment.
Keywords: dev-doc-needed
One thing that Chrome doesn't do and might be worth considering is when an expanding <details> box is rendered beyond the current scroll position, no scrolling occurs to bring the newly rendered content into view.  At the moment, this means I'm overriding Chrome's behaviour altogether in order to give the user the best experience.

Having to scroll manually is more a minor annoyance than anything else, but it would be useful if the bottom of the box is aligned with the bottom of the viewport (in the case of vertical scrolling), but only if the box is smaller than the vertical height of the document.
(In reply to Andy Earnshaw from comment #75)
> One thing that Chrome doesn't do and might be worth considering is when an
> expanding <details> box is rendered beyond the current scroll position, no
> scrolling occurs to bring the newly rendered content into view.  At the
> moment, this means I'm overriding Chrome's behaviour altogether in order to
> give the user the best experience.
> 
> Having to scroll manually is more a minor annoyance than anything else, but
> it would be useful if the bottom of the box is aligned with the bottom of
> the viewport (in the case of vertical scrolling), but only if the box is
> smaller than the vertical height of the document.

This is well outside the scope of this ticket. The user can already perform that scrolling using JS, and there may be cases where the developer does not intend for the browser to scroll.
I agree with Matt. I often find myself wanting to expand a consecutive subset of members in a list, and in Chrome, this is easily done by repeatedly pushing the adjacent arrows starting at the *bottom* of the subset so that the adjacent arrows don't disappear or move under my finger/mouse cursor. Auto-scrolling on every expand would severely hamper this use-case.
(In reply to Matt Basta [:basta] from comment #76)
> This is well outside the scope of this ticket. 
I don't know if I'd agree that it's "well outside".  Borderline, perhaps, but it seems like something that would make sense to add at the time of implementation rather than waiting until it's finished and then asking someone to go back and make the changes then.  Changing behaviour after implementation might cause confusion among developers and users alike.

> The user can already perform that scrolling using JS
Can you elaborate on this?  Do you mean user of the HTML elements or end user of the browser?  I often take "user" to mean the latter, in which case the majority of users are unable to write their own JS and not everyone has JS enabled for every site.

> and there may be cases where the developer does not intend for the browser to scroll.
For which there is .addEventListener('click', ...) and .preventDefault().

(In reply to Jan-Ivar Bruaroey [:jib] from comment #77)
> I agree with Matt. I often find myself wanting to expand a consecutive
> subset of members in a list, and in Chrome, this is easily done by
> repeatedly pushing the adjacent arrows starting at the *bottom* of the
> subset so that the adjacent arrows don't disappear or move under my
> finger/mouse cursor. Auto-scrolling on every expand would severely hamper
> this use-case.
I understand, but surely the majority use case in this situation would be expanding a <details> block to read immediately?  Shouldn't we be aiming to address the majority use case?

Perhaps a modifier key while clicking could address the use case where scrolling isn't desired.
(In reply to Andy Earnshaw from comment #78)
> Changing behaviour after implementation might cause confusion among developers and users alike.

The same argument can be used against implementing behavior different from Chrome here.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #79)
> (In reply to Andy Earnshaw from comment #78)
> > Changing behaviour after implementation might cause confusion among developers and users alike.
> 
> The same argument can be used against implementing behavior different from
> Chrome here.
I don't think vendor implementation and developer implementation is broad enough right now for many end users to even realise that WebKit/Blink's behaviour is built in.  In my case I need to also scroll the element into view, so I'm overriding WebKit's behaviour with the same code I'm using to polyfill other browsers (since it requires more code to detect and the implementation is fairly simple anyway).

Regardless, it was merely a suggestion and I've made my point.  I could argue usability all day, but it's not like this is a major breaking issue.
(In reply to Alice Wonder from comment #82)
> There are two places where I regularly use this tag:
> 
> A) On a 'works cited' page, I will have the author as the summary and the
> full bibtex style reference in the rest of the details. I have JS that
> automatically open the node if the page was access by a link with a #
> pointing to specific work.
> 
> Having the works cited in a details node makes it easy for someone to scan
> for the work they are looking for and open that node.
> 
> The other place I use it - in articles where I have article / section
> structure, my php auto-generates a table of contents that goes two sections
> deep.
> 
> That ToC is put into a nav div as a child of the details node, and the
> summary tag says "Table of Contents" - the details is inserted under the h1
> header at beginning of article.
> 
> Most people visiting the page probably don't need the ToC - but if a visitor
> is looking for something specific they know is in the page, the ToC helps
> them find it.
> 
> This works beautifully in both Chrome and Midori. It would be nice if it
> worked in FireFox too. I've been waiting YEARS for it to work in FireFox, I
> was an early adopter of the tag.
> 
> I wrote JavaScript to emulate it for browsers w/o support, but the
> JavaScript broke with a jQuery update. Rather than fix it, I just removed
> it. I shouldn't have to use JavaScript to provide what HTML5 browsers should
> already do.

I know we're supposed to limit chatter on bug comments, but Alice's last comment made me think of a specific aspect of details element I was curious about. It may be easy to find in spec, but more interesting to ask here. Is browser's 'Find' feature expected to expand results found in folded/collapsed detail elements, so that in Alice's use case of bibliographic citations, if a user searches for 'Princeton', would results be visible if in the full citation that's collapsed? And if yes, should it expand only when the user uses the Find 'next/back' option (and the 'next' result is inside collapsed section) and then the details folds back up when they move on to results outside of that element? Or should all folded elements expand based on Find results and stay expanded while Find is active? Even further, in any of these scenarios, should any/all details that were expanded due to Find go back to pre-Find state when Find is closed?

On the one hand, it may be a bit sloppy if searching for 'smith' results in all details being expanded and requires page reload (or manually collapsing each) to get them out of the way. On the other hand, if I spot my match (yay!) and then close Find, expecting to search further or do some follow up task, I'd be pretty annoyed when the results ran away from me, mocking me.
> Is browser's 'Find' feature expected to expand results found in folded/collapsed 
> detail elements, so that in Alice's use case of bibliographic citations, if a 
> user searches for 'Princeton', would  results be visible if in the full citation 
> that's collapsed?

That's out of the scope of this issue. A better place to ask this might be the mozilla.dev.tech.layout discussion group[1].

Sebastian

[1] https://groups.google.com/forum/#!forum/mozilla.dev.tech.layout
(In reply to wig_2006 from comment #86)
> I have HTML5's <details> and <summary> as same , because have HTML5's
> <details> and <summary> since Google Chrome 12.

Add technicalvalue

http://s29.postimg.org/vwtmpiy5h/Screenshot_from_2015_04_30_20_56_43.png 
- Firefox HTML5 test to not support HTML5's <details> and <summary> (technicalvalue from html5test.com).

http://s29.postimg.org/wauyp4i91/Screenshot_from_2015_04_30_20_58_03.png
http://s29.postimg.org/xr6h79l5x/Screenshot_from_2015_04_30_20_58_16.png
- Compare HTML5's <details> and <summary> between Firefox and Chrome (technicalvalue from caniuse.com).
(In reply to wig_2006 from comment #87)
> (In reply to wig_2006 from comment #86)
> > I have HTML5's <details> and <summary> as same , because have HTML5's
> > <details> and <summary> since Google Chrome 12.
> 
> Add technicalvalue

The tag 'notechnicalvalue' means that the comment didn't add information to implement this feature. Your second comment also just tells that Firefox doesn't have this implemented yet, which is clear, because otherwise this bug would already be closed. :-)
Also that Chrome already has this implemented is tracked by the whiteboard info [parity-blink].

Sebastian
The digital publishing industry is looking at <details><summary> as a vehicle to embed image descriptions which would include the ability to reference an external document with an embedded iFrame to allow for crowd sourcing of image descriptions or provisions for alternative content such as special plugins for charts. One of the proposals has been for an aria-describedat feature that points to another URL but a details/summary option tied to figure and figcaption would be better as other users besides assistive technologies would benefit. 

This would be used in EPUB authoring tools.
Cameron, do you still have plans to work on this? If not, maybe reset the assignee? Thanks
Flags: needinfo?(cam)
No, but I spoke to TYLin the other week who said he might be interested in finishing it off?
Assignee: cam → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(cam) → needinfo?(tlin)
Yes. I'm interested in bring your patch to the finish line.
Assignee: nobody → tlin
Flags: needinfo?(tlin)
Attached file WIP branch on GitHub
My WIP branch on Github. It is not ready to be tested yet.
Blocks: 1225412
Blocks: 1225752
Drupal 8 Core is launching tomorrow. It comes with summary/details built-in. People are obviously already building sites with Drupal 8, but there will be a lot more in the coming months. 

Would be great to improve the support in this great browser.
Bug 591737 - Teach parser about <details> and <summary>
Attachment #8689422 - Flags: review?(mrbkap)
Bug 591737 - Add details and summary to nsHTMLEditUtils
Attachment #8689423 - Flags: review?(masayuki)
Bug 591737 - Add HTMLDetailsElement and webidl interface
Attachment #8689424 - Flags: review?(bzbarsky)
Attachment #8689424 - Flags: review?(bugs)
Bug 591737 - Add HTMLSummaryElement
Attachment #8689425 - Flags: review?(bzbarsky)
Bug 591737 - Make nsIFrame::PrincipalChildList a const function

Since nsIFrame::GetChildList() is a const function, PrincipalChildList()
should be a const function as well.
Attachment #8689426 - Flags: review?(bzbarsky)
Bug 591737 - Add DetailsFrame
Attachment #8689427 - Flags: review?(bzbarsky)
Bug 591737 - Construct details and summary in nsCSSFrameConstructor

For now, we use "display: list-item" on main summary element to draw the
the disclosure widget. We might need a way to customize the disclosure
widget later.
Attachment #8689429 - Flags: review?(bzbarsky)
Bug 591737 - Provide a default summary element by DetailsFrame

If a <details> lacks a direct <summary> child, we need to construct a
default one.
Attachment #8689430 - Flags: review?(bzbarsky)
Bug 591737 - Add reftests for details and summary
Attachment #8689431 - Flags: review?(dbaron)
Bug 591737 - Add crashtest for details and summary
Attachment #8689432 - Flags: review?(dbaron)
Bug 591737 - Implement toggling open details by mouse click
Attachment #8689434 - Flags: review?(bzbarsky)
Bug 591737 - Avoid dispatch mouse double click to content not in doc

When double-clicking on a default anonymous <summary> element, the first
eMouseClick will make the summary element remove from the document. This
generates an assert in PresShell::HandleEventWithTarget().

To prevent this assert, ensure the mouseContent is in document before
dispatching the eMouseDoubleClick event.

Since GetCrossShadowCurrentDoc() was deprecated, replace it with
GetComposedDoc().
Attachment #8689435 - Flags: review?(bzbarsky)
Comment on attachment 8689422 [details]
MozReview Request: Bug 591737 - Teach parser about <details> and <summary>

Bug 591737 - Teach parser about <details> and <summary>
Comment on attachment 8689423 [details]
MozReview Request: Bug 591737 - Add details and summary to nsHTMLEditUtils

Bug 591737 - Add details and summary to nsHTMLEditUtils
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

Bug 591737 - Add HTMLDetailsElement and webidl interface
Comment on attachment 8689425 [details]
MozReview Request: Bug 591737 - Add HTMLSummaryElement

Bug 591737 - Add HTMLSummaryElement
Comment on attachment 8689426 [details]
MozReview Request: Bug 591737 - Add SummaryFrame

Bug 591737 - Make nsIFrame::PrincipalChildList a const function

Since nsIFrame::GetChildList() is a const function, PrincipalChildList()
should be a const function as well.
Comment on attachment 8689427 [details]
MozReview Request: Bug 591737 - Add DetailsFrame

Bug 591737 - Add DetailsFrame
Comment on attachment 8689428 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

Bug 591737 - Add SummaryFrame
Comment on attachment 8689429 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

Bug 591737 - Construct details and summary in nsCSSFrameConstructor

For now, we use "display: list-item" on main summary element to draw the
the disclosure widget. We might need a way to customize the disclosure
widget later.
Comment on attachment 8689430 [details]
MozReview Request: Bug 591737 - Provide a default summary element by DetailsFrame

Bug 591737 - Provide a default summary element by DetailsFrame

If a <details> lacks a direct <summary> child, we need to construct a
default one.
Comment on attachment 8689431 [details]
MozReview Request: Bug 591737 - Add reftests for details and summary

Bug 591737 - Add reftests for details and summary
Comment on attachment 8689432 [details]
MozReview Request: Bug 591737 - Add crashtest for details and summary

Bug 591737 - Add crashtest for details and summary
Comment on attachment 8689434 [details]
MozReview Request: Bug 591737 - Implement toggling open details by mouse click

Bug 591737 - Implement toggling open details by mouse click
Comment on attachment 8689435 [details]
MozReview Request: Bug 591737 - Avoid dispatch mouse double click to content not in doc

Bug 591737 - Avoid dispatch mouse double click to content not in doc

When double-clicking on a default anonymous <summary> element, the first
eMouseClick will make the summary element remove from the document. This
generates an assert in PresShell::HandleEventWithTarget().

To prevent this assert, ensure the mouseContent is in document before
dispatching the eMouseDoubleClick event.

Since GetCrossShadowCurrentDoc() was deprecated, replace it with
GetComposedDoc().
Bug 591737 - Add reftest for mouse click on summary
Attachment #8689437 - Flags: review?(dbaron)
Bug 591737 - Fix test_HTMLSpec.html
Attachment #8689438 - Flags: review?(surkov.alexander)
Bug 591737 - Mark tests in web-platform-test pass

Now we have <details> and <summary> tags implemented. Remove those lines
marked as "expected: FAIL" to let the tests pass.
Attachment #8689439 - Flags: review?(Ms2ger)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #97)
> Created attachment 8689423 [details]
> MozReview Request: Bug 591737 - Add details and summary to nsHTMLEditUtils
> 
> Bug 591737 - Add details and summary to nsHTMLEditUtils

I wonder, why did you request this review to me? I know there is no people working on editor as an owner nor a peer, so, if you don't find better people to review this, I'll try, though.
Flags: needinfo?(tlin)
https://reviewboard.mozilla.org/r/25615/#review23043

::: dom/events/EventStateManager.cpp:4658
(Diff revision 1)
>          ret = presShell->HandleEventWithTarget(&event2, currentTarget,

HandleEventWithTarget() is called here.
Is there a test build available?
Re comment 125: 
Perhaps Ehsan still review editor code. I'll flag him for the review.
Flags: needinfo?(tlin)
Attachment #8689423 - Flags: review?(masayuki)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #128)
> Re comment 125: 
> Perhaps Ehsan still review editor code. I'll flag him for the review.

If he is busy, feel free to ask me again.
Attachment #8689439 - Flags: review?(Ms2ger) → review+
Comment on attachment 8689439 [details]
MozReview Request: Bug 591737 - Mark tests in web-platform-test pass

https://reviewboard.mozilla.org/r/25621/#review23053

This commit should be squashed into the commits that actually fix the tests.
Comment on attachment 8689438 [details]
MozReview Request: Bug 591737 - Fix test_HTMLSpec.html

https://reviewboard.mozilla.org/r/25619/#review23055

r=me but I would prefer to have something like this. If it works. If it doesn't then we need a bug on it.

var obj = {
  role: ROLE_GROUPING,
  absentStates: STATE_EXPANDED,
  children: [
    { role: ROLE_LABEL }
  ]
};
testElm("details", obj);

obj = {
  role: ROLE_GROUPING,
  states: STATE_EXPANDED,
  children: [
    { role: ROLE_LABEL },
    { role: ROLE_ENTRY }
  ]
};
testElm("details_open", obj);
 
<details id="details">
  <summary><label for="name">Name:</label></summary>
  <input type="text" id="name" name="name" />
</details>

 <details id="details_open" open>
  <summary><label for="name">Name:</label></summary>
  <input type="text" id="name" name="name" />
</details>
Attachment #8689438 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #133)
> Comment on attachment 8689438 [details]
> MozReview Request: Bug 591737 - Fix test_HTMLSpec.html
> 
> https://reviewboard.mozilla.org/r/25619/#review23055
> 
> r=me but I would prefer to have something like this. If it works. If it
> doesn't then we need a bug on it.

we do have bug 634004 for this
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

(my review isn't needed here, unless bz wants to move some of the reviews to me.)
Attachment #8689424 - Flags: review?(bugs)
Attachment #8689424 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

https://reviewboard.mozilla.org/r/25595/#review23067

::: dom/html/HTMLDetailsElement.h:18
(Diff revision 1)
> +  using NodeInfo = mozilla::dom::NodeInfo;

Why do you need that?  You're already inside namespace mozilla::dom.

::: dom/html/HTMLDetailsElement.h:20
(Diff revision 1)
> +  explicit HTMLDetailsElement(already_AddRefed<NodeInfo>& aNodeInfo);

I think we've generally tried to make constructors like this inline for elements.

::: dom/html/HTMLDetailsElement.h:29
(Diff revision 1)
> +  void SetOpen(bool aOpen) { SetBoolAttr(nsGkAtoms::open, aOpen); }

This needs to be SetHTMLBoolAttr(nsGkAtoms:open, aOpen, aRv).  And you need to mark this attribute as [SetterThrows] in the IDL.

::: dom/html/nsGenericHTMLElement.h:1761
(Diff revision 1)
> +NS_DECLARE_NS_NEW_HTML_ELEMENT(Details)

Should come before the Div entry.

r=me with the above fixed.
Comment on attachment 8689425 [details]
MozReview Request: Bug 591737 - Add HTMLSummaryElement

https://reviewboard.mozilla.org/r/25597/#review23069

r=me

::: dom/html/HTMLSummaryElement.h:18
(Diff revision 1)
> +  using NodeInfo = mozilla::dom::NodeInfo;

Again, don't need that.

::: dom/html/HTMLSummaryElement.h:20
(Diff revision 1)
> +  explicit HTMLSummaryElement(already_AddRefed<NodeInfo>& aNodeInfo);

Inline.
Attachment #8689425 - Flags: review?(bzbarsky) → review+
> Why do you need that?  You're already inside namespace mozilla::dom.

Ah, you need it because bareword NodeInfo resolves to nsINode::NodeInfo, right.  <sigh>.  Ignore that comment, for both files.
Attachment #8689422 - Flags: review?(mrbkap) → review?(hsivonen)
Attachment #8689426 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689422 [details]
MozReview Request: Bug 591737 - Teach parser about <details> and <summary>

er, sorry, you were right the first time, although I don't know why we'd need to modify the old parser
Attachment #8689422 - Flags: review?(hsivonen) → review?(mrbkap)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #140)
> Comment on attachment 8689422 [details]
> MozReview Request: Bug 591737 - Teach parser about <details> and <summary>
> 
> er, sorry, you were right the first time, although I don't know why we'd
> need to modify the old parser

nsHTMLTagList.h is still used for generating the factory machinery for making created element nodes have the right C++ class.

We should probably remove most of parser/htmlparser/nsHTMLTags.cpp, though.

(I'd r+ this.)
Comment on attachment 8689428 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

https://reviewboard.mozilla.org/r/25603/#review23085

r=me, I think.  I'm pretty rusty on frame classes...
Attachment #8689428 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689427 [details]
MozReview Request: Bug 591737 - Add DetailsFrame

https://reviewboard.mozilla.org/r/25601/#review23075

::: layout/generic/DetailsFrame.h:15
(Diff revision 1)
> +// See nsCSSFrameConstructor::ConstructDetailsFrame for the structure of an

s/an/a/

::: layout/generic/DetailsFrame.cpp:82
(Diff revision 1)
> +    PresContext()->PresShell()->RecreateFramesFor(mContent);

Why are you handling this via AttributeChanged on the frame instead of just having your element implement GetAttributeChangeHint appropriately?  The latter would be far preferable, since it will coalesce much better with other style changes.

r=me with the above fixed, pending what I see in the frame constructor patch.
Attachment #8689427 - Flags: review?(bzbarsky) → review+
Attachment #8689429 - Flags: review?(bzbarsky)
Comment on attachment 8689429 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

https://reviewboard.mozilla.org/r/25605/#review23087

This is a great start, but there are some issues.  No surprise; this part is complicated.  ;)

What should happen if the <details> is styled to be taken out of line (floated, absolutely positioned, fixed positioned)?  From a quick glance at Chrome's behavior and your code, the behavior you're going to end up with is quite different from Chrome's, right?

::: layout/base/nsCSSFrameConstructor.h:387
(Diff revision 1)
> +                                 FrameConstructionItemList& aItems);

Document what this argument is supposed to mean.  But see more on this below....

::: layout/base/nsCSSFrameConstructor.cpp:3236
(Diff revision 1)
> +nsCSSFrameConstructor::ConstructDetailsFrame(nsFrameConstructorState& aState,

I don't see how this handles values of "overflow" other than "visible" on <details>.

::: layout/base/nsCSSFrameConstructor.cpp:3245
(Diff revision 1)
> +  nsIContent* details = aItem.mContent;
> +  RefPtr<nsStyleContext> styleContext = aItem.mStyleContext;

Really best to make these const pointers as elsewhere (const pointers, not pointers to const, note).

::: layout/base/nsCSSFrameConstructor.cpp:3281
(Diff revision 1)
> +      if (f && f->GetType() == nsGkAtoms::summaryFrame) {

I expect you will want f->GetContentInsertionFrame()->GetType() here once you support overflow on <summary>.

::: layout/base/nsCSSFrameConstructor.cpp:3282
(Diff revision 1)
> +        // Take out the first summary frame and put it as a child of the

Have you tested what happens here with floats inside the <summary>?  I _think_ it should work because <summary> is forced to a block-like display so they get parented to it, not to the anonymous detailsContentFrame, but please make sure you have tests.

Also, I assume you have tests for display:inline styling on <details> and the like, right?  If you don't, please add them.

Have you tested what happens with:

  <details style="height: 200px">
    <summary style="height: 100%">Stuff</summary>
  </details>

as well as:

  <details style="height: 200px">
    <div style="height: 100%">Stuff</summary>
  </details>

?  I see nothing in the reftests testing this sort of thing, and while the spec here is useless I would expect that the 100% height nodes in those testcases would end up with a 200px height.

Have you tested how this affects z-ordering of absolutely positioned elements, where one is a child of the <summary> and one is a child of the <details> that comes before <summary> in the DOM (assuming neither <summary> nor <details> are absolute containing blocks)?  Does our behavior match Chrome there?

::: layout/base/nsCSSFrameConstructor.cpp:3532
(Diff revision 1)
> +    SIMPLE_TAG_CREATE(summary, NS_NewSummaryFrame)

How does this handle values of "overflow" other than "visible" on <summary>?  Please add tests.

::: layout/base/nsCSSFrameConstructor.cpp:5432
(Diff revision 1)
> +    return false;

Why is the right place to do this check here and not in AddFrameConstructionItemsInternal where other such checks are done?  In particular, why do we want to skip SetAsUndisplayedContent in this case?  I strongly suspect we do not in fact want that.

::: layout/base/nsCSSFrameConstructor.cpp:7675
(Diff revision 1)
> +    // and let it figures out where this summary element should be laid out.

"let it figure"

::: layout/base/nsCSSFrameConstructor.cpp:9170
(Diff revision 1)
> +    // When we remove a summary frame which is served as the summary content for

s/is served/serves/

::: layout/style/html.css:776
(Diff revision 1)
> +  display: list-item !important;

I think this is technically a spec violation, but so much of the "details binding" stuff is underdefined that maybe it's ok.

::: layout/style/html.css:797
(Diff revision 1)
> +  display: block !important;

You may want to inherit/override more things here.  Specifically at least inherit text-overflow and overflow-clip-box.  Possibly for ::-moz-details-content also need the "block-size: 100%" that ::-moz-fieldset-content has.  This part is not clear to me; in particular this depends on how "height: 100%" on kids should work when the <details> has padding and a summary.
Attachment #8689430 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689430 [details]
MozReview Request: Bug 591737 - Provide a default summary element by DetailsFrame

https://reviewboard.mozilla.org/r/25607/#review23093

::: layout/generic/DetailsFrame.cpp:103
(Diff revision 1)
> +  // element lacks any direct <summry> child.

s/summry/summary/

r=me
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

Actually, wait.  r-, because this is not firing the "toggle" events the spec calls for when the "open" attribute is added/removed.
Attachment #8689424 - Flags: review+ → review-
Comment on attachment 8689434 [details]
MozReview Request: Bug 591737 - Implement toggling open details by mouse click

https://reviewboard.mozilla.org/r/25613/#review23095

::: dom/html/HTMLDetailsElement.h:31
(Diff revision 1)
> +  void ToggleOpen() { SetOpen(!Open()); }

This will need to be updated once SetOpen() can throw.  Probably just suppress the exception.

::: dom/html/HTMLSummaryElement.cpp:73
(Diff revision 1)
> +  DetailsFrame* detailsFrame = do_QueryFrame(details->GetPrimaryFrame());

So... the spec is no help here, but what should happen when a display:none <details> has a mouse event (synthetic, clearly) fired at its first <summary>?  Should the "open" attribute of the <details> change?

What does Chrome do here?  Ideally we would match them and raise a spec issue to define this stuff...
Attachment #8689434 - Flags: review?(bzbarsky)
Comment on attachment 8689435 [details]
MozReview Request: Bug 591737 - Avoid dispatch mouse double click to content not in doc

I'm going to punt this one to smaug.

Olli, the context is that the click default action posts a reframe for the parent of the (anonymous) node we're clicking on.  Then presumably something flushes out style/layout before the doubleclick fires (probably the click event processing for the second click) and the anonymous node is destroyed...
Attachment #8689435 - Flags: review?(bzbarsky) → review?(bugs)
Oh, one more question.  Have you done any testing with pagination/columnation?  What happens if the <details> or its content insertion kid split across a page/column boundary?  Seems like if that happens the "get my content frame" code that gets the last child would be wrong...  but I also didn't see any implementation of continuation frame creation in the frame constructor code, so not sure how this will actually behave in practice.  Please test.
Flags: needinfo?(tlin)
Attachment #8689422 - Flags: review?(mrbkap) → review+
Comment on attachment 8689422 [details]
MozReview Request: Bug 591737 - Teach parser about <details> and <summary>

https://reviewboard.mozilla.org/r/25591/#review23119
Re comment 149: I haven't done any test with pagination/columnation yet. Will investigate these as well as other properties like overflow, etc. described in comment 144.

Re comment 146: I filed bug 1225412 for firing 'toggle' event for details. Is it OK to get the layout right in this bug and deal with the event later?
Flags: needinfo?(tlin) → needinfo?(bzbarsky)
Attachment #8689437 - Flags: review?(dbaron)
> Is it OK to get the layout right in this bug and deal with the event later?

Yes, as long as we make sure to not ship before the event bit is done.
Flags: needinfo?(bzbarsky)
Release Note Request (optional, but appreciated)
[Why is this notable]: The <details> is used as a disclosure widget from which the user
can obtain additional information or controls. <summary> is used as a summary or legend of the details. To expand the details, the user could click on the summary or by adding a bool attribute 'open' to the details.
[Suggested wording]: Support for details and summary HTML elements.
[Links (documentation, blog post, etc)]:
https://html.spec.whatwg.org/multipage/forms.html#the-details-element
https://groups.google.com/forum/#!topic/mozilla.dev.platform/b0UMDIasfq8
relnote-firefox: --- → ?
Comment on attachment 8689435 [details]
MozReview Request: Bug 591737 - Avoid dispatch mouse double click to content not in doc

-      if (NS_SUCCEEDED(ret) && aEvent->clickCount == 2) {
+      if (NS_SUCCEEDED(ret) && aEvent->clickCount == 2 &&
+          mouseContent->GetComposedDoc()) {

Use IsInComposedDoc()
Attachment #8689435 - Flags: review?(bugs) → review+
Regarding the release note request from comment 157, it should rather be requested for bug 1226455.

Sebastian
Comment on attachment 8689423 [details]
MozReview Request: Bug 591737 - Add details and summary to nsHTMLEditUtils

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25593/diff/1-2/
Attachment #8689423 - Flags: review?(ehsan)
Attachment #8689423 - Flags: review?(ehsan) → review+
Comment on attachment 8689423 [details]
MozReview Request: Bug 591737 - Add details and summary to nsHTMLEditUtils

https://reviewboard.mozilla.org/r/25593/#review23385
https://reviewboard.mozilla.org/r/25601/#review23075

> Why are you handling this via AttributeChanged on the frame instead of just having your element implement GetAttributeChangeHint appropriately?  The latter would be far preferable, since it will coalesce much better with other style changes.

I simply do not know I should implement HTMLDetailsElement::GetAttributeChangeHint(). I'll change it in next patch set.
Depends on: 1234744
https://reviewboard.mozilla.org/r/25605/#review23087

If the <details> is taken out of line, it's behavior should like a normal block div. The behavior of my implementation should align with Chrome's.

> Have you tested what happens here with floats inside the <summary>?  I _think_ it should work because <summary> is forced to a block-like display so they get parented to it, not to the anonymous detailsContentFrame, but please make sure you have tests.
> 
> Also, I assume you have tests for display:inline styling on <details> and the like, right?  If you don't, please add them.
> 
> Have you tested what happens with:
> 
>   <details style="height: 200px">
>     <summary style="height: 100%">Stuff</summary>
>   </details>
> 
> as well as:
> 
>   <details style="height: 200px">
>     <div style="height: 100%">Stuff</summary>
>   </details>
> 
> ?  I see nothing in the reftests testing this sort of thing, and while the spec here is useless I would expect that the 100% height nodes in those testcases would end up with a 200px height.
> 
> Have you tested how this affects z-ordering of absolutely positioned elements, where one is a child of the <summary> and one is a child of the <details> that comes before <summary> in the DOM (assuming neither <summary> nor <details> are absolute containing blocks)?  Does our behavior match Chrome there?

I add all test cases mentioned in patchset 2, including floats inside the <summary>, absolute height children, and z-order test.

Q: Have you tested how this affects z-ordering of absolutely positioned elements, where one is a child of the <summary> and one is a child of the <details> that comes before <summary> in the DOM (assuming neither <summary> nor <details> are absolute containing blocks)?  Does our behavior match Chrome there?
A: The child of <summary> will be under the the child of <details> assuming above condition. Our behavior matches both Chrome and Safari.

> I think this is technically a spec violation, but so much of the "details binding" stuff is underdefined that maybe it's ok.

Instead of setting summary element as list-item, I construct nsBulletFrame in SummaryFrame for the disclosure triangle in patchset 2.
https://reviewboard.mozilla.org/r/25605/#review23087

> Why is the right place to do this check here and not in AddFrameConstructionItemsInternal where other such checks are done?  In particular, why do we want to skip SetAsUndisplayedContent in this case?  I strongly suspect we do not in fact want that.

I move the check to AddFrameConstructionItemsInternal() and add undisplyed items by using SetAsUndisplayedContent() in patchset 2.

> You may want to inherit/override more things here.  Specifically at least inherit text-overflow and overflow-clip-box.  Possibly for ::-moz-details-content also need the "block-size: 100%" that ::-moz-fieldset-content has.  This part is not clear to me; in particular this depends on how "height: 100%" on kids should work when the <details> has padding and a summary.

I simplified the frame structure by removing the two anonymous block children from details frame in patchset 2. This is not needed.
https://reviewboard.mozilla.org/r/25613/#review23095

> So... the spec is no help here, but what should happen when a display:none <details> has a mouse event (synthetic, clearly) fired at its first <summary>?  Should the "open" attribute of the <details> change?
> 
> What does Chrome do here?  Ideally we would match them and raise a spec issue to define this stuff...

Both Chrome and Safari do not touch the 'open' attribute if the details has display: none. We'd better follow their behavior here. I've added a test case mouse-click-display-none-details.html. Also filed a spec issue. https://github.com/whatwg/html/issues/517
Attachment #8689429 - Flags: review?(bzbarsky)
Attachment #8689432 - Flags: review?(bzbarsky)
Attachment #8689434 - Flags: review?(bzbarsky)
Attachment #8709973 - Attachment description: MozReview Request: Bug 591737 - Add pref for details and summary elements → MozReview Request:
Attachment #8709973 - Attachment description: MozReview Request: → MozReview Request: Bug 591737 - Add pref for details and summary elements
Due to mozreview server error for my patchset 2, I push my patchset again. The "patchset 2" mentioned in comment 163 and comment 164 should now be refer to "patchset 3".
Olli, you had grant r+ for the patch 
"Bug 591737 - Avoid dispatch mouse double click to content not in doc" in comment 158, but the r+ does not recorded in mozreview. Would you please click "ship it" for me in mozreview again? Thanks.
Flags: needinfo?(bugs)
done. it is unfortunate that bugzilla<->mozreview integration doesn't quite work.
Flags: needinfo?(bugs)
Comment on attachment 8689435 [details]
MozReview Request: Bug 591737 - Avoid dispatch mouse double click to content not in doc

https://reviewboard.mozilla.org/r/25615/#review28359
Attachment #8689435 - Flags: review?(bugs) → review+
bz, this bug needs your review to move forward. Thanks.
Flags: needinfo?(bzbarsky)
Huh.  I thought I had commented on this one back when review was requested...

It's been a long time since I last looked at this stuff, and I need to page it back in.  Which means I need to find a multi-hour-long chunk of time to look at this effectively.

This is pretty high on my priority list, but scheduling around various meetings has been complicated.  I'm certainly not going to get to this today, but hoping to on Monday or Tuesday.  I'm sorry for the lag...
Flags: needinfo?(bzbarsky)
bz, thank you for consider this feature high priority. I take me a long time to address all the comments and add the tests, and I hope I'm on the right track :)
Attachment #8689424 - Flags: review?(bzbarsky)
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

https://reviewboard.mozilla.org/r/25595/#review29845

::: dom/webidl/HTMLDetailsElement.webidl:14
(Diff revisions 1 - 3)
>  interface HTMLDetailsElement : HTMLElement {

This needs to not be exposed until the toggle thing is fixed.  And you need to not have the parser creating this new class until then either.
Comment on attachment 8689427 [details]
MozReview Request: Bug 591737 - Add DetailsFrame

https://reviewboard.mozilla.org/r/25601/#review29849

::: layout/generic/DetailsFrame.cpp:46
(Diff revision 3)
> +DetailsFrame::SetInitialChildList(ChildListID aListID, nsFrameList& aChildList)

Why does this function exist?
Attachment #8689427 - Flags: review+
Comment on attachment 8689428 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

https://reviewboard.mozilla.org/r/25603/#review29851

::: layout/generic/SummaryFrame.cpp:35
(Diff revisions 1 - 3)
> +SummaryFrame::SetInitialChildList(ChildListID aListID, nsFrameList& aChildList)

Again, why does this function exist?
Attachment #8689428 - Flags: review+
https://reviewboard.mozilla.org/r/25605/#review23087

What in your code makes it behave like a normal block?  I don't see anything to do that.

Also, I might have meant <summary>, not <details>, above.  Try this testcase in Chrome:

<!DOCTYPE html>
<body>
  Some text here.
  <details>
    <summary style="position: absolute; top:0; right: 0; width: 100px; border:
                    1px solid green">aaa</summary>
    bbb
  </details>
</body>

and see whether that's the behavior your patch has (it's not) and then we can think about what behavior would actually make sense here.

> I add all test cases mentioned in patchset 2, including floats inside the <summary>, absolute height children, and z-order test.
> 
> Q: Have you tested how this affects z-ordering of absolutely positioned elements, where one is a child of the <summary> and one is a child of the <details> that comes before <summary> in the DOM (assuming neither <summary> nor <details> are absolute containing blocks)?  Does our behavior match Chrome there?
> A: The child of <summary> will be under the the child of <details> assuming above condition. Our behavior matches both Chrome and Safari.

OK.  That probably needs a spec issue raised, then.  Please do that.
Attachment #8689429 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689429 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

https://reviewboard.mozilla.org/r/25605/#review29859

::: dom/html/HTMLDetailsElement.cpp:24
(Diff revision 3)
> +  // XXX: Might want to cache the first summary element.

Might be a decent idea.  Please file a followup.

::: layout/base/nsCSSFrameConstructor.cpp:11508
(Diff revision 3)
> +                blockFrame->GetType() == nsGkAtoms::detailsFrame),

Can summaries get columns?  Followup probably OK for that.

::: layout/generic/DetailsFrame.cpp:48
(Diff revision 3)
> +  if (aListID == kPrincipalList) {

Ah, this is why you needed this function...

::: layout/generic/DetailsFrame.cpp:53
(Diff revision 3)
> +      // If details is open, the first summary need to be render as if it is the

"needs to be rendered"

r=me.  This is much better, thank you.
> What in your code makes it behave like a normal block?  I don't see anything to do that.

I guess the upshot is that absolutely positioning or floating these things will just make them act like blocks from the outside, but still use their special frame classes.  OK.
Comment on attachment 8689434 [details]
MozReview Request: Bug 591737 - Implement toggling open details by mouse click

https://reviewboard.mozilla.org/r/25613/#review29863

::: dom/html/HTMLDetailsElement.h:52
(Diff revision 3)
> +  }

You do need to rv.SuppressException() here.

::: dom/html/HTMLSummaryElement.cpp:55
(Diff revision 3)
> +  if (details->GetPrimaryFrame()) {

So I realized that my "clearly synthetic" comment is wrong, as I commented in the W3C issue.  Also of interest are cases in which a capturing event listener higher up the DOM sets the <details> or <summary> to display:none. Or moves the <summary> to a different <details> element, for that matter.  Please check what Chrome/Safari do in all three of those situations?
Attachment #8689434 - Flags: review?(bzbarsky)
Comment on attachment 8689437 [details]
MozReview Request: Bug 591737 - Add reftest for mouse click on summary

https://reviewboard.mozilla.org/r/25617/#review29865

::: layout/reftests/details-summary/fixed-summary.html:1
(Diff revision 3)
> +<!DOCTYPE html>

layout/reftests/details-summary/fixed-summary.html seems to not be used for anything?

::: layout/reftests/details-summary/float-details.html:1
(Diff revision 3)
> +<!DOCTYPE html>

layout/reftests/details-summary/float-details.html seems to not be used for anything?

::: layout/reftests/details-summary/mouse-click-fixed-summary.html:1
(Diff revision 3)
> +<!DOCTYPE html>

layout/reftests/details-summary/mouse-click-fixed-summary.html seems to not be used for anything?

::: layout/reftests/details-summary/mouse-click-fixed-summary.html:16
(Diff revision 3)
> +    summary.dispatchEvent(new MouseEvent("click"));

Spec editor says this should not toggle, of course (but I think spec editor is wrong).

In general, there's a bunch of files being added here that are not being added to the reftest.list, so this changeset is not making any sense to me.
Attachment #8689437 - Flags: review?(bzbarsky)
FWIW, I believe tests for <details> and <summary> should go to web-platform tests.
Comment on attachment 8709973 [details]
MozReview Request: Bug 591737 - Add pref for details and summary elements

https://reviewboard.mozilla.org/r/31615/#review29867

Ah, here's the disabling for now.  r=me and ignore my earlier comments about this being needed, since it's clearly done.  :)
Attachment #8709973 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689432 [details]
MozReview Request: Bug 591737 - Add crashtest for details and summary

https://reviewboard.mozilla.org/r/25611/#review29869

Probably a good idea to throw some columns and scrollability in here too, right?

r=me with those added.
Attachment #8689432 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

https://reviewboard.mozilla.org/r/25595/#review29871

r=me since the pref gets added later.
Attachment #8689424 - Flags: review+
Comment on attachment 8689427 [details]
MozReview Request: Bug 591737 - Add DetailsFrame

https://reviewboard.mozilla.org/r/25601/#review29873

r=me since this function is in fact used later.  A comment to that effect in this changeset would have been nice, or just not adding it until it was used... ;)
Attachment #8689427 - Flags: review+
Comment on attachment 8689428 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

https://reviewboard.mozilla.org/r/25603/#review29875
Attachment #8689428 - Flags: review+
Comment on attachment 8689431 [details]
MozReview Request: Bug 591737 - Add reftests for details and summary

https://reviewboard.mozilla.org/r/25609/#review29877

::: layout/reftests/details-summary/details-absolute-children.html:39
(Diff revision 3)
> +      <div id="before_summary">div before summary</div> <!-- This div is at front. -->

Please do make sure that a spec issue is filed for this.

::: layout/reftests/details-summary/details-in-ol.html:13
(Diff revision 3)
> +    <!-- Test <ol> numbering does not affect by <details>. -->

s/does not affect/is not affected/

::: layout/reftests/details-summary/details-page-break-before-2.html:8
(Diff revision 3)
> +      <summary>Summary<div style="page-break-after: always;"></div></summary>

This file is identical to details-page-break-after-2.html as far as I can tell.

::: layout/reftests/details-summary/details-three-columns.html:21
(Diff revision 3)
> +  div#columns {

Don't need this style here, right?

::: layout/reftests/details-summary/details-three-columns.html:40
(Diff revision 3)
> +    </div>

Should be </details>

::: layout/reftests/details-summary/details-two-pages.html:1
(Diff revision 3)
> +<!DOCTYPE html>

details-two-pages.html seems to be identical to details-page-break-after-1.html, no?

It would be better to use <div>s for the reference (and suppress the disclosure triangle in the relevant tests in the usual way).

It would also be good to have copies of these page-break tests with overflow:auto on the details and summary, but you should probably do that in a followup, because I suspect they don't do the magic we do for scrollable blocks in printing and adding it might be a bit complicated.

::: layout/reftests/details-summary/float-in-summary.html:30
(Diff revision 3)
> +      <summary><h4>This is float: right in summary</h4>Lorem ipsum dolor sit amet,

The reference has a line break before the <h4>.  The test should too.

::: layout/reftests/details-summary/overflow-auto-details.html:1
(Diff revision 3)
> +<!DOCTYPE html>

overflow-auto-details.html is not in reftest.list that I can see.

::: layout/reftests/details-summary/overflow-hidden-details.html:1
(Diff revision 3)
> +<!DOCTYPE html>

overflow-hidden-details.html isn't in reftest.list

Not marking r+ because of the files missing from reftest.list and the few other substantive issues above that I want to see the changes for.
Attachment #8689431 - Flags: review?(bzbarsky)
And my apologies again for the lag.  Hopefully the next round-trip here will be much quicker on both our parts!
Blocks: 1245032
Blocks: 1245036
https://reviewboard.mozilla.org/r/25605/#review29859

> Might be a decent idea.  Please file a followup.

Filed bug 1245032.

> Can summaries get columns?  Followup probably OK for that.

Currently, summaries do not support columns. Filed bug 1245036

> "needs to be rendered"

Will fix in next patch.
https://reviewboard.mozilla.org/r/25601/#review29873

Sorry for the confusion. I'll move DetailsFrame::SetInitialChildList() and SummaryFrame::SetInitialChildList() to "Bug 591737 - Construct details and summary in nsCSSFrameConstructor" in next patchset.
Blocks: 1245047
(In reply to Xidorn Quan [:xidorn] (UTC+10) (less responsive until Feb 22) from comment #224)
> FWIW, I believe tests for <details> and <summary> should go to web-platform
> tests.

xidorn, thanks for the suggestion. Filed bug 1245047.
After sleeping on this, it might also be worth testing how 

  <details style="display:flex">
    <summary></summary>
    <div></div>
  </details>

and

  <details style="display: table">
    <summary style="display: table-cell"></summary>
    <div style="display: table-cell"></div>
  </details>

work in your implementation and in other browsers.  Please file followup bugs and spec issues as needed based on that testing.
https://reviewboard.mozilla.org/r/25605/#review23087

> I guess the upshot is that absolutely positioning or floating these things will just make them act like blocks from the outside, but still use their special frame classes.  OK.

Your guess is correct. The absolute positioning or floating summary acts like block from outside, and can still be used to toggle details by mouse. Both Chrome and my implementation behave the same way. The frame tree dump of the example from comment 219 is here. https://pastebin.mozilla.org/8858331

> OK.  That probably needs a spec issue raised, then.  Please do that.

Filed a spec issue about the z-order in https://github.com/whatwg/html/issues/603
Blocks: 1245395
https://reviewboard.mozilla.org/r/25617/#review29865

> Spec editor says this should not toggle, of course (but I think spec editor is wrong).

I guess it's OK for now that the MouseEvent still toggle.

Somehow those reftests for 'fixed' and 'float' are missing from reftest.list during my patch rewriting ... I'll add them back. Thanks for pointing this out.
https://reviewboard.mozilla.org/r/25609/#review29877

> Please do make sure that a spec issue is filed for this.

Sure. Filed https://github.com/whatwg/html/issues/603

> This file is identical to details-page-break-after-2.html as far as I can tell.

Nice catch. It should be page-break-after: always.

> Don't need this style here, right?

You're right. Removed div#columns.

> details-two-pages.html seems to be identical to details-page-break-after-1.html, no?
> 
> It would be better to use <div>s for the reference (and suppress the disclosure triangle in the relevant tests in the usual way).
> 
> It would also be good to have copies of these page-break tests with overflow:auto on the details and summary, but you should probably do that in a followup, because I suspect they don't do the magic we do for scrollable blocks in printing and adding it might be a bit complicated.

You're right. Using <div>s for the refernce is better. Filed bug 1245395 for adding page-break tests with overflow:auto.

> The reference has a line break before the <h4>.  The test should too.

Fixed in next patch.

> overflow-auto-details.html is not in reftest.list that I can see.

It's used in "Bug 591737 - Add reftest for mouse click on summary". I'll move it.

> overflow-hidden-details.html isn't in reftest.list

It's used in "Bug 591737 - Add reftest for mouse click on summary". I'll move it.
Blocks: 1245424
https://reviewboard.mozilla.org/r/25613/#review29863

> You do need to rv.SuppressException() here.

Will do in next patchset.

> So I realized that my "clearly synthetic" comment is wrong, as I commented in the W3C issue.  Also of interest are cases in which a capturing event listener higher up the DOM sets the <details> or <summary> to display:none. Or moves the <summary> to a different <details> element, for that matter.  Please check what Chrome/Safari do in all three of those situations?

Curretly, display:none summmary crashes, so filed bug 1245424 for a follow-up. I guess we still need to be compatible with Chrome/Safari in these three cases.
Blocks: 1245430
Blocks: 1245431
(In reply to Boris Zbarsky [:bz] from comment #235)
> After sleeping on this, it might also be worth testing how 
> 
>   <details style="display:flex">
>     <summary></summary>
>     <div></div>
>   </details>
> 
> and
> 
>   <details style="display: table">
>     <summary style="display: table-cell"></summary>
>     <div style="display: table-cell"></div>
>   </details>
> 
> work in your implementation and in other browsers.  Please file followup
> bugs and spec issues as needed based on that testing.

Filed bug 1245430 and bug 1245431 for support display:flex and display:table, respectively.
Comment on attachment 8689422 [details]
MozReview Request: Bug 591737 - Teach parser about <details> and <summary>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25591/diff/3-4/
Comment on attachment 8689423 [details]
MozReview Request: Bug 591737 - Add details and summary to nsHTMLEditUtils

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25593/diff/3-4/
Comment on attachment 8689424 [details]
MozReview Request: Bug 591737 - Add HTMLDetailsElement and webidl interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25595/diff/3-4/
Comment on attachment 8689425 [details]
MozReview Request: Bug 591737 - Add HTMLSummaryElement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25597/diff/3-4/
Comment on attachment 8689427 [details]
MozReview Request: Bug 591737 - Add DetailsFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25601/diff/3-4/
Comment on attachment 8689426 [details]
MozReview Request: Bug 591737 - Add SummaryFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25599/diff/3-4/
Attachment #8689426 - Attachment description: MozReview Request: Bug 591737 - Make nsIFrame::PrincipalChildList a const function → MozReview Request: Bug 591737 - Add SummaryFrame
Attachment #8689428 - Attachment description: MozReview Request: Bug 591737 - Add SummaryFrame → MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor
Comment on attachment 8689428 [details]
MozReview Request: Bug 591737 - Construct details and summary in nsCSSFrameConstructor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25603/diff/3-4/
Comment on attachment 8689430 [details]
MozReview Request: Bug 591737 - Provide a default summary element by DetailsFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25607/diff/3-4/
Comment on attachment 8689431 [details]
MozReview Request: Bug 591737 - Add reftests for details and summary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25609/diff/3-4/
Attachment #8689431 - Flags: review?(bzbarsky)
Comment on attachment 8689432 [details]
MozReview Request: Bug 591737 - Add crashtest for details and summary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25611/diff/3-4/
Comment on attachment 8689434 [details]
MozReview Request: Bug 591737 - Implement toggling open details by mouse click

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25613/diff/3-4/
Attachment #8689434 - Flags: review?(bzbarsky)
Comment on attachment 8689435 [details]
MozReview Request: Bug 591737 - Avoid dispatch mouse double click to content not in doc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25615/diff/3-4/
Attachment #8689437 - Flags: review?(bzbarsky)
Comment on attachment 8689437 [details]
MozReview Request: Bug 591737 - Add reftest for mouse click on summary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25617/diff/3-4/
Comment on attachment 8689438 [details]
MozReview Request: Bug 591737 - Fix test_HTMLSpec.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25619/diff/3-4/
Comment on attachment 8689439 [details]
MozReview Request: Bug 591737 - Mark tests in web-platform-test pass

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25621/diff/3-4/
Comment on attachment 8709973 [details]
MozReview Request: Bug 591737 - Add pref for details and summary elements

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31615/diff/2-3/
Attachment #8689429 - Attachment is obsolete: true
Attachment #8689434 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689434 [details]
MozReview Request: Bug 591737 - Implement toggling open details by mouse click

https://reviewboard.mozilla.org/r/25613/#review30129
Comment on attachment 8689431 [details]
MozReview Request: Bug 591737 - Add reftests for details and summary

https://reviewboard.mozilla.org/r/25609/#review30131
Attachment #8689431 - Flags: review?(bzbarsky) → review+
Comment on attachment 8689437 [details]
MozReview Request: Bug 591737 - Add reftest for mouse click on summary

https://reviewboard.mozilla.org/r/25617/#review30135
Attachment #8689437 - Flags: review?(bzbarsky) → review+
Depends on: 1246404
Note that the support is not actually enabled by default yet (it's behind a pref) and it's not clear whether that will happen for 47.  How can we make MDN reflect that?
Flags: needinfo?(sebastianzartner)
(In reply to Boris Zbarsky [:bz] from comment #265)
> Note that the support is not actually enabled by default yet (it's behind a
> pref) and it's not clear whether that will happen for 47.  How can we make
> MDN reflect that?

I added a hint on the two pages that they are implemented behind a preference and I added now that those two elements are only available on Nightly releases.

Sebastian
Flags: needinfo?(sebastianzartner)
Thanks!
Ritu, in case you didn't notice the implementation is behind the dom.details_element.enabled pref. Bug 1226455 is for shipping without the pref.
Flags: needinfo?(rkothari)
Whiteboard: [parity-webkit][parity-blink] → [parity-webkit][parity-blink][pref=dom.details_element.enabled]
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #269)
> Ritu, in case you didn't notice the implementation is behind the
> dom.details_element.enabled pref. Bug 1226455 is for shipping without the
> pref.

Thanks Matt! I have linked to the MDN articles as the pref is mentioned there.
Flags: needinfo?(rkothari)
This shouldn't be in the release notes until it's actually enabled.  Could you remove it?
Flags: needinfo?(rkothari)
Ok. I can do that. We can add it back when they are pref enabled I suppose.
Flags: needinfo?(rkothari)
Thanks you to result , I have victory to support HTML5's <details> and <summary>.
So is this in FF 46.0 now? Not sure what to pull out of the last few messages here.
Mike, the code is checked into what will become Firefox 47, but it's not enabled there yet, because it has some issues still.

In Firefox 48, the code is enabled on nightly and developer edition only, not in final release.

The current plan is to enable in release in Firefox 49.  See bug 1226455.
Hi Marcia, could you please add this to Fx49 relnotes instead and update the relnote tracking flag to 49+? Thanks!
Flags: needinfo?(mozillamarcia.knous)
(In reply to Ritu Kothari (:ritu) from comment #276)
> Hi Marcia, could you please add this to Fx49 relnotes instead and update the
> relnote tracking flag to 49+? Thanks!

This was already listed in the Developer Notes on MDN: https://developer.mozilla.org/en-US/Firefox/Releases/49#Changes_for_Web_developers. Adding 49+ flag for relnote.
Flags: needinfo?(mozillamarcia.knous)
Depends on: 1562701
You need to log in before you can comment on or make changes to this bug.