928224 - First Character Being Eaten during Type Change on Input Field

Status: NEW

(Core :: DOM: Forms, defect, P3)

Description

[agaripian]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

Use the url below to type in a username, use the tab key to switch to the input field and type in a password.

http://jsbin.com/uVAzenE/5/edit?html,js,output


Actual results:

The first character in the password field was eaten. It never made it to the input field.


Expected results:

The first character should have appeared in the input field.
This used to work fine in older versions of FireFox such as version 17.0.1

Comment 1

[mjh563]

The regression range is

Last good nightly: 2013-02-13
First bad nightly: 2013-02-14

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=161a347bda5b&tochange=aceeea086ccb

Google Chrome (v30) seems to do the same thing, so this might not be a valid bug.

Comment 2

[mjh563]

Attachment: Simplified testcase

Comment 3

[agaripian]

Google Chrome V30+ only has this issue. The issue did not exist in v29 and below. Why did it change? It was working in the past.

Comment 4

[mjh563]

The first bad revision is:
changeset:   121726:25ae6325380b
user:        Mounir Lamouri
date:        Tue Feb 12 20:16:58 2013 +0000
summary:     Bug 665655 - Make mInputData.mValue really used and usable. r=bz

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Handle input type change correctly

From what I understand, the problem is in HTMLInputElement::HandleTypeChange

  if (IsSingleLineTextControl()) {
    mInputData.mState = new nsTextEditorState(this);
  }

this code is resetting the Editor's state every time, not just when needed as it was before. This (probably) causes the onkeypress event to not complete and never adding the character. Reverting this part of the changes fixes this bug, from the discussion it doesn't seem there is a reason to do this every time (not just when needed). Let me know if actually there is.

Side note, I couldn't reproduce the simple testcase on the nightly build, but the link in the first comment is still showing the bug.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 8388217 [details] [diff] [review]
Handle input type change correctly

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

jwatt, could you have a look at that?

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 8388217 [details] [diff] [review]
Handle input type change correctly

Feel free to add me later as a reviewer.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Mounir, do you have time to look at my patch now? It looks like jwatt is away. I'll post a mochitest later this week. Thanks!

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 8388217 [details] [diff] [review]
Handle input type change correctly

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

It looks okay but could you write a test for that please?

::: content/html/content/src/HTMLInputElement.cpp
@@ +4418,5 @@
>    if (aOldValueMode == VALUE_MODE_VALUE) {
>      GetValue(aOldValue);
>    }
>  
> +  // Only single line text inputs have a text editor state.

Hmm, what about this:
// Only single line text inputs have a text editor state but we want to make sure we have it correctly cleaned or set-up when needed.

@@ +4420,5 @@
>    }
>  
> +  // Only single line text inputs have a text editor state.
> +  bool isNewTypeSingleLine = IsSingleLineTextControl(PR_FALSE, aNewType);
> +  bool isCurrentTypeSingleLine = IsSingleLineTextControl(PR_FALSE, mType);

s/PR_FALSE/false/

@@ +4427,5 @@
> +    FreeData();
> +    mInputData.mState = new nsTextEditorState(this);
> +  } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
> +    FreeData();
> +  }

I think this is pretty hard to read. What about this:
if (IsSingleTextControl(false, aNewType) != IsSingleTextControl()) {
  FreeData();

  if (IsSingleTextControl(false, aNewType)) {
    mInputData.mState = new nsTextEditorState(this);
  }
}

@@ +4428,5 @@
> +    mInputData.mState = new nsTextEditorState(this);
> +  } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
> +    FreeData();
> +  }
> + 

nit: trailing whitespace.

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Handle input type change correctly

Thanks for taking the time Mounir. I added a mochitest and pushed to Try servers here: https://tbpl.mozilla.org/?tree=Try&rev=cc743a1a9cbe

It looks like only e10s is failing the test. I'm guessing is a dependency from another bug, but I haven't really investigated it.

Mounir, do you think we can push it to m-c disabling the test on e10s? Or else I can look more into it.

Thanks!

Comment 11

[Mounir Lamouri (:mounir)]

Dropping needinfo? request because it was two years ago. If you still want to hear from me, please add the flag back.

Comment 12

[Robert]

If this issue is available I'd take it, if anyone sees this could you please assign me so I can find this bug more easily again later

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 14

[Jai Sai Chand Tumu]

If this issue if available I'd take it.. if anyone sees this could you please assign me if no one else is working on this. I am very interested in this

Comment 15

[Richard Cheng]

Can I take this issue and try to work on it?
This is my first time trying to contribute to firefox nightly

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Jai Sai Chand Tumu and Richard Cheng,
I am sorry that I didn't notice your comments earlier. The issue is still open, and we welcome and appreciate your contribution.

If you are still available and interested in taking this bug, please feel free to assign this to yourself, or Request information (needinfo) from me that I can assign to you.
If you are looking for other good-first-bugs, we hope this page could be useful to you.

Comment 17

[Jai Sai Chand Tumu]

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #16)

Hi Jai Sai Chand Tumu and Richard Cheng,
I am sorry that I didn't notice your comments earlier. The issue is still open, and we welcome and appreciate your contribution.

If you are still available and interested in taking this bug, please feel free to assign this to yourself, or Request information (needinfo) from me that I can assign to you.
If you are looking for other good-first-bugs, we hope this page could be useful to you.

I wanna work on this.. I might reach out to you on mail if I need help as I have never fixed any bugs in firefox and this is my first bug

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jai Sai Chand Tumu from comment #17)

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #16)

Hi Jai Sai Chand Tumu and Richard Cheng,
I am sorry that I didn't notice your comments earlier. The issue is still open, and we welcome and appreciate your contribution.

If you are still available and interested in taking this bug, please feel free to assign this to yourself, or Request information (needinfo) from me that I can assign to you.
If you are looking for other good-first-bugs, we hope this page could be useful to you.

I wanna work on this.. I might reach out to you on mail if I need help as I have never fixed any bugs in firefox and this is my first bug

Hi! Thank you for the interest. I've assigned this bug to you. :)
Please feel free to reach out to me if you have any questions; or find our team at Matrix

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

To recap the next steps for resolving this issue per the chat with :edgar -
generally comment 5 is still the right summary for this issue. This WIP patch points to the right direction. Who plans to fix this may want to start with looking into the code of HTMLInputElement::HandleTypeChange and update the code of calling IsSingleLineTextControl() in that class as what the WIP patch did.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.