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

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)

Details

(Whiteboard: [milestone4])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

a year ago
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.
Blocks: 888320
(Assignee)

Updated

10 months ago
Assignee: nobody → jjong
(Assignee)

Comment 1

10 months ago
Created attachment 8800993 [details] [diff] [review]
patch, v1.
(Assignee)

Comment 2

10 months ago
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.
Attachment #8800993 - Flags: feedback?(bugs)

Comment 3

10 months ago
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)
Attachment #8800993 - Flags: feedback?(bugs)
(Assignee)

Comment 4

10 months ago
Created attachment 8801655 [details] [diff] [review]
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.
Attachment #8800993 - Attachment is obsolete: true
Attachment #8801655 - Flags: feedback?(bugs)

Comment 5

10 months ago
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

10 months ago
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?
Attachment #8801655 - Flags: feedback?(bugs)
(Assignee)

Comment 7

10 months ago
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.
Flags: needinfo?(bugs)

Comment 8

10 months ago
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 9

10 months ago
Created attachment 8802402 [details] [diff] [review]
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?
Attachment #8801655 - Attachment is obsolete: true
Attachment #8802402 - Flags: feedback?(bugs)

Comment 10

10 months ago
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
Attachment #8802402 - Flags: feedback?(bugs)
(Assignee)

Comment 11

10 months ago
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
(Assignee)

Comment 12

10 months ago
Created attachment 8803274 [details] [diff] [review]
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.
Attachment #8803274 - Flags: review?(bugs)
(Assignee)

Updated

10 months ago
Attachment #8802402 - Attachment is obsolete: true

Updated

10 months ago
Attachment #8803274 - Flags: review?(bugs) → review+

Comment 13

10 months ago
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

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

Comment 15

10 months ago
(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
(Assignee)

Comment 16

10 months ago
(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 :).
(Assignee)

Comment 17

10 months ago
Created attachment 8807444 [details] [diff] [review]
patch, v5.

Handle also eFocusIn and eFocusOut. Carrying r+.
Attachment #8803274 - Attachment is obsolete: true
Attachment #8807444 - Flags: review+
(Assignee)

Comment 18

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48664e511d34db40f9431b6e81995b40d6297652&group_state=expanded
Keywords: checkin-needed

Comment 19

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d27bc400b98
Stop focus events from anon. content when moving inside input=time element. r=smaug
Keywords: checkin-needed

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d27bc400b98
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Whiteboard: [milestone4]
You need to log in before you can comment on or make changes to this bug.