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

Status: RESOLVED FIXED

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

Description

[Simon Pieters [:zcorpan]]

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Jessica Jong [:jessica] (inactive)]

(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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Let type=number support text selection methods, this was made intentionally in bug 1003741.

Comment 7

[Jessica Jong [:jessica] (inactive)]

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...

Comment 8

[Jessica Jong [:jessica] (inactive)]

(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

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

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).

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

rebased after Bug 1287655, carrying r+.

Comment 13

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4ac842e39f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1e5c61b395
Do not throw when getting selectionDirection/selectionStart/selectionEnd on input/textarea. r=smaug

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c1e5c61b395