Closed Bug 1281769 Opened 6 years ago Closed 6 years ago

Do not throw when getting selectionDirection/selectionStart/selectionEnd on input

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: zcorpan, Assigned: jessica)

Details

(Whiteboard: [tw-dom])

Attachments

(1 file, 3 obsolete files)

See https://github.com/whatwg/html/pull/1006

var input = document.createElement('input');
input.type = 'checkbox';
input.selectionDirection; // should return null
input.selectionStart; // should return null
input.selectionEnd; // should return null
Priority: -- → P3
Whiteboard: [tw-dom]
Assignee: nobody → jjong
After the spec changes to:

  attribute unsigned long? selectionStart;
  attribute unsigned long? selectionEnd;

means that we can also set a null value to selectionStart/End, I think we can just ignore when value is null, right?

Another thing is, what about the selectionStart/End attributes in xpidl? Since xpidl does not support null values, I am changing it to 'jsval', I am wondering whether that is what we want, since there are some complexity doing the conversion.
Flags: needinfo?(bugs)
We don't need to change .idl. It just won't support null values. .webidl is what is exposed to the web.

And the spec is just buggy right now, IMO.
I *think* null is supposed to be converted to 0, but it is unclear.
https://github.com/whatwg/html/issues/1628
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #2)
> We don't need to change .idl. It just won't support null values. .webidl is
> what is exposed to the web.

I see. But since .idl implementation calls .webidl implementation directly [1], should we keep throwing if null is returned?

> 
> And the spec is just buggy right now, IMO.
> I *think* null is supposed to be converted to 0, but it is unclear.
> https://github.com/whatwg/html/issues/1628


[1] http://hg.mozilla.org/mozilla-central/file/1576e7bc1bec/dom/html/HTMLInputElement.cpp#l5784
(In reply to Jessica Jong [:jessica] from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > We don't need to change .idl. It just won't support null values. .webidl is
> > what is exposed to the web.
> 
> I see. But since .idl implementation calls .webidl implementation directly
> [1], should we keep throwing if null is returned?
returning some error value from idl methods in that case should be fine yes
Attached patch patch, v1. (obsolete) — Splinter Review
Attached patch patch, v2. (obsolete) — Splinter Review
Let type=number support text selection methods, this was made intentionally in bug 1003741.
Attachment #8778098 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6642ed3b71d8&selectedJob=25198371

This seems to be breaking toolkit/components/satchel/test/test_form_autocomplete.html on OSX, still figuring out why...
(In reply to Jessica Jong [:jessica] from comment #7)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6642ed3b71d8&selectedJob=25198371
> 
> This seems to be breaking
> toolkit/components/satchel/test/test_form_autocomplete.html on OSX, still
> figuring out why...

Turns out autocomplete uses nsIDOMHTMLInputElement::GetSelectionStart/GetSelectionEnd/SetSelectionRange [1], I think I'll just add checks in webidl implementations, and leave xpidl implementation as it is, for backward compatibility. 

[1] http://hg.mozilla.org/mozilla-central/file/6b65dd49d4f0/toolkit/components/satchel/nsFormFillController.cpp#l527
Attached patch patch, v3. (obsolete) — Splinter Review
For GetSelectionStart/GetSelectionEnd/SetSelectionRange, I put the implementation part in .idl functions instead of .webidl functions, so that .webidl can filter out non-supported types before executing. I only moved theses three functions cause these are the only called internally (from nsFormFillController.cpp).
Attachment #8778526 - Attachment is obsolete: true
Attachment #8778757 - Flags: review?(bugs)
Comment on attachment 8778757 [details] [diff] [review]
patch, v3.

>-   * Returns true if setRangeText can be called on element
>+   * Returns true if selection methods can be called on element
>    */
>-  bool SupportsSetRangeText() const {
>+  bool SupportsTextSelection() const {
>     return mType == NS_FORM_INPUT_TEXT || mType == NS_FORM_INPUT_SEARCH ||
>            mType == NS_FORM_INPUT_URL || mType == NS_FORM_INPUT_TEL ||
>            mType == NS_FORM_INPUT_PASSWORD || mType == NS_FORM_INPUT_NUMBER;
Hmm, number shouldn't be in this list.
This is a regression from bug 1003741. Could you file a separate bug to sort this out?


> HTMLTextAreaElement::GetSelectionStart(int32_t *aSelectionStart)
> {
>   NS_ENSURE_ARG_POINTER(aSelectionStart);
> 
>   ErrorResult error;
>-  *aSelectionStart = GetSelectionStart(error);
>+  Nullable<uint32_t> selStart(GetSelectionStart(error));
>+  if (error.Failed()) {
>+    return error.StealNSResult();
>+  }
>+
>+  *aSelectionStart = int32_t(selStart.Value());
ok, so in textarea case GetSelectionStart never returns null, so this is ok.
Attachment #8778757 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8778757 [details] [diff] [review]
> patch, v3.
> 
> >-   * Returns true if setRangeText can be called on element
> >+   * Returns true if selection methods can be called on element
> >    */
> >-  bool SupportsSetRangeText() const {
> >+  bool SupportsTextSelection() const {
> >     return mType == NS_FORM_INPUT_TEXT || mType == NS_FORM_INPUT_SEARCH ||
> >            mType == NS_FORM_INPUT_URL || mType == NS_FORM_INPUT_TEL ||
> >            mType == NS_FORM_INPUT_PASSWORD || mType == NS_FORM_INPUT_NUMBER;
> Hmm, number shouldn't be in this list.
> This is a regression from bug 1003741. Could you file a separate bug to sort
> this out?

Sure, filed Bug 1293570 for this.

> 
> 
> > HTMLTextAreaElement::GetSelectionStart(int32_t *aSelectionStart)
> > {
> >   NS_ENSURE_ARG_POINTER(aSelectionStart);
> > 
> >   ErrorResult error;
> >-  *aSelectionStart = GetSelectionStart(error);
> >+  Nullable<uint32_t> selStart(GetSelectionStart(error));
> >+  if (error.Failed()) {
> >+    return error.StealNSResult();
> >+  }
> >+
> >+  *aSelectionStart = int32_t(selStart.Value());
> ok, so in textarea case GetSelectionStart never returns null, so this is ok.
Attached patch patch, v4.Splinter Review
rebased after Bug 1287655, carrying r+.
Attachment #8778757 - Attachment is obsolete: true
Attachment #8779195 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1e5c61b395
Do not throw when getting selectionDirection/selectionStart/selectionEnd on input/textarea. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c1e5c61b395
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.