Bug 109176 – Support a relatedTarget attribute on focus/blur events

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

In bug 76216, we supported a relatedTarget attribute on focusin/focusout events. We should support it on focus/blur events too.

See http://lists.w3.org/Archives/Public/www-dom/2012OctDec/0061.html for the www-dom discussion.

In a follow-up patch, I'll move {FocusIn,FocusOut,Focus,Blur}EventDispatchMediator to FocusEvent.cpp.

[Kentaro Hara]

Created attachment 187060 [details]
Patch

[Dimitri Glazkov (Google)]

Hayato-san

[Build Bot]

Comment on attachment 187060 [details]
Patch

Attachment 187060 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16434235

New failing tests:
fast/dom/adopt-node-prevented.html

[WebKit Review Bot]

Comment on attachment 187060 [details]
Patch

Attachment 187060 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16428282

New failing tests:
fast/forms/time-multiple-fields/time-multiple-fields-crash-after-adoptnode.html
fast/dom/adopt-node-prevented.html

[Eric Seidel (no email)]

Comment on attachment 187060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187060&action=review

> Source/WebCore/dom/Node.cpp:2426
> +    dispatchScopedEventDispatchMediator(FocusEventDispatchMediator::create(FocusEvent::create(eventNames().focusEvent, false, false, document()->defaultView(), 0, 0), oldFocusedNode));

I feel like we should have an internal create method which takes fewer args.

[Kentaro Hara]

Created attachment 187216 [details]
patch for landing

[Kentaro Hara]

Comment on attachment 187216 [details]
patch for landing

hayato-san: cq? if it looks OK?

[Hayato Ito]

Comment on attachment 187216 [details]
patch for landing

RelatedTarget! Yay! I hope that makes both event retargeting code and section 5 of shadow dom spec much simpler.

View in context: https://bugs.webkit.org/attachment.cgi?id=187216&action=review

> Source/WebCore/dom/EventDispatchMediator.cpp:55
> +PassRefPtr<FocusEventDispatchMediator> FocusEventDispatchMediator::create(PassRefPtr<Event> event, PassRefPtr<Node> oldFocusedNode)

We don't need a second parameter since FocusEvent has a relatedTarget. It seems redundant.

> Source/WebCore/dom/EventDispatchMediator.cpp:60
> +FocusEventDispatchMediator::FocusEventDispatchMediator(PassRefPtr<Event> event, PassRefPtr<Node> oldFocusedNode)

Ditto.

> Source/WebCore/dom/EventDispatchMediator.cpp:72
> +PassRefPtr<BlurEventDispatchMediator> BlurEventDispatchMediator::create(PassRefPtr<Event> event, PassRefPtr<Node> newFocusedNode)

Ditto.

> Source/WebCore/dom/EventDispatchMediator.cpp:77
> +BlurEventDispatchMediator::BlurEventDispatchMediator(PassRefPtr<Event> event, PassRefPtr<Node> newFocusedNode)

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:62
> +    static PassRefPtr<FocusEventDispatchMediator> create(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:64
> +    FocusEventDispatchMediator(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:71
> +    static PassRefPtr<BlurEventDispatchMediator> create(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:73
> +    BlurEventDispatchMediator(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/Node.cpp:2426
> +    RefPtr<FocusEvent> event = FocusEvent::create(eventNames().focusEvent, false, false, document()->defaultView(), 0, 0);

You should pass oldFocusedNode to FocusEvent::create().

> Source/WebCore/dom/Node.cpp:2427
> +    EventDispatcher::dispatchEvent(this, FocusEventDispatchMediator::create(event.release(), oldFocusedNode));

Then you don't have to pass oldFocusedNode here.

> Source/WebCore/dom/Node.cpp:2435
> +    RefPtr<FocusEvent> event = FocusEvent::create(eventNames().blurEvent, false, false, document()->defaultView(), 0, 0);

Ditto.

> Source/WebCore/dom/Node.cpp:2436
> +    EventDispatcher::dispatchEvent(this, BlurEventDispatchMediator::create(event.release(), newFocusedNode));

Ditto.

> LayoutTests/fast/events/related-target-focusevent.html:22
> +input1.addEventListener("focus", function(event) { shouldBe('event.relatedTarget', 'null'); });

Use shouldBeNull(..)

> LayoutTests/fast/events/related-target-focusevent.html:28
>  input3.addEventListener("focusin", function(event) { shouldBe('event.relatedTarget', 'input2'); });

it seems tests for input2 and input3 are testing the same things. We can remove either test.

> LayoutTests/fast/events/related-target-focusevent.html:33
>  for (var i = 0; i < 4; i++) {

I am afraid that it is not guaranteed that pressing tab key will focus the first focusable node (input1) in document.
It might be safe to focus input1 explicitly  at the begining before sending tab key.

[Kentaro Hara]

Thanks hayato-san

> > Source/WebCore/dom/Node.cpp:2426
> > +    RefPtr<FocusEvent> event = FocusEvent::create(eventNames().focusEvent, false, false, document()->defaultView(), 0, 0);
> 
> You should pass oldFocusedNode to FocusEvent::create().
> 
> > Source/WebCore/dom/Node.cpp:2427
> > +    EventDispatcher::dispatchEvent(this, FocusEventDispatchMediator::create(event.release(), oldFocusedNode));
> 
> Then you don't have to pass oldFocusedNode here.

We've tried this before, but it caused failures on fast/dom/shadow/shadow-boundary-events.html. Events wrongly propagated over shadow boundaries. (See https://bugs.webkit.org/show_bug.cgi?id=76216.) It looks like we shouldn't set a relatedTarget until EventContext::handleLocalEvents() is called.

[Kentaro Hara]

Created attachment 187233 [details]
patch for landing

[Hayato Ito]

(In reply to comment #9)
> Thanks hayato-san
> 
> > > Source/WebCore/dom/Node.cpp:2426
> > > +    RefPtr<FocusEvent> event = FocusEvent::create(eventNames().focusEvent, false, false, document()->defaultView(), 0, 0);
> > 
> > You should pass oldFocusedNode to FocusEvent::create().
> > 
> > > Source/WebCore/dom/Node.cpp:2427
> > > +    EventDispatcher::dispatchEvent(this, FocusEventDispatchMediator::create(event.release(), oldFocusedNode));
> > 
> > Then you don't have to pass oldFocusedNode here.
> 
> We've tried this before, but it caused failures on fast/dom/shadow/shadow-boundary-events.html. Events wrongly propagated over shadow boundaries. (See https://bugs.webkit.org/show_bug.cgi?id=76216.) It looks like we shouldn't set a relatedTarget until EventContext::handleLocalEvents() is called.

Could you figure out the reason?

I cannot read why we can not set relatedTaraget in FocusEvent constructor in https://bugs.webkit.org/show_bug.cgi?id=76216.
The thread is too long. :)

[Kentaro Hara]

(In reply to comment #11)
> Could you figure out the reason?

OK, will try later. I couldn't figure out in one day debugging though:(

[Kentaro Hara]

Created attachment 187235 [details]
patch that fixed test cases for now

[Kentaro Hara]

(In reply to comment #8)
> > LayoutTests/fast/events/related-target-focusevent.html:22
> > +input1.addEventListener("focus", function(event) { shouldBe('event.relatedTarget', 'null'); });
> 
> Use shouldBeNull(..)

Fixed.

> > LayoutTests/fast/events/related-target-focusevent.html:28
> >  input3.addEventListener("focusin", function(event) { shouldBe('event.relatedTarget', 'input2'); });
> 
> it seems tests for input2 and input3 are testing the same things. We can remove either test.

Fixed.

> > LayoutTests/fast/events/related-target-focusevent.html:33
> >  for (var i = 0; i < 4; i++) {
> 
> I am afraid that it is not guaranteed that pressing tab key will focus the first focusable node (input1) in document.
> It might be safe to focus input1 explicitly  at the begining before sending tab key.

Fixed.


> I cannot read why we can not set relatedTaraget in FocusEvent constructor in https://bugs.webkit.org/show_bug.cgi?id=76216.

This issue happens not only in focus/blur events but also in focusin/focusout events. I filed a bug for this (https://bugs.webkit.org/show_bug.cgi?id=109261). Let me fix it after landing the patch.

[Hayato Ito]

(In reply to comment #14)
> (In reply to comment #8)
> > > LayoutTests/fast/events/related-target-focusevent.html:22
> > > +input1.addEventListener("focus", function(event) { shouldBe('event.relatedTarget', 'null'); });
> > 
> > Use shouldBeNull(..)
> 
> Fixed.
> 
> > > LayoutTests/fast/events/related-target-focusevent.html:28
> > >  input3.addEventListener("focusin", function(event) { shouldBe('event.relatedTarget', 'input2'); });
> > 
> > it seems tests for input2 and input3 are testing the same things. We can remove either test.
> 
> Fixed.

You don't have to remove both. We should have a test where a relatedTarget is null.

> 
> > > LayoutTests/fast/events/related-target-focusevent.html:33
> > >  for (var i = 0; i < 4; i++) {
> > 
> > I am afraid that it is not guaranteed that pressing tab key will focus the first focusable node (input1) in document.
> > It might be safe to focus input1 explicitly  at the begining before sending tab key.
> 
> Fixed.
> 
> 
> > I cannot read why we can not set relatedTaraget in FocusEvent constructor in https://bugs.webkit.org/show_bug.cgi?id=76216.
> 
> This issue happens not only in focus/blur events but also in focusin/focusout events. I filed a bug for this (https://bugs.webkit.org/show_bug.cgi?id=109261). Let me fix it after landing the patch.

Ideally, we should fix bug 109261 before landing this since this patch is spreading the misusage.
Fixing later is okay to me, but you should add FIXME comments, pointing to bug id, near the corresponding code.

[Kentaro Hara]

Created attachment 187242 [details]
comments addressed

[Kentaro Hara]

Comment on attachment 187242 [details]
comments addressed

> You don't have to remove both. We should have a test where a relatedTarget is null.

Fixed.

> Ideally, we should fix bug 109261 before landing this since this patch is spreading the misusage.
> Fixing later is okay to me, but you should add FIXME comments, pointing to bug id, near the corresponding code.

Thanks. Done.

[Hayato Ito]

Comment on attachment 187242 [details]
comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=187242&action=review

> Source/WebCore/dom/EventDispatchMediator.h:62
> +    static PassRefPtr<FocusEventDispatchMediator> create(PassRefPtr<Event>, PassRefPtr<Node>);

Could you add  the parameter name for PassRefPtr<Node> here?

> Source/WebCore/dom/EventDispatchMediator.h:64
> +    FocusEventDispatchMediator(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:71
> +    static PassRefPtr<BlurEventDispatchMediator> create(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

> Source/WebCore/dom/EventDispatchMediator.h:73
> +    BlurEventDispatchMediator(PassRefPtr<Event>, PassRefPtr<Node>);

Ditto.

[Kentaro Hara]

Created attachment 187243 [details]
comments addressed

[Kentaro Hara]

Comment on attachment 187243 [details]
comments addressed

> Could you add  the parameter name for PassRefPtr<Node> here?

Done.

[Hayato Ito]

Thank you!
Unofficial rs=me on this.

(In reply to comment #20)
> (From update of attachment 187243 [details])
> > Could you add  the parameter name for PassRefPtr<Node> here?
> 
> Done.

[Kentaro Hara]

Comment on attachment 187243 [details]
comments addressed

Thanks!

[WebKit Review Bot]

Comment on attachment 187243 [details]
comments addressed

Clearing flags on attachment: 187243

Committed r142240: <http://trac.webkit.org/changeset/142240>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.