Closed
Bug 1231923
Opened 9 years ago
Closed 8 years ago
Enable select events for chrome consumers
Categories
(Core :: DOM: Selection, defect)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
At some point, we should flip the pref outside of nightly.
Comment 2•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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•8 years ago
|
||
selection.modify() is not related to this per se, we can track those changes independently.
Assignee: nobody → ehsan
Comment 6•8 years ago
|
||
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•8 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 | ||
Comment 9•8 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•8 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.
Comment 11•8 years ago
|
||
(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.
Assignee | ||
Comment 13•8 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•8 years ago
|
Flags: needinfo?(michael)
Comment 14•8 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-
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 15•8 years ago
|
||
Hi Michael, I can confirm that it works as expected with the patch in bug 1304073 applied. Thanks!
Updated•8 years ago
|
Attachment #8795451 -
Flags: feedback?(mdeboer) → feedback+
Comment 16•8 years 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).
Comment 17•8 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•8 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•8 years ago
|
||
Updated version in response to review requests
Attachment #8800741 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Attachment #8795451 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 years ago
|
||
I had already written up a fixed up patch, but hadn't pushed it yet.
Flags: needinfo?(michael)
Comment 21•8 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•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Assignee: nobody → michael
Comment 24•8 years ago
|
||
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.
Description
•