Closed
Bug 1287655
Opened 9 years ago
Closed 9 years ago
input/textarea selectionStart, selectionEnd should return cursor position when selection is empty
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68336/
Attachment #8776618 -
Flags: review?(bugs)
Comment 3•9 years ago
|
||
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-
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
(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?
Comment 6•9 years ago
|
||
(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?
Assignee | ||
Comment 7•9 years ago
|
||
(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
Comment 8•9 years ago
|
||
ok, good, thanks. So that behavior isn't change by the patch.
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
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
Comment 13•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Keywords: dev-doc-needed,
site-compat
Comment 14•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/caret-will-be-placed-at-the-end-when-textbox-automatically-gets-focus/
Comment 15•8 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 16•8 years ago
|
||
(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/
Comment 17•8 years ago
|
||
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.
Comment 18•8 years ago
|
||
Jörn, could you please file a new bug and make it block this one?
Flags: needinfo?(joern.reimerdes)
Flags: needinfo?(coss)
You need to log in
before you can comment on or make changes to this bug.
Description
•