'blur' on clicking padding area of 'number' input

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Events
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Yuriy, Assigned: jessica)

Tracking

({testcase})

46 Branch
mozilla49
testcase
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8746592 [details]
sample.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:

I'm clicking on padding area of input[type='number']


Actual results:

blur event fires


Expected results:

blur should not fire on clicking padding area of input

Comment 1

a year ago
Created attachment 8747006 [details]
sample.html (reporter's testcase)

Updated

a year ago
Keywords: testcase
Jessica's been looking at input types recently. Maybe she can take a quick look here?
Flags: needinfo?(jjong)
Whiteboard: btpp-followup-2016-05-06
(Assignee)

Comment 3

a year ago
(In reply to Andrew Overholt [:overholt] from comment #2)
> Jessica's been looking at input types recently. Maybe she can take a quick
> look here?

Sure, let me take a look at this.
Assignee: nobody → jjong
Flags: needinfo?(jjong)
(Assignee)

Comment 4

a year ago
Hi Olli,

Do you mind giving me some hint on this?
I noticed that when clicking on the padding area, both focus and blur events are fired. I'm looking at EventStateManager::PostHandleEvent, but I am not sure whether it is the right place that handles this logic, cause it seems that only input=number has this issue.

Thanks.
Flags: needinfo?(bugs)
aha, I need to click right side of the arrow buttons...

So first question to answer is that where is the focus being moved to when blur is fired? what is focus event's originalTarget? (web page can't access that but C++ can and privileged js.)

And EventStateManager is probably wrong place to look at. 
You may need to tweak HTMLInputElement::IsHTMLFocusable so that we don't try to move focus while focus is already in the relevant input type=number element. 
It is also possible that tabindex attribute handling in nsNumberControlFrame.cpp isn't quite right.
Flags: needinfo?(bugs)

Comment 6

a year ago
When I also faced this issue, I debugged that focus is being moved to the same element in blur event. You can get newly focused element as "document.activeElement". Please see this answer on how to do it: http://stackoverflow.com/a/11592974
(Assignee)

Comment 7

a year ago
Created attachment 8750208 [details] [diff] [review]
patch, v1.

When focusing on a number input control, the focus is redirected to the text control. So in 'CheckIfFocusable()', if the content to focus is a number control, we should redirect the check to it's anonymous text control, otherwise the number control gets blurred and focused again.
(Assignee)

Comment 8

a year ago
Comment on attachment 8750208 [details] [diff] [review]
patch, v1.

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

Olli, I have hacked nsFocusManager::GetRedirectedFocus() so that it returns the anon text control when it's a number input control, just like the other XUL elements.
Do you think it's a good approach for this bug? Thanks.
Attachment #8750208 - Flags: feedback?(bugs)
Whiteboard: btpp-followup-2016-05-06 → btpp-active
(Assignee)

Comment 9

a year ago
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45793739325e&selectedJob=20555340

The proposed patch breaks 'dom/html/test/forms/test_input_typing_sanitization.html', it seems that when input type is changed from number to date, the first time .focus() is called, the content to focus is still a number control. Still looking into it...
(Assignee)

Comment 10

a year ago
Created attachment 8750710 [details] [diff] [review]
patch, v2.

This patch fixes the failure in 'dom/html/test/forms/test_input_typing_sanitization.html'.
Turns out that when input type is changed from 'number' to 'date', the HTMLInputElement's type is changed but the corresponding frame has not switched yet. This patch adds an extra check on HTMLInputElement's type in nsFocusManager.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5a649a0053&selectedJob=20611026
Attachment #8750208 - Attachment is obsolete: true
Attachment #8750208 - Flags: feedback?(bugs)
Attachment #8750710 - Flags: feedback?(bugs)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 11

a year ago
Comment on attachment 8750710 [details] [diff] [review]
patch, v2.

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

I think this is ready for review, changing to r? :)
Attachment #8750710 - Flags: feedback?(bugs) → review?(bugs)
Blocks: 1272474
Comment on attachment 8750710 [details] [diff] [review]
patch, v2.

ok, I guess this works.
Can we get some tests for this? Perhaps something which 
checks using nsFocusManager that focus is in the inner element, but
document.activeElement still returns the <input type=number> ?
Attachment #8750710 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

a year ago
Created attachment 8755342 [details] [diff] [review]
tests, v1.

Thanks Olli, here come the tests.

Will merge the patches once they are reviewed.
Attachment #8755342 - Flags: review?(bugs)

Updated

a year ago
Attachment #8755342 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

a year ago
Created attachment 8755726 [details] [diff] [review]
final patch. r=smaug
Attachment #8750710 - Attachment is obsolete: true
Attachment #8755342 - Attachment is obsolete: true
Attachment #8755726 - Flags: review+
(Assignee)

Comment 15

a year ago
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f12da6706394

(compared with other try results, the intermittent failures in Win7 are not related to this bug)
Keywords: checkin-needed

Comment 16

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c1f4b7c2ebd
Keywords: checkin-needed

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c1f4b7c2ebd
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

a year ago
Duplicate of this bug: 1272474
You need to log in before you can comment on or make changes to this bug.