Enable select events for chrome consumers

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Updated

3 years ago
Duplicate of this bug: 1128226
What is this waiting on? I'd like to use it for some of the Firefox frontend work I'm doing (bug 1294799)
Flags: needinfo?(michael)
Duplicate of this bug: 1214446
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
Flags: needinfo?(michael)

Comment 5

3 years ago
selection.modify() is not related to this per se, we can track those changes independently.
Assignee: nobody → ehsan
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!
Flags: needinfo?(ehsan)

Comment 7

3 years ago
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.  :(
Flags: needinfo?(ehsan)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1242718
(Assignee)

Comment 9

3 years ago
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

3 years ago
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.
(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.
Michael, would you? (see comment 10)
Flags: needinfo?(michael)
(Assignee)

Comment 13

3 years ago
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?
Attachment #8795451 - Flags: review?(ehsan)
Attachment #8795451 - Flags: feedback?(mdeboer)
(Assignee)

Updated

3 years ago
Flags: needinfo?(michael)

Comment 14

3 years ago
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?
Attachment #8795451 - Flags: review?(ehsan) → review-
Hi Michael, I can confirm that it works as expected with the patch in bug 1304073 applied. Thanks!
Attachment #8795451 - Flags: feedback?(mdeboer) → feedback+
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

3 years ago
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.
Assignee: ehsan → nobody
Summary: Enable select events outside of nightly → Enable select events for chrome consumers

Comment 18

3 years ago
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?
Flags: needinfo?(michael)
(Assignee)

Comment 19

3 years ago
Updated version in response to review requests
Attachment #8800741 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Attachment #8795451 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
I had already written up a fixed up patch, but hadn't pushed it yet.
Flags: needinfo?(michael)

Comment 21

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

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

Thanks!
Attachment #8800741 - Flags: review?(ehsan) → review+

Comment 22

3 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d87bca44d8
Enable selection events for documents with the System Principal, r=ehsan

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9d87bca44d8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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!
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.