1309628 - Ensure that we don't dispatch the selectstart and selectionchange events for the selections inside input and textarea elements

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[(no longer active)]

The current spec restricts these events to Window's selection.

Comment 1

[(no longer active)]

Attachment: Hide support for dispatching selection events on the contents of text controls behind a pref

This patch adds a pref to control whether we dispatch the selection
events for changes in the contents of input and textarea text controls.
The spec for this feature hasn't been written yet, and we need to exclude
this part of the selection API from the part we want to ship.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8800511 [details] [diff] [review]
Hide support for dispatching selection events on the contents of text controls behind a pref

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

Looks good, but the test looks a bit broken, so clearing for that.

::: dom/tests/mochitest/general/frameSelectEvents.html
@@ +48,5 @@
>  
> +        var selectEventsOnTextControlsEnabledOriginal =
> +          SpecialPowers.getBoolPref("dom.select_events.textcontrols.enabled");
> +        function* UpdateSelectEventsOntextControlsPref(aEnabled) {
> +          var promise = new Promise((done) => {

SpecialPowers.pushPrefEnv returns a promise if you don't pass a callback, so you can probably just write this function as:

function UpdateSelectEventsOntextControlsPref(aEnabled) {
    return SpecialPowers.pushPrefEnv({set: [['dom.select_events.textcontrols.enabled', aEnabled]]});
}

@@ +49,5 @@
> +        var selectEventsOnTextControlsEnabledOriginal =
> +          SpecialPowers.getBoolPref("dom.select_events.textcontrols.enabled");
> +        function* UpdateSelectEventsOntextControlsPref(aEnabled) {
> +          var promise = new Promise((done) => {
> +            SpecialPowers.pushPrefEnv({'set': [['dom.select_events.textcontrols.enabled', true]]}, done);

It seems like you're accepting an aEnabled parameter but always setting the pref to true? This seems wrong.

@@ +436,5 @@
>          is(inputSelectionchange, 0, "tdn - isc 2");
>          is(textareaSelectionchange, 0, "tdn - tsc 2");
>          reset();
> +
> +        yield* UpdateSelectEventsOntextControlsPref(selectEventsOnTextControlsEnabledOriginal);

You're using pushPrefEnv, so the prefs should clean themselves up after the test is over if I understand how it works correctly. I don't think this bookkeeping or call to restore the pref at the end is necessary.

Comment 3

[(no longer active)]

You're right about all of the above.  New patch coming up.

Comment 4

[(no longer active)]

Attachment: Hide support for dispatching selection events on the contents of text controls behind a pref

This patch adds a pref to control whether we dispatch the selection
events for changes in the contents of input and textarea text controls.
The spec for this feature hasn't been written yet, and we need to exclude
this part of the selection API from the part we want to ship.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8800816 [details] [diff] [review]
Hide support for dispatching selection events on the contents of text controls behind a pref

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

::: dom/tests/mochitest/general/frameSelectEvents.html
@@ +45,5 @@
>  
>          var cancel = false;
>          var selectstartTarget = null;
>  
> +        function* UpdateSelectEventsOntextControlsPref(aEnabled) {

This shouldn't have to be a function* as it only returns a single Promise, and then you could yield it with `yield` instead of `yield*`. I feel like that would be cleaner.

@@ +323,5 @@
>          */
>  
>          // Select a region
>  
> +        // Without the pref, pressing the mouse shouldn't do anything.

Can you clarify what pref you're talking about in this comment? "the pref" seems overly general.

@@ +374,5 @@
>  
>  
> +        yield* UpdateSelectEventsOntextControlsPref(false);
> +
> +        // Without the pref, pressing the mouse shouldn't do anything.

Same with this comment

Comment 6

[(no longer active)]

Attachment: interdiff for bug 1309628

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8800830 [details] [diff] [review]
interdiff for bug 1309628

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

\o/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e664a1bdebd
Hide support for dispatching selection events on the contents of text controls behind a pref; r=mystor

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e664a1bdebd

Comment 10

[Andreas Herd]

I apologize in advance for probably not knowing enough about the w3c specs on this, but unlike selectionstart selectionchange fires in Edge and Chrome whenever selectionStart or selectionEnd on an input field changes. This happens even on document bound event listeners. In Firefox this event is not fired when I focus an input field or move the cursor via arrow keys. It sort of makes sense as electionstart implies something actually behind selected, selectionchange should fire even if the selection length is zero, is minding the cursor.

Comment 11

[Andreas Herd]

I apologize in advance for probably not knowing enough about the w3c specs on this, but unlike selectionstart selectionchange fires in Edge and Chrome whenever selectionStart or selectionEnd on an input field changes. This happens even on document bound event listeners. In Firefox this event is not fired when I focus an input field or move the cursor via arrow keys. It sort of makes sense as selectionstart implies something actually being selected, selectionchange should fire even if the selection length is zero, is moving the cursor.