DOMActivate is fired twice for input[type=file] if user clicks on browse button

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
mozilla1.9.3a1
x86
Windows Vista
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5

If the button part of a file input control is clicked (or enter is pressed), the DOMActivate event is fired twice (presumably once for the child button control, and again for the parent input control). This does not occur if the text field is clicked on.

Reproducible: Always

Steps to Reproduce:
1. Load the testcase
2. Click the browse button
3. Choose a file
Actual Results:  
Two alerts boxes are posted, indicating the event is fired twice.

Expected Results:  
One alert box.
Assignee

Comment 1

10 years ago
Posted file test case
Assignee

Updated

10 years ago
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
smaug, any idea what's up here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
    We have two nested input elements (file and button) and
    nsHTMLInputElement::PostHandleEvent doesn't handle that case too well.
Assignee

Comment 4

10 years ago
Posted patch proposed patch (obsolete) — Splinter Review
Attachment #404490 - Flags: review?(Olli.Pettay)
Attachment #404490 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 404490 [details] [diff] [review]
proposed patch

>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -1709,16 +1709,27 @@ nsHTMLInputElement::PostHandleEvent(nsEv
>       // If activate is cancelled, we must do the same as when click is
>       // cancelled (revert the checkbox to its original value).
>       if (status == nsEventStatus_eConsumeNoDefault)
>         aVisitor.mEventStatus = status;
>     }
>   }
> 
>   if (outerActivateEvent) {
>+    // ignore the activate event fired by the "Browse..." button
>+    // (file input controls fire their own) (bug 500885)
>+    if (mType == NS_FORM_INPUT_FILE) {
>+      nsCOMPtr<nsIContent> maybeButton =
>+        do_QueryInterface(aVisitor.mEvent->originalTarget);
>+      if (maybeButton && maybeButton->AttrValueIs(kNameSpaceID_None,
>+                                                  nsGkAtoms::type,
>+                                                  nsGkAtoms::button,
>+                                                  eCaseMatters))
>+        return NS_OK;
>+    }
Add a check that maybeButton is in native anonymous content.
Assignee

Comment 6

10 years ago
I can't seem to make my patch do what it's supposed to do any more. Does it work for anybody else?
Assignee

Comment 7

10 years ago
Posted patch patch v2 (obsolete) — Splinter Review
I've moved this further up the function and outside the |if (outerActivateEvent)|, and it works again. Still don't know why the original patch stopped working but I'm sure there's a logical explanation.
Assignee: nobody → geoff
Attachment #404490 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #413583 - Flags: review?(Olli.Pettay)
Comment on attachment 413583 [details] [diff] [review]
patch v2

Could you write mochitest for this.
The test should check that one and only one DOMActivate is dispatched to <input type="file">
Attachment #413583 - Flags: review?(Olli.Pettay) → review-
And sorry, I should have asked a testcase already earlier.
Or please inform me if you don't have time to write the testcase, then I'll
do it.
Assignee

Comment 11

10 years ago
I was getting there! Really wish I'd decided that I didn't have time though... this took WAY too long :(
Attachment #413583 - Attachment is obsolete: true
Attachment #414414 - Flags: superreview?(jonas)
Attachment #414414 - Flags: review?(Olli.Pettay)
Comment on attachment 414414 [details] [diff] [review]
patch with tests

Def want smaugs review on this though.
Attachment #414414 - Flags: superreview?(jonas) → superreview+
Attachment #414414 - Flags: review?(Olli.Pettay) → review+
Assignee

Updated

10 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/0db0a2034733
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.