Closed Bug 1268556 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: yurko.sirko, Assigned: jessica)

References

Details

(Keywords: testcase, Whiteboard: btpp-active)

Attachments

(3 files, 3 obsolete files)

Attached file 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
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
(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)
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)
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
Attached patch patch, v1. (obsolete) — Splinter Review
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.
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
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...
Attached patch patch, v2. (obsolete) — Splinter Review
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
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)
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+
Attached patch tests, v1. (obsolete) — Splinter Review
Thanks Olli, here come the tests.

Will merge the patches once they are reviewed.
Attachment #8755342 - Flags: review?(bugs)
Attachment #8755342 - Flags: review?(bugs) → review+
Attachment #8750710 - Attachment is obsolete: true
Attachment #8755342 - Attachment is obsolete: true
Attachment #8755726 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/0c1f4b7c2ebd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1272474
You need to log in before you can comment on or make changes to this bug.