Bug 109176 - Support a relatedTarget attribute on focus/blur events
Summary: Support a relatedTarget attribute on focus/blur events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-07 04:23 PST by Kentaro Hara
Modified: 2013-02-08 00:07 PST (History)
13 users (show)

See Also:


Attachments
Patch (13.45 KB, patch)
2013-02-07 04:26 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (13.63 KB, patch)
2013-02-07 18:48 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (13.94 KB, patch)
2013-02-07 21:35 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch that fixed test cases for now (13.88 KB, patch)
2013-02-07 21:43 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
comments addressed (15.61 KB, patch)
2013-02-07 22:56 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
comments addressed (15.76 KB, patch)
2013-02-07 23:10 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-02-07 04:23:26 PST
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.
Comment 1 Kentaro Hara 2013-02-07 04:26:06 PST
Created attachment 187060 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2013-02-07 08:58:45 PST
Hayato-san
Comment 3 Build Bot 2013-02-07 09:24:24 PST
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
Comment 4 WebKit Review Bot 2013-02-07 10:40:25 PST
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
Comment 5 Eric Seidel (no email) 2013-02-07 11:05:48 PST
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.
Comment 6 Kentaro Hara 2013-02-07 18:48:27 PST
Created attachment 187216 [details]
patch for landing
Comment 7 Kentaro Hara 2013-02-07 18:58:01 PST
Comment on attachment 187216 [details]
patch for landing

hayato-san: cq? if it looks OK?
Comment 8 Hayato Ito 2013-02-07 20:24:48 PST
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.
Comment 9 Kentaro Hara 2013-02-07 21:22:34 PST
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.
Comment 10 Kentaro Hara 2013-02-07 21:35:05 PST
Created attachment 187233 [details]
patch for landing
Comment 11 Hayato Ito 2013-02-07 21:39:39 PST
(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. :)
Comment 12 Kentaro Hara 2013-02-07 21:41:07 PST
(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:(
Comment 13 Kentaro Hara 2013-02-07 21:43:01 PST
Created attachment 187235 [details]
patch that fixed test cases for now
Comment 14 Kentaro Hara 2013-02-07 22:23:27 PST
(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.
Comment 15 Hayato Ito 2013-02-07 22:39:37 PST
(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.
Comment 16 Kentaro Hara 2013-02-07 22:56:56 PST
Created attachment 187242 [details]
comments addressed
Comment 17 Kentaro Hara 2013-02-07 22:58:43 PST
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.
Comment 18 Hayato Ito 2013-02-07 23:08:20 PST
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.
Comment 19 Kentaro Hara 2013-02-07 23:10:58 PST
Created attachment 187243 [details]
comments addressed
Comment 20 Kentaro Hara 2013-02-07 23:11:36 PST
Comment on attachment 187243 [details]
comments addressed

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

Done.
Comment 21 Hayato Ito 2013-02-07 23:20:07 PST
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.
Comment 22 Kentaro Hara 2013-02-07 23:21:40 PST
Comment on attachment 187243 [details]
comments addressed

Thanks!
Comment 23 WebKit Review Bot 2013-02-08 00:07:25 PST
Comment on attachment 187243 [details]
comments addressed

Clearing flags on attachment: 187243

Committed r142240: <http://trac.webkit.org/changeset/142240>
Comment 24 WebKit Review Bot 2013-02-08 00:07:32 PST
All reviewed patches have been landed.  Closing bug.