HTMLDetailsElement should fire a 'toggle' event

HTMLDetailsElement should fire a 'toggle' event

As per the specification, a 'toggle' event should be fired whenever the 'open'
attribute is added to or removed from a details element:
http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#attr-details-open

Firefox 28 and IE11 do not support this yet.

R=arv@chromium.org, tkent@chromium.org
BUG=366105
TEST=fast/html/details-open-toggle-event.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172427

[arv (Not doing code reviews), 2014-04-23 17:03:31]

Don't we have infrastructure for async event dispatch already?

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
File LayoutTests/fast/html/details-open-toggle-event.html (right):

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:19: testEvent = ev;
Please add a global var for this too.

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:45: toggleEventCount = 0;
I think this might read clearer if this was not reset.

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:55: details =
document.getElementById("details");
missing var

[Inactive, 2014-04-23 19:19:29]

I updated the code to use a GenericEventQueue, which I assume is the
infrastructure you were referring to. I hesitated using it earlier because it
does not make the code much simpler for such easy case IMHO.

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
File LayoutTests/fast/html/details-open-toggle-event.html (right):

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:19: testEvent = ev;
On 2014/04/23 17:03:32, arv wrote:
> Please add a global var for this too.

Done.

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:45: toggleEventCount = 0;
On 2014/04/23 17:03:32, arv wrote:
> I think this might read clearer if this was not reset.

Done.

https://codereview.chromium.org/249573002/diff/1/LayoutTests/fast/html/detail...
LayoutTests/fast/html/details-open-toggle-event.html:55: details =
document.getElementById("details");
On 2014/04/23 17:03:32, arv wrote:
> missing var

Done.

[Inactive, 2014-04-23 19:25:46]

On 2014/04/23 19:19:29, Chris Dumez wrote:
> I updated the code to use a GenericEventQueue, which I assume is the
> infrastructure you were referring to. I hesitated using it earlier because it
> does not make the code much simpler for such easy case IMHO.

The reason I think the GenericEventQueue is a bit overkill here is that we never
need to queue more than one event and we only have one type of event. Maybe
there is another simpler infrastructure for async event firing that I am not
aware of?

[esprehn, 2014-04-23 20:21:46]

You don't want the event queue, you might want an EventSender.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-04-23 20:51:01]

On 2014/04/23 20:21:46, esprehn wrote:
> You don't want the event queue, you might want an EventSender.

Thanks! I updated the CL to use EventSender instead.

[arv (Not doing code reviews), 2014-04-23 20:54:28]

LGTM

[tkent, 2014-04-23 21:36:46]

lgtm

https://codereview.chromium.org/249573002/diff/80001/Source/core/html/HTMLDet...
File Source/core/html/HTMLDetailsElement.h (right):

https://codereview.chromium.org/249573002/diff/80001/Source/core/html/HTMLDet...
Source/core/html/HTMLDetailsElement.h:29: typedef
EventSender<HTMLDetailsElement> DetailEventSender;
nit: DetailEventSender -> Detail*s*EventSender?

[Inactive, 2014-04-23 22:03:28]

https://codereview.chromium.org/249573002/diff/80001/Source/core/html/HTMLDet...
File Source/core/html/HTMLDetailsElement.h (right):

https://codereview.chromium.org/249573002/diff/80001/Source/core/html/HTMLDet...
Source/core/html/HTMLDetailsElement.h:29: typedef
EventSender<HTMLDetailsElement> DetailEventSender;
On 2014/04/23 21:36:46, tkent wrote:
> nit: DetailEventSender -> Detail*s*EventSender?

Done.

[Inactive, 2014-04-23 22:03:35]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-23 22:04:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/249573002/100001

[tkent, 2014-04-23 22:49:09]

We need to support ontoggle IDL attribute and ontoggle HTML attribute.  
(A separated patch is ok.)

[commit-bot: I haz the power, 2014-04-23 23:09:57]

Change committed as 172427

[Inactive, 2014-04-23 23:11:45]

On 2014/04/23 22:49:09, tkent wrote:
> We need to support ontoggle IDL attribute and ontoggle HTML attribute.  
> (A separated patch is ok.)

Good point. I'll take care of this in a follow-up patch.