Closed Bug 926019 Opened 8 years ago Closed 5 years ago

focus and blur an input element with type of text will trigger change event even if content hasn't changed

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: xulxqqqq, Assigned: jessica)

References

Details

(Keywords: testcase, Whiteboard: btpp-active)

Attachments

(2 files, 1 obsolete file)

very simplistic showcase to highlight the issue
630 bytes, text/html
Details
patch, v2.
4.61 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Steps to reproduce:

The page code such as follow:
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body>
<input type="text" value="1">
<script type="text/javascript">
document.getElementsByTagName('input')[0].onfocus = function(){
	this.blur();
};

document.getElementsByTagName('input')[0].onchange = function(){
	alert('changed');
}
</script>
</body>
</html>


Actual results:

The problem occurs on Firefox 20.0 (Ubunt 13.04 32bit) / 20.0.1 (Windows 7 64)
When click input first time, the alert popups.
Then click input again, alert will not show.


Expected results:

On Firefox 11.0 (windows) and previous version / chrome / ie
Clicks input and alert will never show
Summary: focus and blur and input with type of text will trigger change event even if content hasn't changed → focus and blur an input element with type of text will trigger change event even if content hasn't changed
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Keywords: testcase
I was able to reproduce this on Nightly.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 24 Branch → Trunk
Assignee: nobody → jjong
The reason for this is because blur pre/postHandleEvent is nested in focus pre/postHandleEvent, but mFocusedValue is updated in focus postHandleEvent:
  1. preHandleEvent focus
  2. preHandleEvent blur -> compare value and mFocusedValue and fire change event if needed
  3. postHandleEvent blur
  4. postHandleEvent focus -> update mFocusedValue

A way to fix this is to update mFocusedValue in focus preHandleEvent. Need to check if this has any side effects.
Attached patch patch, v1. (obsolete) — — Splinter Review 
Comment on attachment 8756160 [details] [diff] [review]
patch, v1.

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

Hi Olli, I ran try and it seems okay, do you think this fix is good? Thanks.
Attachment #8756160 - Flags: feedback?(bugs)
Comment on attachment 8756160 [details] [diff] [review]
patch, v1.


>+  if (aVisitor.mEvent->mMessage == eFocus &&
while you're here, want to check that the event is actually trusted.

>+      MayFireChangeOnBlur() &&
>+      !mIsDraggingRange) { // StartRangeThumbDrag already set mFocusedValue
>+    GetValue(mFocusedValue);
>+  }

StartRangeThumbDrag isn't called in PreHandleEvent. Should that part be fixed too?
At leas the comment needs to be updated.


This all looks like a regression from Bug 722599.
Attachment #8756160 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8756160 [details] [diff] [review]
> patch, v1.
> 
> 
> >+  if (aVisitor.mEvent->mMessage == eFocus &&
> while you're here, want to check that the event is actually trusted.

Will do.

> 
> >+      MayFireChangeOnBlur() &&
> >+      !mIsDraggingRange) { // StartRangeThumbDrag already set mFocusedValue
> >+    GetValue(mFocusedValue);
> >+  }
> 
> StartRangeThumbDrag isn't called in PreHandleEvent. Should that part be
> fixed too?
> At leas the comment needs to be updated.

We still need to check the mIsDraggingRange flag since the order of the events are:
- preHandleEvent mousedown
- postHandleEvent mousedown -> StartRangeThumbDrag
- preHandlEvent focus -> now we set mFocusedValue here
- postHandleEvent focus

Will update the comment a bit to make it clearer. Thanks.

> 
> 
> This all looks like a regression from Bug 722599.
Whiteboard: btpp-active
Attached patch patch, v2. — — Splinter Review
Attachment #8756160 - Attachment is obsolete: true
Attachment #8757813 - Flags: review?(bugs)
Attachment #8757813 - Flags: review?(bugs) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e49cbb3a5b
focus and blur an input element should not trigger change event if content hasn't changed. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a6e49cbb3a5b
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1173341
You need to log in before you can comment on or make changes to this bug.