Enable select events for chrome consumers

RESOLVED FIXED in Firefox 52

Status

()

Core
Selection
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

2 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
Blocks: 571294
Blocks: 1294799
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)
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)
Blocks: 1304073
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

9 months ago
Duplicate of this bug: 1242718
(Assignee)

Comment 9

9 months ago
Created attachment 8795451 [details] [diff] [review]
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).
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

9 months 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

9 months ago
Flags: needinfo?(michael)
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-
Keywords: dev-doc-needed
Hi Michael, I can confirm that it works as expected with the patch in bug 1304073 applied. Thanks!
Attachment #8795451 - Flags: feedback?(mdeboer) → feedback+

Comment 16

8 months ago
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).
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
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

8 months ago
Created attachment 8800741 [details] [diff] [review]
Enable selection events for documents with the System Principal

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

Updated

8 months ago
Attachment #8795451 - Attachment is obsolete: true
(Assignee)

Comment 20

8 months ago
I had already written up a fixed up patch, but hadn't pushed it yet.
Flags: needinfo?(michael)
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

8 months 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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9d87bca44d8
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → michael
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.