1225412 - Add support to fire 'toggle' event at the details element

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

According to the <details> spec, whenever 'open' attribute is added to or removed from a details element, a simple 'toggle' event must be fired at the details element. 

This bug is for implementing the 'toggle' event and 'ontoggle' event handler for details element.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

We have a test case in web-platform-test.

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/interactive-elements/the-details-element/toggleEvent.html

Comment 2

[Boris Zbarsky [:bzbarsky]]

> a simple 'toggle' event must be fired at the details element

It's a bit more complicated, due to the coalescing.  But the wpt test tests that, so if we end up passing it we're probably good to go.  ;)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I'm working on this. Still trying to figure out how to implement the coalescing requirement of the toggle events to pass test in [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/4ea7408b3eef059aa248f4b00328f8fdb4475112/testing/web-platform/tests/html/semantics/interactive-elements/the-details-element/toggleEvent.html#80-88

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add ontoggle event handler and dispatch toggle event. (WIP)

MozReview-Commit-ID: EN6Jf5hVHHD

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

File a chromium bug for the order of toggle events.
https://bugs.chromium.org/p/chromium/issues/detail?id=593576

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

Add ontoggle event handler, and dispatch toggle events to the details
element if the open attribute is changed. According to the spec, if a
toggle event has been queued, the subsequent toggle events must not be
fired.

Review commit: https://reviewboard.mozilla.org/r/39135/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39135/

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39135/diff/1-2/

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

https://reviewboard.mozilla.org/r/39135/#review36011

::: dom/html/HTMLDetailsElement.cpp:83
(Diff revision 2)
> +  // Don't fire another toggle event if we already have a pending one.

So per spec adding "open" content attribute should dispatch the event and idl attribute just reflects that. 
So, setAttribute("open", ""); should end up firing the event.

So, I would override AfterSetAttr and there dispatch the event and then SetOpen() could just set the attribute.


Also, the task queuing isn't quite what the spec says. Per spec we should always queue a task and then when we're about to run it, check whether another task has been queued. The difference comes from the fact that per spec changing open attribute ends up effectively push the event dispatch task to the end of the event loop and removing any earlier tasks.
Would be good to test other browsers actually follow the spec here.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

Silly MozReview. Doesn't allow setting r-.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Also, the task queuing isn't quite what the spec says. Per spec we should
> always queue a task and then when we're about to run it, check whether
> another task has been queued. The difference comes from the fact that per
> spec changing open attribute ends up effectively push the event dispatch
> task to the end of the event loop and removing any earlier tasks.

I think the spec is worded specifically *not* to do that.  The spec says to use the earlier task and not enqueue the later ones.

https://html.spec.whatwg.org/multipage/forms.html#details-notification-task-steps says:
> 1. If another task has been queued to run the details notification task steps for this details element, then abort these steps.

> Would be good to test other browsers actually follow the spec here.

Chrome does not; Ting-Yu filed https://bugs.chromium.org/p/chromium/issues/detail?id=593576

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

> 1. If another task has been queued to run the details notification task steps for 
> this details element, then abort these steps.

Is it just me who feels that "abort these steps" can be interpreted either as "abort these steps of the previous queued tasks" or "abort these steps of the current task which is to be enqueued"?

FWIW, Chrome cancel the previous queued toggle event and enqueue a new one as in [1]. The test case I attached in the Chrome bug mentioned in comment 5 can confirm this.

[1] https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebKit/Source/core/html/HTMLDetailsElement.cpp#148

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #11)
> > 1. If another task has been queued to run the details notification task steps for 
> > this details element, then abort these steps.
> 
> Is it just me who feels that "abort these steps" can be interpreted either
> as "abort these steps of the previous queued tasks" or "abort these steps of
> the current task which is to be enqueued"?

Er, you're right.  I misread the spec before and thought it was the latter, but it's actually the former, and Chrome's behavior is correct.

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://reviewboard.mozilla.org/r/39135/#review36011

> So per spec adding "open" content attribute should dispatch the event and idl attribute just reflects that. 
> So, setAttribute("open", ""); should end up firing the event.
> 
> So, I would override AfterSetAttr and there dispatch the event and then SetOpen() could just set the attribute.
> 
> 
> Also, the task queuing isn't quite what the spec says. Per spec we should always queue a task and then when we're about to run it, check whether another task has been queued. The difference comes from the fact that per spec changing open attribute ends up effectively push the event dispatch task to the end of the event loop and removing any earlier tasks.
> Would be good to test other browsers actually follow the spec here.

I'll use AfterSetAttr() in my next patch. 

You're right about the spec. To have to ability to removing tasks, we'll need to make AsyncEventDispatcher cancelable.

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: MozReview Request: Bug 1225412 Part 1 - Make AsyncEventDispatcher cancelable. r?smaug

Also I found that mBubbles are not initialized in the constructor in
AsyncEventDispatcher.cpp. I initialized those bool member variables
directly.

Review commit: https://reviewboard.mozilla.org/r/39447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39447/

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39135/diff/2-3/

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8729444 [details]
MozReview Request: Bug 1225412 Part 1 - Make AsyncEventDispatcher cancelable. r?smaug

https://reviewboard.mozilla.org/r/39447/#review36169

Ah, we can add support for Cancelling in general. Can be useful elsewhere. Other option would have been to just check in 
ToggleEventDispatcher::Run() whether 'this' is the currently pending async event.
But ok, this approach is more generic.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

https://reviewboard.mozilla.org/r/39135/#review36173

::: dom/html/HTMLDetailsElement.cpp:74
(Diff revision 3)
>      NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>    }
>    return hint;
>  }
>  
> +nsresult

Oh, I gave wrong suggestion. Af.
terSetAttr doesn't quite work since we can't check whether there already was open attribute and per spec the event should be dispatched only "Whenever the open attribute is added to or removed from a details element". So need to use BeforeSetAttr.
And actually add a test that just changing the attribute value if there is attribute already doesn't dispatch event.

Sorry, I should have noticed before.

::: dom/html/HTMLDetailsElement.cpp:79
(Diff revision 3)
> +nsresult
> +HTMLDetailsElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
> +                                 const nsAttrValue* aValue, bool aNotify)
> +{
> +  if (aNameSpaceID == kNameSpaceID_None && aName == nsGkAtoms::open) {
> +    if (mToggleEventDispatcher.get()) {

no need for .get().
if (mToggleEventDispatcher) {
...
}
should work just fine.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

(mozreview is silly and doesn't let one to r-).

So, use BeforeSetAttr and there check whether the existence of the attribute changes, and only in that case dispatch the event.
And add a test for this case, hopefully a web-platform-test. Just modify some existing test file.

Comment 19

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://reviewboard.mozilla.org/r/39135/#review36173

> Oh, I gave wrong suggestion. Af.
> terSetAttr doesn't quite work since we can't check whether there already was open attribute and per spec the event should be dispatched only "Whenever the open attribute is added to or removed from a details element". So need to use BeforeSetAttr.
> And actually add a test that just changing the attribute value if there is attribute already doesn't dispatch event.
> 
> Sorry, I should have noticed before.

You're right. When the 'open' attribute already exists, set the 'open' attribute shouldn't fire toggle event. I'll add web platform tests to covert this.

Sorry I should have read the spec thoroghly.

> no need for .get().
> if (mToggleEventDispatcher) {
> ...
> }
> should work just fine.

Right, get() is not needed.

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://reviewboard.mozilla.org/r/39447/#review36169

Thanks for the comment. I feel generic approch might benefit future use cases :)

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8729444 [details]
MozReview Request: Bug 1225412 Part 1 - Make AsyncEventDispatcher cancelable. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39447/diff/1-2/

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39135/diff/3-4/

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8728797 [details]
MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug

https://reviewboard.mozilla.org/r/39135/#review36285

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48adfd045db5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee96a182b8d

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9f0c40c06e1

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/48adfd045db5
https://hg.mozilla.org/mozilla-central/rev/5ee96a182b8d
https://hg.mozilla.org/mozilla-central/rev/e9f0c40c06e1