Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Description • 9 years ago

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.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 1 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 9 years ago

> 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.  ;)

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 3 • 8 years ago

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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 4 • 8 years ago

MozReview-Commit-ID: EN6Jf5hVHHD

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 5 • 8 years ago

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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 6 • 8 years ago

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/

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 7 • 8 years ago

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/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 8 years ago

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-.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 10 • 8 years ago

(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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 11 • 8 years ago

> 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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 12 • 8 years ago

(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.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 13 • 8 years ago

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.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 14 • 8 years ago

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/

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 15 • 8 years ago

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/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 18 • 8 years ago

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.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 19 • 8 years ago

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.

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 20 • 8 years ago

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

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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 21 • 8 years ago

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/

	Ting-Yu Lin [:TYLin] (PDT, UTC-7) Assignee
	Comment 22 • 8 years ago

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/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 8 years ago

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

	Pulsebot
	Comment 25 • 8 years ago

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

	Pulsebot
	Comment 26 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 8 years ago
bugherder

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