Open Bug 928224 Opened 8 years ago Updated 7 months ago

First Character Being Eaten during Type Change on Input Field

Categories

(Core :: DOM: Forms, defect)

defect

Tracking

()

People

(Reporter: agaripian, Unassigned)

References

()

Details

(4 keywords)

Attachments

(2 files, 1 obsolete file)

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
Severity: normal → major
Priority: -- → P3
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.
Attached file Simplified testcase
Keywords: regression, testcase
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.
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
Blocks: 665655
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Product: Firefox → Core
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.
Attachment #8388217 - Flags: review?(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?
Attachment #8388217 - Flags: review?(jwatt)
Comment on attachment 8388217 [details] [diff] [review]
Handle input type change correctly

Feel free to add me later as a reviewer.
Attachment #8388217 - Flags: review?(mounir)
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!
Assignee: nobody → agi.novanta
Status: NEW → ASSIGNED
Flags: needinfo?(mounir)
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 24 Branch → Trunk
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.
Attachment #8388217 - Flags: review?(jwatt)
Flags: needinfo?(mounir)
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!
Attachment #8388217 - Attachment is obsolete: true
Attachment #8413427 - Flags: feedback?(mounir)
Flags: needinfo?(mounir)
Dropping needinfo? request because it was two years ago. If you still want to hear from me, please add the flag back.
Flags: needinfo?(mounir)
Attachment #8413427 - Flags: feedback?(mounir)
Assignee: agi.novanta → nobody
Status: ASSIGNED → NEW
Severity: major → --
Component: DOM: Core & HTML → DOM: Forms
Priority: P3 → --
You need to log in before you can comment on or make changes to this bug.