Closed Bug 1283915 Opened 8 years ago Closed 8 years ago

Cursor moves when dynamically altering a Text field using Javascript.

Categories

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

Product:
Core
Component:
DOM: Events
Version:
47 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: james.jackson, Assigned: deckycoss)

References

(
URL
)

Details

Attachments

(1 file)

Bug 1283915: preserve input selection properties after type change;
58 bytes, text/x-review-board-request
smaug
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

Visit https://compass.lsus.edu/FCCSC/navigate/student.jsp

Attempt to enter text into the Student ID text field.




Actual results:

The cursor moves to the beginning of the field when you enter the first character.


Expected results:

The cursor should remain at the end of the field during the entire typing process.

Please note that this field is coded the way it is to avoid a problem we are having with Chrome. The Student ID field needs to be masked, but loading the page with it as a password type field causes it to have issues with the Chrome password manager.

As such, we have the field load as a text field, and then change to a password field using the oninput listener.

We presume that when this change occurs, the cursor is regenerated at the beginning of the field. No other browsers are experiencing this cursor moving behavior.
Please note that this field is coded the way it is to avoid a problem we are having with Chrome. The Student ID field needs to be masked, but loading the page with it as a password type field causes it to have issues with the Chrome password manager.

As such, we have the field load as a text field, and then change to a password field using the oninput listener.

We presume that when this change occurs, the cursor is regenerated at the beginning of the field. No other browsers are experiencing this cursor moving behavior.
OS: Unspecified → All
Hardware: Unspecified → All
Olli, WDYT?
Status: UNCONFIRMED → NEW
Component: HTML: Parser → DOM: Events
Ever confirmed: true
Flags: needinfo?(bugs)
James, could you perhaps write a minimal testcase for this?

Is the case here so that you have <input type=text> initially and then switch to <input type=password>.
And yes, I'm not aware us having such caret location cache when totally recreating a widget and I'm not aware of anyone ever complained about it.
But, a minimal testcase would be really nice. If you don't have time to write such, I'll try to find time. Please let me know.
Flags: needinfo?(bugs) → needinfo?(james.jackson)
ehsan, you might remember if we have anything for this, given that you moved <input> element's editor controlling from layout to dom or so.
Flags: needinfo?(ehsan)
Priority: -- → P3
The following is a simple test case in which the problem still exists:

http://pastebin.com/TzMrNJFq


Please let me know if there is any other way I can be of assistance.
Yes Olli. Our intent is to have <input type=text> initially and then switch to <input type=password>.
I guess 
data:text/html,<input type=text oninput="if (this.type !='password') this.type = 'password';">
is close to minimal testcase.
URL: data:text/html,<input type=text oninp...
Flags: needinfo?(james.jackson)
(In reply to Olli Pettay [:smaug] from comment #4)
> ehsan, you might remember if we have anything for this, given that you moved
> <input> element's editor controlling from layout to dom or so.

Yes, we do cache the selection information in nsTextEditorState::mSelectionProperties.  We currently populate the cache when the frame for the input element is about to be destroyed: <http://searchfox.org/mozilla-central/source/dom/html/nsTextEditorState.cpp#1623>.  We restore this after the element gains a few frame: <http://searchfox.org/mozilla-central/source/dom/html/nsTextEditorState.cpp#108>.  Judging from the symptoms of this, I think something in one of these two places is failing...
Flags: needinfo?(ehsan)
Decky said she's interested in investigating this.
Assignee: nobody → coss
Depends on: 1287655
Comment on attachment 8779773 [details]
Bug 1283915: preserve input selection properties after type change;

https://reviewboard.mozilla.org/r/70708/#review68056

::: dom/html/HTMLInputElement.cpp:4827
(Diff revision 1)
> +  nsTextEditorState::SelectionProperties sp;
> +
> +  if (GetEditorState()) {
> +    sp = mInputData.mState->GetSelectionProperties();
> +  } else {
> +    sp = nsTextEditorState::SelectionProperties();

Why you need this else ?

::: dom/html/HTMLInputElement.cpp:4837
(Diff revision 1)
>    mType = aNewType;
>  
>    if (IsSingleLineTextControl()) {
> +
> -    mInputData.mState = new nsTextEditorState(this);
> +      mInputData.mState = new nsTextEditorState(this);
> +      if (! sp.IsDefault()) {

extra space after !

::: dom/html/nsTextEditorState.cpp:1539
(Diff revision 1)
>    }
>    return mSelectionProperties;
>  }
>  
> +void
> +nsTextEditorState::SetSelectionProperties(nsTextEditorState::SelectionProperties& props)

aProps

::: dom/html/nsTextEditorState.cpp:1541
(Diff revision 1)
>  }
>  
> +void
> +nsTextEditorState::SetSelectionProperties(nsTextEditorState::SelectionProperties& props)
> +{
> +  if (mBoundFrame) {

This needs some explanation. Why mBoundFrame means number control? Since, if I read the code right, it doesn't do that. Or perhaps this doesn't even mean that.
Anyhow, this needs some comment. Similar to what nsTextEditorState::UnbindFromFrame has.


Hmm, but why nsTextEditorState::UnbindFromFrame isn't enough?

::: dom/html/nsTextEditorState.cpp:1547
(Diff revision 1)
> +    HTMLInputElement* number = GetParentNumberControl(mBoundFrame);
> +    if (number) {
> +      number->SetSelectionProperties(props);
> +    }
> +  }
> +  else {

nit, 

if (...) {
  ...
} else {
  ...
}
Attachment #8779773 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8779773 [details]
> Bug 1283915: preserve input selection properties after type change;
> 
> https://reviewboard.mozilla.org/r/70708/#review68056
> 
> ::: dom/html/HTMLInputElement.cpp:4827
> (Diff revision 1)
> > +  nsTextEditorState::SelectionProperties sp;
> > +
> > +  if (GetEditorState()) {
> > +    sp = mInputData.mState->GetSelectionProperties();
> > +  } else {
> > +    sp = nsTextEditorState::SelectionProperties();
> 
> Why you need this else ?

wouldn't sp be uninitialized otherwise? would it be safe to assume that sp.IsDefault() is true if i remove it?

> ::: dom/html/HTMLInputElement.cpp:4837
> (Diff revision 1)
> >    mType = aNewType;
> >  
> >    if (IsSingleLineTextControl()) {
> > +
> > -    mInputData.mState = new nsTextEditorState(this);
> > +      mInputData.mState = new nsTextEditorState(this);
> > +      if (! sp.IsDefault()) {
> 
> extra space after !
> 
> ::: dom/html/nsTextEditorState.cpp:1539
> (Diff revision 1)
> >    }
> >    return mSelectionProperties;
> >  }
> >  
> > +void
> > +nsTextEditorState::SetSelectionProperties(nsTextEditorState::SelectionProperties& props)
> 
> aProps
> 
> ::: dom/html/nsTextEditorState.cpp:1541
> (Diff revision 1)
> >  }
> >  
> > +void
> > +nsTextEditorState::SetSelectionProperties(nsTextEditorState::SelectionProperties& props)
> > +{
> > +  if (mBoundFrame) {
> 
> This needs some explanation. Why mBoundFrame means number control? Since, if
> I read the code right, it doesn't do that. Or perhaps this doesn't even mean
> that.
> Anyhow, this needs some comment. Similar to what
> nsTextEditorState::UnbindFromFrame has.
> 
> 
> Hmm, but why nsTextEditorState::UnbindFromFrame isn't enough?

to be honest, i only included that because that's what GetSelectionProperties was doing...perhaps it's related to this: https://hg.mozilla.org/mozilla-central/file/157268e90825/dom/html/nsTextEditorState.cpp#l1615
(In reply to Decky Coss (:deckycoss) from comment #12)
> > > +void
> > > +nsTextEditorState::SetSelectionProperties(nsTextEditorState::SelectionProperties& props)
> > > +{
> > > +  if (mBoundFrame) {
> > 
> > This needs some explanation. Why mBoundFrame means number control? Since, if
> > I read the code right, it doesn't do that. Or perhaps this doesn't even mean
> > that.
> > Anyhow, this needs some comment. Similar to what
> > nsTextEditorState::UnbindFromFrame has.
> > 
> > 
> > Hmm, but why nsTextEditorState::UnbindFromFrame isn't enough?
> 
> to be honest, i only included that because that's what
> GetSelectionProperties was doing...perhaps it's related to this:
> https://hg.mozilla.org/mozilla-central/file/157268e90825/dom/html/
> nsTextEditorState.cpp#l1615

Hmm, I don't think you need to care about number controls specifically at all here.  (In fact, over in bug 1293570, I'm removing any handling of selection APIs for number controls.)

Instead, if mBoundFrame is not null, you should set the selection properties on the frame by calling SetSelectionRange(), similar to what RestoreSelectionState::Run() does.
Comment on attachment 8779773 [details]
Bug 1283915: preserve input selection properties after type change;

https://reviewboard.mozilla.org/r/70708/#review68152

Minor things, but could you update the test a bit to include type=number

::: dom/html/HTMLInputElement.cpp:4830
(Diff revision 2)
> +  nsTextEditorState::SelectionProperties sp;
> +
> +  if (GetEditorState()) {
> +    sp = mInputData.mState->GetSelectionProperties();
> +  } else {
> +    sp = nsTextEditorState::SelectionProperties();

this is still unneeded. sp has been initialized with the default ctor already.

::: dom/html/nsTextEditorState.h:263
(Diff revision 2)
>        nsITextControlFrame::SelectionDirection mDirection;
>    };
>  
>    bool IsSelectionCached() const;
>    SelectionProperties& GetSelectionProperties();
> +  void SetSelectionProperties(SelectionProperties&);

add argument name

::: dom/html/test/forms/test_bug1283915.html:38
(Diff revision 2)
> +  </script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1283915">Mozilla Bug 1283915</a>
> +<p id="display"></p>
> +<input id="textField" type="text" oninput="if (this.type !='password') this.type = 'password';">

We need also some test for number, given that it atm supports selection handling, yet its layout is special
Attachment #8779773 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8779773 [details]
> Bug 1283915: preserve input selection properties after type change;
> 
> https://reviewboard.mozilla.org/r/70708/#review68152
> 
> We need also some test for number, given that it atm supports selection
> handling, yet its layout is special

what cases should i test? switching type to/from 'number'?
Comment on attachment 8779773 [details]
Bug 1283915: preserve input selection properties after type change;

https://reviewboard.mozilla.org/r/70708/#review68696

::: dom/html/test/forms/test_bug1283915.html:18
(Diff revision 4)
> +
> +  /** Test for Bug 1283915 **/
> +
> +  SimpleTest.waitForExplicitFinish();
> +
> +  function test() {

So this test is missing to check that selection properties are right.
You only test .value.
Attachment #8779773 - Flags: review?(bugs) → review-
Comment on attachment 8779773 [details]
Bug 1283915: preserve input selection properties after type change;

https://reviewboard.mozilla.org/r/70708/#review68698
Comment on attachment 8779773 [details]
Bug 1283915: preserve input selection properties after type change;

https://reviewboard.mozilla.org/r/70708/#review68772

thanks, and sorry that I didn't ask for testing selection properties before.
Attachment #8779773 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8779773 [details]
> Bug 1283915: preserve input selection properties after type change;
> 
> https://reviewboard.mozilla.org/r/70708/#review68772
> 
> thanks, and sorry that I didn't ask for testing selection properties before.

np, thanks for reviewing
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f709b7217a6
Preserve input selection properties after type change. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f709b7217a6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hey everyone,

Thank you for all of your effort in working on this issue. I greatly appreciate it.

When do you think this bug fix will go live? I just updated FF and the issue still seems to persist as of this moment.

-James
This is fixed in Nightlies, FF51, which is still couple of months away from release.
The change is relatively safe, so we could try to get it to FF50 I think.
But beta (FF49) feels like stretch


deckycoss, want to ask approval for Aurora?
Flags: needinfo?(coss)
(In reply to Olli Pettay [:smaug] from comment #28)
> This is fixed in Nightlies, FF51, which is still couple of months away from
> release.
> The change is relatively safe, so we could try to get it to FF50 I think.
> But beta (FF49) feels like stretch
> 
> 
> deckycoss, want to ask approval for Aurora?

it depends on a patch that changes a core behaviour (bug 1287655) and a bunch of tests, so i'm not sure it actually is safe enough to uplift to aurora.
Flags: needinfo?(coss)
ah, right. And this is just a normal bug, so normal release cycle it is then.
Thanks for the update! Again, I really appreciate the work that was put into this.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: