Closed Bug 1287655 Opened 3 years ago Closed 3 years ago

input/textarea selectionStart, selectionEnd should return cursor position when selection is empty

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: deckycoss, Assigned: deckycoss, NeedInfo)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

according to the spec, selectionStart and selectionEnd "must return the offset (in logical order) to the character that immediately follows the text entry cursor" if no text is selected: https://www.w3.org/TR/html5/forms.html#textFieldSelection

however, according to this test, selectionStart and selectionEnd will always be 0 if there is no selection: http://w3c-test.org/html/semantics/forms/textfieldselection/selection.html
So we have text entry cursor at 0.
As far as I see, the test is invalid, but seems like some other browsers pass it so worth to investigate how/why browsers work differently.

And note, we are not trying to implement https://www.w3.org/TR/html5 but http://www.whatwg.org/specs/web-apps/current-work/

data:text/html,<script>window.onload = function() { var i = document.createElement("input"); i.value="foo"; document.body.appendChild(i); i.focus();  }</script>
shows where cursor is put. We have it at 0, Chrome puts it at the end of text.
Blocks: 1283915
Comment on attachment 8776618 [details]
Bug 1287655: place textarea/input cursor at end of text when initialized;

https://reviewboard.mozilla.org/r/68336/#review65714

::: dom/html/nsTextEditorState.h:216
(Diff revision 1)
>  
>    void HideSelectionIfBlurred();
>  
>    struct SelectionProperties {
> +    private:
> +      int32_t mStart, mEnd;

Please don't move member variable to top. They should be after method declarations.

::: dom/html/nsTextEditorState.h:219
(Diff revision 1)
>    struct SelectionProperties {
> +    private:
> +      int32_t mStart, mEnd;
> +      nsITextControlFrame::SelectionDirection mDirection;
> +    public:      
> +      bool mIsDirty = false;

Why mIsDirty is public? getter/setter for it would be nicer.

And what does mIsDirty mean? It needs some comment.

::: dom/html/nsTextEditorState.h:226
(Diff revision 1)
> -      mDirection(nsITextControlFrame::eForward) {}
> +        mDirection(nsITextControlFrame::eForward) {}
> -    bool IsDefault() const {
> +      bool IsDefault() const {
> -      return mStart == 0 && mEnd == 0 &&
> +        return mStart == 0 && mEnd == 0 &&
> -             mDirection == nsITextControlFrame::eForward;
> +               mDirection == nsITextControlFrame::eForward;
> -    }
> +      }
> -    int32_t mStart, mEnd;
> +      int32_t GetStart() const {

{ goes to its own line

::: dom/html/nsTextEditorState.h:229
(Diff revision 1)
> -             mDirection == nsITextControlFrame::eForward;
> +               mDirection == nsITextControlFrame::eForward;
> -    }
> +      }
> -    int32_t mStart, mEnd;
> -    nsITextControlFrame::SelectionDirection mDirection;
> +      int32_t GetStart() const {
> +        return mStart;
> +      }
> +      void SetStart(int32_t value){

ditto (and there was missing space before {, but since { should be moved to its own line, no need to add space)

::: dom/html/nsTextEditorState.h:233
(Diff revision 1)
> +      }
> +      void SetStart(int32_t value){
> +        mIsDirty = true;
> +        mStart = value;
> +      }
> +      int32_t GetEnd() const {

ditto

::: dom/html/nsTextEditorState.h:236
(Diff revision 1)
> +        mStart = value;
> +      }
> +      int32_t GetEnd() const {
> +        return mEnd;
> +      }
> +      void SetEnd(int32_t value){

ditto

::: dom/html/nsTextEditorState.h:240
(Diff revision 1)
> +      }
> +      void SetEnd(int32_t value){
> +        mIsDirty = true;
> +        mEnd = value;
> +      }
> +      nsITextControlFrame::SelectionDirection GetDirection() const {

and here too

::: dom/html/nsTextEditorState.h:243
(Diff revision 1)
> +        mEnd = value;
> +      }
> +      nsITextControlFrame::SelectionDirection GetDirection() const {
> +        return mDirection;
> +      }
> +      void SetDirection(nsITextControlFrame::SelectionDirection value){

and here

::: layout/forms/nsTextControlFrame.cpp:312
(Diff revision 1)
> +    txtCtrl->GetTextEditorValue(val, true);
> +    int32_t length = val.Length();
> +
> +    // Set the selection to the end of the text field. (bug 1287655)
>      if (weakFrame.IsAlive()) {
> -      SetSelectionEndPoints(0, 0);
> +      SetSelectionEndPoints(length, length);

So have you tested the cursor is in right position after reframing <input> or <textarea>?
also in case the cursor is somewhere in middle of the text.

Something like
data:text/html,<input oninput="setTimeout(function() {event.target.style.display='none'; setTimeout(function() {event.target.style.display='block'; }, 100); }, 0)">
Attachment #8776618 - Flags: review?(bugs) → review-
Comment on attachment 8776618 [details]
Bug 1287655: place textarea/input cursor at end of text when initialized;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68336/diff/1-2/
Attachment #8776618 - Attachment description: Bug 1287655: place textarea/input cursor at end of text when initialized; → Bug 1287655: place textarea/input cursor at end of text when initialized ;
Attachment #8776618 - Flags: review- → review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8776618 [details]
> Bug 1287655: place textarea/input cursor at end of text when initialized ;
> 
> https://reviewboard.mozilla.org/r/68336/#review65714
> 
> ::: dom/html/nsTextEditorState.h:216
> (Diff revision 1)
> >  
> >    void HideSelectionIfBlurred();
> >  
> >    struct SelectionProperties {
> > +    private:
> > +      int32_t mStart, mEnd;
> 
> Please don't move member variable to top. They should be after method
> declarations.
> 
> ::: dom/html/nsTextEditorState.h:219
> (Diff revision 1)
> >    struct SelectionProperties {
> > +    private:
> > +      int32_t mStart, mEnd;
> > +      nsITextControlFrame::SelectionDirection mDirection;
> > +    public:      
> > +      bool mIsDirty = false;
> 
> Why mIsDirty is public? getter/setter for it would be nicer.
> 
> And what does mIsDirty mean? It needs some comment.
> 

done; i decided to not implement a setter because i couldn't think of a reason for mIsDirty to be modified externally.

by the way, should i name the getter IsDirty() instead of GetIsDirty() according to the style guide? (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods) there is currently no public means of setting mIsDirty to null, nor any private method that does so.

> 
> So have you tested the cursor is in right position after reframing <input>
> or <textarea>?
> also in case the cursor is somewhere in middle of the text.
> 
> Something like
> data:text/html,<input oninput="setTimeout(function()
> {event.target.style.display='none'; setTimeout(function()
> {event.target.style.display='block'; }, 100); }, 0)">

i tried this and the cursor would always move to the position after the inputted character, instead of resetting to the beginning or end. is this correct?
(In reply to Decky Coss (:deckycoss) from comment #5)
> by the way, should i name the getter IsDirty() instead of GetIsDirty()
IsDirty() sounds better

 
> i tried this and the cursor would always move
move? 

> to the position after the
> inputted character, instead of resetting to the beginning or end. is this
> correct?
I'm not sure what you mean here now, but cursor shouldn't move anywhere even if input element was hidden and then shown again, even if cursor was in middle of the text somewhere.
So what behavior do you actually get with the patch?
(In reply to Olli Pettay [:smaug] from comment #6)
> (In reply to Decky Coss (:deckycoss) from comment #5)
> > by the way, should i name the getter IsDirty() instead of GetIsDirty()
> IsDirty() sounds better
> 
>  
> > i tried this and the cursor would always move
> move? 
> 
> > to the position after the
> > inputted character, instead of resetting to the beginning or end. is this
> > correct?
> I'm not sure what you mean here now, but cursor shouldn't move anywhere even
> if input element was hidden and then shown again, even if cursor was in
> middle of the text somewhere.
> So what behavior do you actually get with the patch?

i mean that the cursor follows the input, like this: http://i.imgur.com/qKb1xyM.gif
ok, good, thanks. So that behavior isn't change by the patch.
Comment on attachment 8776618 [details]
Bug 1287655: place textarea/input cursor at end of text when initialized;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68336/diff/2-3/
Attachment #8776618 - Attachment description: Bug 1287655: place textarea/input cursor at end of text when initialized ; → Bug 1287655: place textarea/input cursor at end of text when initialized;
Comment on attachment 8776618 [details]
Bug 1287655: place textarea/input cursor at end of text when initialized;

https://reviewboard.mozilla.org/r/68336/#review66196

ok, good to land this early in the cycle, given that this is a bit regression risky change.

::: layout/forms/nsTextControlFrame.cpp:312
(Diff revision 3)
> +    txtCtrl->GetTextEditorValue(val, true);
> +    int32_t length = val.Length();
> +
> +    // Set the selection to the end of the text field. (bug 1287655)
>      if (weakFrame.IsAlive()) {
> -      SetSelectionEndPoints(0, 0);
> +      SetSelectionEndPoints(length, length);

Ok, so we override these values in case there is TextEditorState which has selection state stored.
Attachment #8776618 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a0d2d76397
place textarea/input cursor at end of text when initialized; r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52a0d2d76397
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(In reply to Eric Shepherd [:sheppy] from comment #15)
> Documentation updated:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/51
> 
> Let me know if anything's still missing!

the comments on what the behaviour was like before the patch are misleading. the issue turned out not to be a bug in selectionStart or selectionEnd--they always returned the correct cursor position. rather, the issue was that the default cursor position of input/textarea was 0 instead of the end of the text. see this posting on fxsitecompat for a summary: https://www.fxsitecompat.com/en-CA/docs/2016/caret-will-be-placed-at-the-end-when-textbox-automatically-gets-focus/
Blocks: 1315146
Is it possible that this change broke the `selectionStart/-End` of input elements with `type=number`?

For input elements with type number `selectionStart` and `selectionEnd` return `null` after installing Firefox 51.
Jörn, could you please file a new bug and make it block this one?
Flags: needinfo?(joern.reimerdes)
Flags: needinfo?(coss)
Depends on: 1333737
Depends on: 1334723
Depends on: 1337392
You need to log in before you can comment on or make changes to this bug.