Closed Bug 1309628 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files, 1 obsolete file)

The current spec restricts these events to Window's selection.
See Also: → 1196479
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.
Attachment #8800511 - Flags: review?(michael)
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.
Attachment #8800511 - Flags: review?(michael)
You're right about all of the above.  New patch coming up.
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.
Attachment #8800816 - Flags: review?(michael)
Attachment #8800511 - Attachment is obsolete: true
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
Attachment #8800816 - Flags: review?(michael) → review+
Attachment #8800830 - Flags: review?(michael)
Comment on attachment 8800830 [details] [diff] [review]
interdiff for bug 1309628

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

\o/
Attachment #8800830 - Flags: review?(michael) → review+
Pushed by eakhgari@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4e664a1bdebd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
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.
You need to log in before you can comment on or make changes to this bug.