1301306 - [DateTimeInput] blur/focus events triggered when moving through inner text boxes

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Follow-up to Bug 1288591.

There is a known issue where the input element gets blurred and focused again when moving through inner text boxes.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8800993 [details] [diff] [review]
patch, v1.

Review of attachment 8800993 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Olli, I couldn't use aVisitor.mEvent->StopPropagation(), cause then the cursor does not show on the text box, aVisitor.mEvent->stopCrossProcessForwarding() does not work either. So I am using mCanHandle and mAutomaticChromeDispatch to stop the event. Is this the right approach? Thanks.

Comment 3

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

Comment on attachment 8800993 [details] [diff] [review]
patch, v1.

I think you could use "composed" stuff here.
Perhaps set mComposedInNativeAnonymousContent to false in PreHandleEvent.
See bug 1292063 (and ping stone)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Hi Olli, I can use 'mComposedInNativeAnonymousContent' to stop the event when moving through inner text boxes, however it doesn't help the case of clicking on padding area (the input also gets blurred and focused), so I still need the last part using .mCanHandle and .mAutomaticChromeDispatch.

Comment 5

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

Hmm, why doesn't mComposedInNativeAnonymousContent work? 
What are the originalTarget and relatedTarget of the event in that padding area case?
I'd assume one is something in NAC and one is the non-NAC input element.

Comment 6

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

Comment on attachment 8801655 [details] [diff] [review]
patch, v2.

So shouldn't you catch some more cases here.
Like when related target isn't something in NAC, but the non-NAC input itself.
That one doesn't return anything from GetOwnerDateTimeControl(), right?

Comment 7

[Jessica Jong [:jessica] (inactive)]

In the padding area case, we get:
- INPUT_TEXT blur (relatedTarget: INPUT_TIME)
- INPUT_TIME focus (relatedTarget: INPUT_TEXT)
- INPUT_TIME blur (relatedTarget: INPUT_TEXT)
- INPUT_TEXT focus (relatedTarget: INPUT_TIME)

By catching more cases, as mentioned in comment 6, and using mComposedInNativeAnonymousContent, I can stop the first and last event, but since mComposedInNativeAnonymousContent only works in NAC, it doesn't work in the second and third event. In these cases, only .mCanHandle and .mAutomaticChromeDispatch works.

Comment 8

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

I must be missing something...
Why can't you stop them all in the non-NAC level? You compare event's originalTarget and relatedTarget or so.
Basically compare if originalTarget->FindFirstNonChromeOnlyAccessContent() to relatedTarget->FindFirstNonChromeOnlyAccessContent(). If they both are the non-NAC input element, then event could be mCanHandle = false; or so.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Hi Olli, per our talk on IRC, I am trying to stop all the events on non-NAC level using mCanHandle, since using mComposedInNativeAnonymousContent can not cover all cases. But if, as suggested, setting only mCanHandle (and not setting mAutomaticChromeDispatch), the cursor does not show after the focus moves to the next anonymous text box. So I need both parts to work, as this patch shows. What do you think?

Comment 10

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

Comment on attachment 8802402 [details] [diff] [review]
patch, v3.

aha, ok, so there is some listener somewhere listening focus to trigger cursor.
Sounds like mAutomaticChromeDispatch is needed then.

So why we'd need these two different 'if's.
Wouldn't one work in mType == NS_FORM_INPUT_TIME level and there
check if either originalTarget or relatedTarget is in NAC (IsInNativeAnonymousSubtree) and if so, check that both's
FindFirstNonChromeOnlyAccessContent() return the same object which should be 'this'. And if so aVisitor.mCanHandle = false; mAutomaticChromeDispatch = true;
In other words very close to what you did in patch 1.


The change to datetimebox.xml looks wrong. Web page could detect that prevent default was called, right? I mean if it has event listener for mousedown, it could check if event.defaultPrevented is true. And if web page calls event.stopPropagation() in capture phase, that event listener won't even be called.
Why is that change needed?

If it is needed:
It is possible to add listeners to system event group 
http://searchfox.org/mozilla-central/source/dom/webidl/EventTarget.webidl#18
(system event group listeners are called after the dispatch in normal group has been done. stopPropagation in the default group doesn't affect to those listeners.)
But we may need some new preventDefault() which XBL code can call without affecting defaultPrevented value in web pages.
Currently we have such for chrome js http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/events/Event.cpp#507

Comment 11

[Jessica Jong [:jessica] (inactive)]

Per our discussion on IRC, need to find out why setting only mCanHandle=false (and not setting mAutomaticChromeDispatch), the cursor does not show.

These are my today's findings:
EditorEventListener listens for focus events and calls EditorBase to initialize the selection/caret [1]. However, before calling EditorBase, EditorEventListener checks if the target element is still the same as the focused element (from nsFocusManager) [2]. Whe setting mCanHandle=false only, the event target element is *input time* but the focused element is *input text*, so it bails out. In other cases, the event target element and the focused element are both *input text*.

The code in EventDispatcher differs from here [3] when mCanHandle is set to false. And when mAutomaticChromeDispatch is set to false as well, it does not create event target chain item for the parent [4].

[1] http://hg.mozilla.org/mozilla-central/file/99a239e1866a/editor/libeditor/EditorEventListener.cpp#l1097
[2] http://hg.mozilla.org/mozilla-central/file/99a239e1866a/editor/libeditor/EditorEventListener.cpp#l1091
[3] http://hg.mozilla.org/mozilla-central/file/99a239e1866a/dom/events/EventDispatcher.cpp#l674
[4] http://hg.mozilla.org/mozilla-central/file/99a239e1866a/dom/events/EventDispatcher.cpp#l679

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

As suggested on IRC, in EditorEventListener, we'll use original target's first non-native ancestor to compare to focused elemenet's first non-native ancestor. This fixes the cursor issue. Tested on input type=number and it works as expected.

I think we don't need the preventDefault's in the button for now. I was just wondering whether we should let the button get the focus or not.

Comment 13

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

Not exactly about this bug, but do we still need some tweak to FormFillController when dealing with date/time types?
Thinking about nsFormFillController::MaybeStartControllingInput and such.

Comment 14

[Kevin Wern]

eFocusin/eFocusout will need to be added to these cases in either this patch or bug 687787's patch (depending on which lands first).

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #13)
> Not exactly about this bug, but do we still need some tweak to
> FormFillController when dealing with date/time types?
> Thinking about nsFormFillController::MaybeStartControllingInput and such.

Since input type=time is no longer a single line text control [1], nsFormFillController does not handle it [2].

[1] http://hg.mozilla.org/mozilla-central/file/b1b18f25c0ea/dom/html/nsIFormControl.h#l270
[2] http://hg.mozilla.org/mozilla-central/file/b1b18f25c0ea/toolkit/components/satchel/nsFormFillController.cpp#l903

Comment 16

[Jessica Jong [:jessica] (inactive)]

(In reply to Kevin Wern from comment #14)
> eFocusin/eFocusout will need to be added to these cases in either this patch
> or bug 687787's patch (depending on which lands first).

Thanks for the gentle reminder. Since bug 687787 has marked checkin-needed, I can wait for it to land and change accordingly. Or let me know if you'll need more time, so can I land first :).

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v5.

Handle also eFocusIn and eFocusOut. Carrying r+.

Comment 18

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48664e511d34db40f9431b6e81995b40d6297652&group_state=expanded

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d27bc400b98
Stop focus events from anon. content when moving inside input=time element. r=smaug

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8d27bc400b98