1231923 - Enable select events for chrome consumers

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

At some point, we should flip the pref outside of nightly.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

What is this waiting on? I'd like to use it for some of the Firefox frontend work I'm doing (bug 1294799)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Current status: There are three outstanding issues with the spec currently.
https://github.com/w3c/selection-api/issues/53 Selection events for text controls
https://github.com/w3c/selection-api/issues/54 Clarify the additions of this spec to GlobalEventHandlers
https://github.com/w3c/selection-api/issues/37 Specify Selection.modify()?

On IRC today,
<rniwa> ehsan, jaws: I'm gonna try to update the spec sometime this month or next month before TPAC

Comment 5

[(no longer active)]

selection.modify() is not related to this per se, we can track those changes independently.

Comment 6

[Mike de Boer [:mikedeboer]]

Or perhaps at least let this ride the trains usable to chrome only? I mean, if that's at all possible and easy to do?
Thanks!

Comment 7

[(no longer active)]

I don't think it's worth spending time to expose this to chrome only, but that being said, I'm not sure when I will get to work on this.  I have an enormous backlog of things I need to do.  :(

Comment 9

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

Attachment: Enable selection events for documents with the System Principal

It didn't seem like it would be very tricky at all to make these events be fired only on documents with the system principal. This is a patch which I _think_ should do that (I have barely tested it at all).

Comment 10

[(no longer active)]

The reason why I prefer to not do this is that some of the changes we need to make to our implementation will probably not be backwards compatible with the current implementation, and I prefer to not have to chase down chrome consumers (and add-ons) for that stuff when we start to align with the spec.  But if you really want to have this, I guess I won't object as long as you'd be OK with the implementation changing without notice.

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to :Ehsan Akhgari from comment #10)
> But if you really want to have this, I guess I won't object as long as you'd
> be OK with the implementation changing without notice.

I wouldn't complain at all.

Comment 12

[Mike de Boer [:mikedeboer]]

Michael, would you? (see comment 10)

Comment 13

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

Comment on attachment 8795451 [details] [diff] [review]
Enable selection events for documents with the System Principal

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

Ehsan, does this seem like an OK temporary solution to make mike able to work with select events while the spec is being figured out?

Mike, can you try applying this patch and make sure that it does what you want?

Comment 14

[(no longer active)]

Comment on attachment 8795451 [details] [diff] [review]
Enable selection events for documents with the System Principal

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

This looks fine as is, but I'm puzzled by the question below.  I think it's a bit better to keep things consistent.

::: layout/generic/nsSelection.cpp
@@ -933,5 @@
>        mDomSelections[index]->AddSelectionListener(eventHub);
>      }
>    }
>  
> -  if (sSelectionEventsEnabled) {

This is kind of a round-about way of doing things.  Why not just look at the principal of the selection's parent object here as well?

Comment 15

[Mike de Boer [:mikedeboer]]

Hi Michael, I can confirm that it works as expected with the patch in bug 1304073 applied. Thanks!

Comment 16

[Piotrek Koszuliński]

Hi guys!

We're watching this ticket at CKEditor (https://github.com/ckeditor/ckeditor5-engine/issues/407) and we really hope that the event will be enabled publicly soon.

To avoid creating a polyfill for selectionchange (because Firefox is the only browser which doesn't support it), we've been testing the editor with the event enabled for quite a long time and haven't seen any issues. So, from our perspective, it's ready to be shipped publicly.

Even if there are some glitches and even if some behaviour may change in the future (once the spec is clarified), we'd encourage you to go public with it. As RTEs developers we're used to the fact that browsers are changing quite frequently and that nothing is perfect :D. It's better for us if the event is enabled and buggy than having to create a polyfill (which will be buggy as well).

Comment 17

[(no longer active)]

The remaining work is mostly around spec compliance.  I'm glad to hear your testing hasn't uncovered any issues, that's reassuring.  I also agree with you that we should work on enabling this API for normal web pages.

This bug's summary is unfortunately incorrect now, after it started to get hijacked in comment 2, it's now tracking enabling this event for internal consumers in Firefox.

But we still need to ship this API, which is more important IMHO.  I filed bug 1309612 for that.  Please follow along there.

Comment 18

[(no longer active)]

So I realized that we want to do this, even if we end up working on shipping the API at the same time, so that the front-end code can depend on this feature even if we end up disabling the API on the train ride towards shipping.

Michael, are you planning to keep working on this?

Comment 19

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

Attachment: Enable selection events for documents with the System Principal

Updated version in response to review requests

Comment 20

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

I had already written up a fixed up patch, but hadn't pushed it yet.

Comment 21

[(no longer active)]

Comment on attachment 8800741 [details] [diff] [review]
Enable selection events for documents with the System Principal

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

Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d87bca44d8
Enable selection events for documents with the System Principal, r=ehsan

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9d87bca44d8

Comment 24

[Eric Shepherd [:sheppy]]

As far as I can tell, this bug is simply about letting chrome code use selection events just like web code can, no? If that’s the case, then simply documenting the selection events (which is being tracked with bug 1309612) will cover this. I’m removing dev-doc-needed from this bug. Please re-add it if there are special issues related specifically to this bug. Thanks!