Closed Bug 1225412 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

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.
> 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.  ;)
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
Assignee: nobody → tlin
MozReview-Commit-ID: EN6Jf5hVHHD
Status: NEW → ASSIGNED
File a chromium bug for the order of toggle events.
https://bugs.chromium.org/p/chromium/issues/detail?id=593576
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/
Attachment #8728797 - Flags: review?(bugs)
Component: Layout → DOM: Events
Attachment #8726138 - Attachment is obsolete: true
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/
Attachment #8728797 - Flags: review?(bugs)
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 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-.
Attachment #8728797 - Flags: review-
(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
> 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
(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.
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.
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/
Attachment #8729444 - Flags: review?(bugs)
Attachment #8728797 - Attachment description: MozReview Request: Bug 1225412 - Add support to dispatch toggle events to details element. r?smaug → MozReview Request: Bug 1225412 Part 2 - Add support to dispatch toggle events to details element. r?smaug
Attachment #8728797 - Flags: review- → review?(bugs)
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 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.
Attachment #8729444 - Flags: review?(bugs) → review+
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.
Attachment #8728797 - Flags: review?(bugs)
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.
Attachment #8728797 - Flags: review-
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.
https://reviewboard.mozilla.org/r/39447/#review36169

Thanks for the comment. I feel generic approch might benefit future use cases :)
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 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/
Attachment #8728797 - Flags: review- → review?(bugs)
Duplicate of this bug: 1254900
Attachment #8728797 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/48adfd045db5
https://hg.mozilla.org/mozilla-central/rev/5ee96a182b8d
https://hg.mozilla.org/mozilla-central/rev/e9f0c40c06e1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.