Last Comment Bug 722599 - Make the change event fired by the content instead of the layout
: Make the change event fired by the content instead of the layout
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
Depends on: 757694 786172 787102
Blocks: 743618
  Show dependency treegraph
 
Reported: 2012-01-30 18:44 PST by Yifan Mai
Modified: 2012-08-30 11:55 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (15.50 KB, patch)
2012-03-05 11:27 PST, Andrew Quartey [:drexler]
mounir: review-
Details | Diff | Review
Patch v2 (16.81 KB, patch)
2012-03-07 17:59 PST, Andrew Quartey [:drexler]
mounir: review-
Details | Diff | Review
Patch v3 (39.13 KB, patch)
2012-04-13 07:37 PDT, Andrew Quartey [:drexler]
mounir: review-
Details | Diff | Review
patch (28.82 KB, patch)
2012-04-21 08:58 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (28.73 KB, patch)
2012-04-27 15:13 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (32.78 KB, patch)
2012-05-03 12:14 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (32.65 KB, patch)
2012-05-03 12:53 PDT, Andrew Quartey [:drexler]
mounir: review+
Details | Diff | Review
updated_patch (32.17 KB, patch)
2012-05-04 11:24 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review

Description Yifan Mai 2012-01-30 18:44:18 PST
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.7 (KHTML, like Gecko) Chrome/16.0.912.75 Safari/535.7

Steps to reproduce:

If a user is editing a textbox, and the textbox is hidden while it still has focus, giving focus to another element does not cause the onchange event to fire. However the onblur event fires correctly.

Here's a jsfiddle setup with a textbox timed to hide in five seconds, and a console that indicates when the onchange event fires: http://jsfiddle.net/dugKE/12/

To reproduce bug:
1) Type something in the textbox
2) Wait five seconds
3) Click outside the textbox


Actual results:

No message indicating that the onchange event fired.


Expected results:

You should see a message indicating that the onchange event fired.
Comment 1 Josh Matthews [:jdm] 2012-01-30 19:20:18 PST
Presumably GetPrimaryFrame() at http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1893 returns null when the element is hidden. Skimming through the sections about change events in the spec, I didn't see anything that indicated that this is the correct behaviour, but I'm hardly an expert.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-31 05:20:40 PST
> Presumably GetPrimaryFrame() at http://mxr.mozilla.org/mozilla-central/source/content
> /html/content/src/nsHTMLInputElement.cpp#1893 returns null when the element is hidden. 

It would, yes.

Mounir and Ehsan, want to take a look?  This business of something having focus but no frame is a bit weird.
Comment 3 Mounir Lamouri (:mounir) 2012-01-31 05:54:17 PST
(In reply to Boris Zbarsky (:bz) from comment #2)
> > Presumably GetPrimaryFrame() at http://mxr.mozilla.org/mozilla-central/source/content
> > /html/content/src/nsHTMLInputElement.cpp#1893 returns null when the element is hidden. 
> 
> It would, yes.
> 
> Mounir and Ehsan, want to take a look?  This business of something having
> focus but no frame is a bit weird.

The change event is fired by the frame instead of the content. If there is no frame, there will be no change event. I guess it might be a good idea to move that logic to the content.
Comment 4 Neil Deakin 2012-01-31 06:53:25 PST
Also related is bug 570835 to remove focus when elements are hidden
Comment 5 :Ehsan Akhgari (out sick) 2012-01-31 07:07:06 PST
Yes, the entire process of firing the change event should move to content.  Note that this is also the case for other form controls.
Comment 6 Mounir Lamouri (:mounir) 2012-02-03 07:18:28 PST
Excepted a few cases, the change event handling for text fields is made by this method: 
nsTextControlFrame::CheckFireOnChange

To know more about it, you should have a look at:
layout/forms/nsTextControlFrame.h
layout/forms/nsTextControlFrame.cpp

If you look at <input> and <textarea> content implementation, you will see that the change event is fired by using the layout:
content/html/content/src/nsHTMLInputElement.cpp
content/html/content/src/nsHTMTextAreaElement.cpp

For exemple, line 1897 in nsHTMLInputElement::NeedToInitializeEditorForEvent.

What should be done is to be able to remove nsTextControlFrame::CheckFireOnChange and move the entire logic in nsHTMLInputElement and nsHTMLTextAreaElement.
Note that this might need some other changes like the calls from layout/forms/nsFileControlFrame.cpp
To have the list of all call of this method, have a look here: https://mxr.mozilla.org/mozilla-central/ident?i=CheckFireOnChange&filter=
Comment 7 Andrew Quartey [:drexler] 2012-03-05 11:27:55 PST
Created attachment 602985 [details] [diff] [review]
Patch v1
Comment 8 Mounir Lamouri (:mounir) 2012-03-07 08:11:36 PST
Comment on attachment 602985 [details] [diff] [review]
Patch v1

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

I think you should also remove |mFocusedValue| from nsTextControlFrame and by extent, |InitFocusedValue()|.

Also, I would name |FireChangeEvent| |FireChangeEventIfNeeded| instead because you actually don't fire the event if mFocusedValue is equal to the current value.

Finally, I would prefer the change event being fired by nsHTMLInputElement instead of nsFileControlFrame for <input type='file'>. I believe that might be easy to do actually.

Those are general comments about the patches. You can find more specific comments below.

And thank you for working on this, it is much appreciated! :)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1320,5 @@
> +  NS_ASSERTION(aContent, "The frame has no content???");
> +  nsString value;
> +  GetTextEditorValue(value, true);
> +  if (!mFocusedValue.Equals(value))
> +  {

nit: the coding style requires this:
if (condition) {
  something();
}

@@ +1654,5 @@
>    if (mType != NS_FORM_INPUT_FILE) {
>      return nsGenericHTMLElement::Focus();
>    }
>  
> +  // For file inputs, if text control initialize value upon focus.

"... initialize mFocusedValue upon focus."

@@ +1665,5 @@
>        nsCOMPtr<nsIFormControl> formCtrl =
>          do_QueryInterface(childFrame->GetContent());
> +      if (formCtrl) {
> +        if (formCtrl->GetType() == NS_FORM_INPUT_TEXT) {
> +          GetTextEditorValue(mFocusedValue, true);

I think you should put those changes in PreHandleEvent or PostHandleEvent (likely).

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +119,5 @@
>    }
>    NS_SCRIPTABLE NS_IMETHOD GetTabIndex(PRInt32* aTabIndex);
>    NS_SCRIPTABLE NS_IMETHOD SetTabIndex(PRInt32 aTabIndex);
>    NS_SCRIPTABLE NS_IMETHOD Focus() {
> +    GetValue(mFocusedValue);

This is only called when .focus() is called from Javascript. You want to add this to PreHandleEvent or PostHandleEvent.
Comment 9 Andrew Quartey [:drexler] 2012-03-07 17:59:32 PST
Created attachment 603935 [details] [diff] [review]
Patch v2

> @@ +1665,5 @@
> >        nsCOMPtr<nsIFormControl> formCtrl =
> >          do_QueryInterface(childFrame->GetContent());
> > +      if (formCtrl) {
> > +        if (formCtrl->GetType() == NS_FORM_INPUT_TEXT) {
> > +          GetTextEditorValue(mFocusedValue, true);
> 
> I think you should put those changes in PreHandleEvent or PostHandleEvent
> (likely).
> 
> ::: content/html/content/src/nsHTMLTextAreaElement.cpp
> @@ +119,5 @@
> >    }
> >    NS_SCRIPTABLE NS_IMETHOD GetTabIndex(PRInt32* aTabIndex);
> >    NS_SCRIPTABLE NS_IMETHOD SetTabIndex(PRInt32 aTabIndex);
> >    NS_SCRIPTABLE NS_IMETHOD Focus() {
> > +    GetValue(mFocusedValue);
> 
> This is only called when .focus() is called from Javascript. You want to add
> this to PreHandleEvent or PostHandleEvent.

Looking back, i think these were superfluous, since prior to firing the change event, mFocusedValue is updated to the current editor's value. Hence, the removal.
Comment 10 Mounir Lamouri (:mounir) 2012-03-08 00:54:42 PST
Comment on attachment 603935 [details] [diff] [review]
Patch v2

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

You didn't change nsFileControlFrame parts. I don't think the frame of <input type='file'> should handle the change event. We should do that in the content (nsHTMLInputElement) like for other input elements.

Also, |mFocusedValue| should be set when focusing the element. I don't think your patch is handling that correctly.

Actually, I think you might want to write tests for the change event so you will be able to see if you break anything. There might be existing tests but it is unlikely they are testing everything.
You will need to write mochitests as explained here: https://developer.mozilla.org/en/Mochitest
You could create the test file here: content/html/content/test/forms/test_change_event.html

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +259,5 @@
>    bool                     mCanShowInvalidUI;
>    /** Whether we should make :-moz-ui-valid apply on the element. **/
>    bool                     mCanShowValidUI;
> +  
> +  void FireChangeEventIfNeeded(nsIContent* aContent);

Actually, I realize you don't need the parameter given that you are passing |this|...

@@ +748,5 @@
> +  NS_ASSERTION(aContent, "The frame has no content???");
> +  nsString value;
> +  GetValue(value);
> +  if (!mFocusedValue.Equals(value))
> +  {

nit: coding style
Comment 11 Andrew Quartey [:drexler] 2012-04-13 07:37:41 PDT
Created attachment 614775 [details] [diff] [review]
Patch v3

Moved both event handling from nsFileControlFrame to the input element.  

> Actually, I think you might want to write tests for the change event so you
> will be able to see if you break anything. There might be existing tests but
> it is unlikely they are testing everything.

I think test_bug592802.html has sufficient coverage for  the change event firing for <input type='file'>. I might have to write some tests for the CapturePicker but unsure of how to proceed at this point.
Comment 12 Mounir Lamouri (:mounir) 2012-04-16 07:17:28 PDT
Comment on attachment 614775 [details] [diff] [review]
Patch v3

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

The change in nsFileControlFrame isn't what I meant. You should keep UI events there. What you should do is to send the change events from the content. You could probably just call FireChangeEventIfNeeded() but I'm not sure that would work.

I think we should discuss this patch a bit deeper before you attach a new version. We could do that in this bug, or via email/Skype/anything.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1333,5 @@
>  
> +void
> +nsHTMLInputElement::FireChangeEventIfNeeded()
> +{
> +  nsIContent* content = static_cast<nsIContent*>(this);

See my comment in nsTextAreaElement.cpp

@@ +1339,5 @@
> +  GetTextEditorValue(value, true);
> +  if (!mFocusedValue.Equals(value)) {
> +    mFocusedValue = value;
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,

See my comment in nsTextAreaElement.cpp

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +117,5 @@
>    }
>    NS_SCRIPTABLE NS_IMETHOD GetTabIndex(PRInt32* aTabIndex);
>    NS_SCRIPTABLE NS_IMETHOD SetTabIndex(PRInt32 aTabIndex);
>    NS_SCRIPTABLE NS_IMETHOD Focus() {
> +		GetValue(mFocusedValue);

Why do you do that?

@@ +591,5 @@
>  NS_IMETHODIMP 
>  nsHTMLTextAreaElement::SetValue(const nsAString& aValue)
>  {
> +  SetValueInternal(aValue, false);
> +	GetValue(mFocusedValue);

ditto

@@ +747,5 @@
> +void
> +nsHTMLTextAreaElement::FireChangeEventIfNeeded()
> +{
> +  nsIContent* content = static_cast<nsIContent*>(this);
> +  NS_ASSERTION(content, "The frame has no content???");

You don't need that, nsHTMLTextAreaElement inherits from nsIContent, you can simple use the methods from nsIContent.

@@ +754,5 @@
> +  if (!mFocusedValue.Equals(value))
> +  { 
> +    mFocusedValue = value;
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,

Should be:
nsContentUtils::DispatchTrustedEvent(OwnerDoc(), this,

::: layout/forms/nsTextControlFrame.h
@@ -197,4 @@
>  
>    void SetFireChangeEventState(bool aNewState)
>    {
>      mFireChangeEventState = aNewState;

I think you could remove SetFireChangeEventState, GetFireChangeEventState and mFireChangeEventState.

@@ -220,5 @@
>        // from nsFileControlFrame which sets mFireChangeEventState==true and
>        // restores it afterwards (ie. we want 'change' events for those changes).
>        // Focused value must be updated to prevent incorrect 'change' events,
>        // but only if user hasn't changed the value.
> -      , mFocusValueInit(!mFrame->mFireChangeEventState && aHasFocusValue)

nit: you should remove the comment above too. And make sure your patch take that into account.

@@ -253,5 @@
>  
>    private:
>      nsTextControlFrame* mFrame;
>      nsCOMPtr<nsIEditor> mEditor;
>      bool mFocusValueInit;

I guess you could remove this variable too?
Comment 13 :Ms2ger 2012-04-21 04:31:36 PDT
Comment on attachment 614775 [details] [diff] [review]
Patch v3

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

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1020,5 @@
>      }
>    }
>    else {
>      SetValueInternal(aValue, false, true);
> +		GetTextEditorValue(mFocusedValue,true);

Hard tabs
Comment 14 Andrew Quartey [:drexler] 2012-04-21 08:58:38 PDT
Created attachment 617230 [details] [diff] [review]
patch

I needed GetValueInternal(mFocusedValue); in nsHTMLInputElement::SetValue to ensure correct behavior since we longer initialize mFocusedValue via nsTextConrolFrame::ValueSetter. In addition, i added tests to simulate a user focusing on the control, blurring it and refocusing on a dummy control; just as the jsfiddle test in comment 0.
Comment 15 Mounir Lamouri (:mounir) 2012-04-26 04:13:13 PDT
Comment on attachment 617230 [details] [diff] [review]
patch

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

It seems like you are saving the mFocusedValue on blur for <input> and on focus for <textarea>. Why? You should probably try to be consistent here.

Sorry if it took so long to make that review, I had serious hardware issues :-/

Your patch is very good, I hope you will not be discouraged by all those comments. Keep in mind that this patch would fix quite some bugs related to firing the change event! ;)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1330,5 @@
>  
> +void
> +nsHTMLInputElement::FireChangeEventIfNeeded(nsIContent* aContent)
> +{
> +  nsIContent* content = aContent ? aContent : static_cast<nsIContent*>(this);

No need for that.

@@ +1336,5 @@
> +  GetTextEditorValue(value, true);
> +  if (!mFocusedValue.Equals(value)) {
> +    mFocusedValue = value;
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content, 

...(OwnerDoc(), static_cast<nsIContent*>(this),

@@ +1339,5 @@
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content, 
> +                                       NS_LITERAL_STRING("change"), true, 
> +                                       false);
> +	}

You should fix the indentation.

@@ +2211,5 @@
>            if (aVisitor.mEvent->message == NS_KEY_PRESS &&
>                (keyEvent->keyCode == NS_VK_RETURN ||
>                 keyEvent->keyCode == NS_VK_ENTER) &&
>                 IsSingleLineTextControl(false, mType)) {
> +            FireChangeEventIfNeeded();

Could you check that we have tests for that? Just comment that call and see if we have failing test. If not, could you write one? :)

::: content/html/content/src/nsHTMLInputElement.h
@@ +301,5 @@
>      return HasAttr(kNameSpaceID_None, nsGkAtoms::checked);
> +  }
> +
> +  /**
> +  * Fires onchange event.

This comment is wrong. It will fire the event only if mFocusedValue and the current value are different.

@@ +304,5 @@
> +  /**
> +  * Fires onchange event.
> +  */
> +  void FireChangeEventIfNeeded(nsIContent* aContent = 0);
> +  void InitFocusedValue();

I think you could remove that Init...() method, see nsFileControlFrame.cpp comments.

@@ +589,5 @@
>    nsRefPtr<nsDOMFileList>  mFileList;
>  
>    nsString mStaticDocFileList;
>  
> +  nsString mFocusedValue;

Could you add a comment explaining what this variable is for.

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +753,5 @@
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
> +                      static_cast<nsIContent*>(this),
> +                      NS_LITERAL_STRING("change"), true, false);
> +	}

You have indentation/coding style issues here.
It should be:
if (foo) {
  mFocusedValue = value;
  nsContentUtils::Dispatch..(OwnerDoc(),
                             static_cast<...>(this),
                             NS_LITERAL(...), true, false));
}

::: content/html/content/test/test_bug722599.html
@@ +16,5 @@
> +  <input type="text" id="input" onchange="++inputChange;"></input>
> +  <input type="file" id="fileInput"></input>
> +  <textarea id="textarea" onchange="++textareaChange;"></textarea>
> +  <button id="button">dummy button</button>
> +  <button id="filePickerButton" onclick="document.getElementById('fileInput').click();">File Picker</button>

Do you really need that? Can't you simply call .click() on the <input type=file>?

@@ +32,5 @@
> +    MockFilePicker.init();
> +
> +    function testUserInput() {
> +  
> +      //simulating an OK click and with a file name return

nit: // Simulating ... name return.

@@ +48,5 @@
> +      }, false);
> +
> +      btn.click();
> +
> +      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

This is now forbidden in mochitests AFAIK. Using synthesizeKey as suggested below should remove the need for that.

@@ +53,5 @@
> +      var input = document.getElementById("input");
> +      var textarea = document.getElementById("textarea");
> +
> +      input.focus();
> +      input.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).setUserInput("foo");

Please use synthesizeKey from EventUtils instead. You can just type one letter to make it simpler.

@@ +55,5 @@
> +
> +      input.focus();
> +      input.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).setUserInput("foo");
> +      input.blur();
> +      button.focus();  // forced focus switch

I don't think you need .focus() to be called, do you?

@@ +59,5 @@
> +      button.focus();  // forced focus switch
> +      is(inputChange, 1, "Input element should have dispatched change event.");
> +
> +      textarea.focus();
> +      textarea.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).setUserInput("bar");

Please use synthesizeKey from EventUtils instead. You can just type one letter to make it simpler.

@@ +61,5 @@
> +
> +      textarea.focus();
> +      textarea.QueryInterface(Components.interfaces.nsIDOMNSEditableElement).setUserInput("bar");
> +      textarea.blur();
> +      button.focus();

I don't think you need .focus() to be called, do you?

::: layout/forms/nsFileControlFrame.cpp
@@ +374,5 @@
>    rv = capturePicker->Init(win, title, mMode);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Tell our input element to remember the currently focused value.
> +  inputElement->InitFocusedValue();

I think we could remove that method.
Given that nsHTMLInputElement::mFocusedValue is updated when the element is blurred, and the value of the element is updated later (with ->SetFiles()), the FireChangeEventIfNeeded() call should be enough, right?

@@ +416,2 @@
>      // May need to fire an onchange here
> +    inputElement->FireChangeEventIfNeeded(mContent);

It's not "may need to". This will fire an change event given that the file comes from a capture, it shouldn't be the same as before.

::: layout/forms/nsFileControlFrame.h
@@ +114,5 @@
>      NS_DECL_ISUPPORTS
>      
> +    MouseListener(nsFileControlFrame* aFrame, nsIContent* aContent)
> +     : mFrame(aFrame),
> +       mContent(aContent) {}

mFrame->GetContent() would return the content, no need to take it as a parameter.

@@ +150,5 @@
>    class CaptureMouseListener: public MouseListener {
>    public:
> +    CaptureMouseListener(nsFileControlFrame* aFrame, nsIContent* aContent) 
> +      : MouseListener(aFrame, aContent),
> +        mMode(0) {};

ditto

@@ +159,5 @@
>    
>    class BrowseMouseListener: public MouseListener {
>    public:
> +    BrowseMouseListener(nsFileControlFrame* aFrame, nsIContent* aContent) 
> +      : MouseListener(aFrame, aContent) {};

ditto
Comment 16 Andrew Quartey [:drexler] 2012-04-27 09:18:51 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #15)
> Comment on attachment 617230 [details] [diff] [review]

> 
> It seems like you are saving the mFocusedValue on blur for <input> and on
> focus for <textarea>. Why? You should probably try to be consistent here.
> 
Oops that was a "copy code and paste" typo. Completely unintentional. I have fixed most of it and sent it to try. 

>            if (aVisitor.mEvent->message == NS_KEY_PRESS &&
>                (keyEvent->keyCode == NS_VK_RETURN ||
>                 keyEvent->keyCode == NS_VK_ENTER) &&
>                 IsSingleLineTextControl(false, mType)) {
> +            FireChangeEventIfNeeded();

> Could you check that we have tests for that? Just comment that call and see if we >have failing test. If not, could you write one? :)

There's isn't any covering test as far i can tell from the try results. I will append a quickie test for this.
Comment 17 Andrew Quartey [:drexler] 2012-04-27 15:13:32 PDT
Created attachment 619191 [details] [diff] [review]
patch

Cleaned up the indentation and added extra tests for text, email, search, telephone, url & password input elements.
Comment 18 Mounir Lamouri (:mounir) 2012-05-02 08:46:26 PDT
Comment on attachment 619191 [details] [diff] [review]
patch

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

There is an issue with HTMLInputElement if they are not text fields. For example, I think changing the <input type=radio> value would fire two change events with this patch: one when the value is changed and one on blur. This is not what we want.
Could you add tests checking that for some input types (file, radio, checkbox), we do not fire an event on blur but only *when* the change happens).

Also, I wonder if we could add an argument to SetFiles() that says if a change event should be fired. As far as I can see, three calls of that methods wants a change event to be fired and only one doesn't want that. This could be done in a follow-up though.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1020,5 @@
>      }
>    }
>    else {
>      SetValueInternal(aValue, false, true);
> +    GetValueInternal(mFocusedValue);

Sorry, I just think about that but we only want to do that for text controls?

Maybe you could do:
if (IsSingleLineTextControl(false)) {
  GetValueInternal(mFocusedValue);
}

@@ +1332,5 @@
>  }
>  
> +void
> +nsHTMLInputElement::FireChangeEventIfNeeded()
> +{

I think you should have an early return if it's not a single line text control:
if (!IsSingleLineTextControl(false)) {
  return;
}

@@ +1334,5 @@
> +void
> +nsHTMLInputElement::FireChangeEventIfNeeded()
> +{
> +  nsString value;
> +  GetTextEditorValue(value, true);

Why do you want to use GetTextEditorValue() instead of GetValueInternal()?

@@ +1335,5 @@
> +nsHTMLInputElement::FireChangeEventIfNeeded()
> +{
> +  nsString value;
> +  GetTextEditorValue(value, true);
> +  if (!mFocusedValue.Equals(value)) {

nit: you could do an early return as explained for nsHTMLTextAreaElement.cpp

@@ +1340,5 @@
> +    mFocusedValue = value;
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
> +                       static_cast<nsIContent*>(this), 
> +                       NS_LITERAL_STRING("change"), true, false);

nit: like said for nsHTMLTextAreaElement.cpp, lines should be indented with |OwnerDoc()|.

::: content/html/content/src/nsHTMLInputElement.h
@@ +304,5 @@
> +  }
> +
> +  /**
> +  * Fires onchange event if mFocusedValue and current value held are unequal
> +  */

nit: style issue:
/**
 * Your comment.
 */

In addition s/onchange/change/ and the sentence should end with a ".".

@@ +590,5 @@
>    nsRefPtr<nsDOMFileList>  mFileList;
>  
>    nsString mStaticDocFileList;
> +  
> +  /*

nit: /**

@@ +592,5 @@
>    nsString mStaticDocFileList;
> +  
> +  /*
> +   * The value of the input element when first initialized and it is updated
> +   * when the element is either focused or dispatches a change event.  

It is also updated when the value is changed trough a script, right?

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +749,5 @@
> +nsHTMLTextAreaElement::FireChangeEventIfNeeded()
> +{
> +  nsString value;
> +  GetValue(value);
> +  if (!mFocusedValue.Equals(value)) {

nit: you could do:
if (mFocusedValue.Equals(value)) {
  return;
}

Early return can be more readable and will make you win a level of indentation.

@@ +754,5 @@
> +    mFocusedValue = value;
> +    // Dispatch the change event.
> +    nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
> +                      static_cast<nsIContent*>(this),
> +                      NS_LITERAL_STRING("change"), true, false);

The indentation isn't correct here: everything should be aligned with |OwnerDoc()|.

::: content/html/content/test/test_bug722599.html
@@ +76,5 @@
> +      is(inputChange[0], 2, "text input element should have dispatched change event (2).");
> +
> +      input.focus();
> +      synthesizeKey("VK_ENTER", {});
> +      is(inputChange[0], 2, "Change event shouldn't be dispatched on text input element(2)");

Aren't you testing this a above in the for loop?

@@ +80,5 @@
> +      is(inputChange[0], 2, "Change event shouldn't be dispatched on text input element(2)");
> +
> +      synthesizeKey("b", {});
> +      synthesizeKey("VK_RETURN", {});
> +      is(inputChange[0], 3, "text input element should have dispatched change event (3).");

ditto

::: layout/forms/nsFileControlFrame.cpp
@@ +411,3 @@
>      inputElement->SetFiles(newFiles, true);
> +    
> +    // Should fire an onchange here

nit: Make this comment a bit more verbose like:
"Should fire a change event here because SetFiles() above have change the value of |inputElement| and it shouldn't be the same as the previous value given that it comes from the camera picker."
(note that "onchange" is the change event handler, not the event name)

::: layout/forms/nsFileControlFrame.h
@@ +148,5 @@
>    class CaptureMouseListener: public MouseListener {
>    public:
> +    CaptureMouseListener(nsFileControlFrame* aFrame) 
> +      : MouseListener(aFrame),
> +        mMode(0) {};

nit: as long as you change the indentation, could you do:
CaptureMouseListener(nsFileControlFrame* aFrame)
  : MouseListener(aFrame)
  , mMode(0)
{}

@@ +157,5 @@
>    
>    class BrowseMouseListener: public MouseListener {
>    public:
> +    BrowseMouseListener(nsFileControlFrame* aFrame) 
> +      : MouseListener(aFrame) {};

nit: as long as you change the indentation could you do:
BrowseMouseListener(nsFileControlFrame* aFrame)
  : MouseListener(aFrame)
{}
Comment 19 Andrew Quartey [:drexler] 2012-05-02 12:36:33 PDT
> There is an issue with HTMLInputElement if they are not text fields. For
> example, I think changing the <input type=radio> value would fire two change
> events with this patch: one when the value is changed and one on blur. This
> is not what we want.

I think the only change required will be to use GetTextEditorValue() instead of GetValueInternal in setting the mFocusedValue, since GetTextEditorValue in turn calls GetEditorState() which performs the necessary test for a text field input. Therefore, adding that extra check and early return in FireChangeEventIfNeeded shouldn't be needed right?
Comment 20 Mounir Lamouri (:mounir) 2012-05-03 03:33:00 PDT
(In reply to Andrew Quartey [:drexler] from comment #19)
> > There is an issue with HTMLInputElement if they are not text fields. For
> > example, I think changing the <input type=radio> value would fire two change
> > events with this patch: one when the value is changed and one on blur. This
> > is not what we want.
> 
> I think the only change required will be to use GetTextEditorValue() instead
> of GetValueInternal in setting the mFocusedValue, since GetTextEditorValue
> in turn calls GetEditorState() which performs the necessary test for a text
> field input. Therefore, adding that extra check and early return in
> FireChangeEventIfNeeded shouldn't be needed right?

I would prefer to be explicit and do an early return. That would prevent any ambiguity, doesn't make this code depends on other method behavior and isn't worse for performance (even better because we prevent the function call).

BTW, that let me think of something: what do we want to do if the input element's type changes? |mFocusedValue| should probably be updated, I guess...
Comment 21 Andrew Quartey [:drexler] 2012-05-03 12:14:47 PDT
Created attachment 620808 [details] [diff] [review]
patch

Made changes per comment 20 and added tests for non-text control input element types and also changes on type.
Comment 22 Andrew Quartey [:drexler] 2012-05-03 12:53:00 PDT
Created attachment 620821 [details] [diff] [review]
patch

Indentation fixes.
Comment 23 Mounir Lamouri (:mounir) 2012-05-04 05:37:29 PDT
Comment on attachment 620821 [details] [diff] [review]
patch

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

r=me with the comments addressed.

Let me know if you need me to push the patch to try or mozilla-inbound.

Awesome work Andrew! :)

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +605,5 @@
>    } else {
>      UnbindFromFrame(nsnull);
>      delete mInputData.mState;
>      mInputData.mState = nsnull;
> +    mFocusedValue.SetIsVoid(true);

I believe you want .Truncate() here. |SetIsVoid| is to make the string equals |null| in JS.

@@ +1338,5 @@
> +nsHTMLInputElement::FireChangeEventIfNeeded()
> +{
> +  nsString value;
> +  GetValueInternal(value);
> +  if (!IsSingleLineTextControl(false) || mFocusedValue.Equals(value)) {

nit: Leave an blank line before the |if ()|.

@@ +1341,5 @@
> +  GetValueInternal(value);
> +  if (!IsSingleLineTextControl(false) || mFocusedValue.Equals(value)) {
> +    return;
> +  }
> +  // Dispatch the change event.

nit: Leave a blank line before the comment.

@@ +1969,3 @@
>  
>      UpdateValidityUIBits(aVisitor.mEvent->message == NS_FOCUS_CONTENT);
> +    

nit: are you adding spaces here? If so, could you remove them?

@@ +2444,5 @@
>            }
>            SetValueInternal(value, false, false);
> +          if (isNewTypeSingleLine) {
> +            GetValueInternal(mFocusedValue);
> +          }

I would prefer a block like this after the switch block:

// Updating mFocusedValue in consequence.
if (isNewTypeSingleLine && !isCurrentTypeSingleLine) {
  GetValueInternal(mFocusedValue);
} else if (!isNewTypeSingleLine && isCurrentTypeSingleLine) {
  mFocusedValue.Truncate();
}

::: content/html/content/src/nsHTMLInputElement.h
@@ +595,5 @@
> +   * The value of the input element when first initialized and it is updated
> +   * when the element is either changed through a script, focused or dispatches   
> +   * a change event. This is to ensure correct future change event firing.
> +   * NB: This is ONLY applicable where the element is a text control. ie,
> +   * where <input type= "text", "email", "search", "tel", "url" or "password".

nit: remove "<input "

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +749,5 @@
> +nsHTMLTextAreaElement::FireChangeEventIfNeeded()
> +{
> +  nsString value;
> +  GetValueInternal(value, true);
> +  if (mFocusedValue.Equals(value)) {

nit: Could you add a blank line before the if ()?

@@ +752,5 @@
> +  GetValueInternal(value, true);
> +  if (mFocusedValue.Equals(value)) {
> +    return;
> +  }
> +  // Dispatch the change event.

nit: Could you add a blank like before the comment.

::: content/html/content/test/Makefile.in
@@ +276,5 @@
>  		test_bug677658.html \
>  		test_bug677463.html \
>  		test_bug682886.html \
>  		test_bug717819.html \
> +		test_bug722599.html \

Could you move the test file to content/html/content/test/forms/ and name it "test_change_event.html"

::: content/html/content/test/test_bug722599.html
@@ +70,5 @@
> +        is(fileInputChange, 1, "change event should have been dispatched on file input.");
> +        blurTestCalled = true;
> +        fileInputBlurTest();
> +      }
> +	  else {

nit: indentation issue

::: layout/forms/nsFileControlFrame.h
@@ -160,2 @@
>      NS_DECL_NSIDOMEVENTLISTENER
> -

nit: you don't have to remove that line
Comment 24 Andrew Quartey [:drexler] 2012-05-04 11:24:20 PDT
Created attachment 621112 [details] [diff] [review]
updated_patch

Nits amendments and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c688626646f4
Comment 25 Mounir Lamouri (:mounir) 2012-05-07 08:48:43 PDT
Comment on attachment 621112 [details] [diff] [review]
updated_patch

No need to re-ask a review the reviewer says "r+ with <something>" (unless you changed other things).
Comment 26 Mounir Lamouri (:mounir) 2012-05-07 08:51:29 PDT
I will push that later today but if someone wants to do it before, feel free.
Comment 27 :Ehsan Akhgari (out sick) 2012-05-07 09:56:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba79daeeb874
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-05-07 18:01:59 PDT
http://hg.mozilla.org/mozilla-central/rev/ba79daeeb874

Note You need to log in before you can comment on or make changes to this bug.