Make the change event fired by the content instead of the layout

RESOLVED FIXED in mozilla15

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yifan Mai, Assigned: drexler)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mounir][lang=c++])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
> 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.
Component: DOM: Events → Layout: Form Controls
QA Contact: events → layout.form-controls
(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.
Also related is bug 570835 to remove focus when elements are hidden
Yes, the entire process of firing the change event should move to content.  Note that this is also the case for other form controls.
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=
Summary: onchange event does not fire when input element is hidden → Make the change event fired by the content instead of the layout
Whiteboard: [mentor=mounir][lang=C++]
Version: unspecified → Trunk

Updated

6 years ago
Whiteboard: [mentor=mounir][lang=C++] → [mentor=mounir][lang=c++]
(Assignee)

Updated

5 years ago
Assignee: nobody → andrew.quartey
(Assignee)

Comment 7

5 years ago
Created attachment 602985 [details] [diff] [review]
Patch v1
Attachment #602985 - Flags: review?(mounir)
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.
Attachment #602985 - Flags: review?(mounir) → review-
(Assignee)

Comment 9

5 years ago
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.
Attachment #602985 - Attachment is obsolete: true
Attachment #603935 - Flags: review?(mounir)
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
Attachment #603935 - Flags: review?(mounir) → review-
Blocks: 743618
(Assignee)

Comment 11

5 years ago
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.
Attachment #603935 - Attachment is obsolete: true
Attachment #614775 - Flags: review?(mounir)
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?
Attachment #614775 - Flags: review?(mounir) → review-
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
(Assignee)

Comment 14

5 years ago
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.
Attachment #614775 - Attachment is obsolete: true
Attachment #617230 - Flags: review?(mounir)
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
Attachment #617230 - Flags: review?(mounir)
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
Created attachment 619191 [details] [diff] [review]
patch

Cleaned up the indentation and added extra tests for text, email, search, telephone, url & password input elements.
Attachment #617230 - Attachment is obsolete: true
Attachment #619191 - Flags: review?(mounir)
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)
{}
Attachment #619191 - Flags: review?(mounir)
(Assignee)

Comment 19

5 years ago
> 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?
(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...
(Assignee)

Comment 21

5 years ago
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.
Attachment #619191 - Attachment is obsolete: true
Attachment #620808 - Flags: review?(mounir)
(Assignee)

Comment 22

5 years ago
Created attachment 620821 [details] [diff] [review]
patch

Indentation fixes.
Attachment #620808 - Attachment is obsolete: true
Attachment #620808 - Flags: review?(mounir)
Attachment #620821 - Flags: review?(mounir)
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
Attachment #620821 - Flags: review?(mounir) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 621112 [details] [diff] [review]
updated_patch

Nits amendments and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c688626646f4
Attachment #621112 - Flags: review?(mounir)
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).
Attachment #621112 - Flags: review?(mounir)
I will push that later today but if someone wants to do it before, feel free.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba79daeeb874
Keywords: checkin-needed
Target Milestone: --- → mozilla15
http://hg.mozilla.org/mozilla-central/rev/ba79daeeb874
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 757694

Updated

5 years ago
Depends on: 786172

Updated

5 years ago
Depends on: 787102
You need to log in before you can comment on or make changes to this bug.