1297225 - CSS filter editor relies on the setSelectionRange() for an <input type=number>

Status: RESOLVED FIXED

Description

[:Ehsan Akhgari]

The HTML spec says that setSelectionRange() and the rest of the selection APIs do not apply to <input type=number>.  I'm fixing our implementation in bug 1293570 and have discovered this dependency through that bug.

The code here doesn't actually depend on being able to do this for <input type=number>s.  This is only used for text input boxes in this widget.  Note that the code in _keyDown() also accesses these APIs, but for filters which don't have a unit, which are the ones that we create <input type=text>s for in render(), so those call sites are fine.

Comment 1

[:Ehsan Akhgari]

Created attachment 8783746 [details] [diff] [review]
Avoid calling HTMLInputElement.setSelectionRange() on <input type=number>s in the CSS filter widget code

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8783746 [details] [diff] [review]
Avoid calling HTMLInputElement.setSelectionRange() on <input type=number>s in the CSS filter widget code

Review of attachment 8783746 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/widgets/FilterWidget.js
@@ +675,5 @@
>      }
>  
>      let lastInput =
>          this.filtersList.querySelector(".filter:last-of-type input");
> +    if (lastInput && lastInput.type == "text") {

Adding the condition here means that we're no longer focusing the field, which we want to do so users can start typing in the field directly.

Can you move the if (lastInput.type == "text") below, so it's just around the
const end = lastInput.value.length;
lastInput.setSelectionRange(end, end);

Comment 3

[:Ehsan Akhgari]

Good catch, will do!

Comment 4

[:Ehsan Akhgari]

Created attachment 8784102 [details] [diff] [review]
Avoid calling HTMLInputElement.setSelectionRange() on <input type=number>s in the CSS filter widget code

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8784102 [details] [diff] [review]
Avoid calling HTMLInputElement.setSelectionRange() on <input type=number>s in the CSS filter widget code

Review of attachment 8784102 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/502e48c4fb6a
Avoid calling HTMLInputElement.setSelectionRange() on <input type=number>s in the CSS filter widget code; r=pbro

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/502e48c4fb6a

Comment 8

[Sebastian Zartner [:sebo]]

> Review of attachment 8784102 [details] [diff] [review] [diff] [review]:
> -----------------------------------------------------------------
> +      if (lastInput.type == "text") {

Nit: That should have been ===.

Sebastian

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/251a9edbb0cc
Address the post-landing drive-by nit

Comment 10

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/251a9edbb0cc