Closed Bug 571294 Opened 10 years ago Closed 4 years ago

Add selection events

Categories

(Core :: DOM: Selection, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jrmuizel, Assigned: Nika)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 9 obsolete files)

11.83 KB, patch
Details | Diff | Splinter Review
43.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
It would be quite useful to have events for selection change. Editor's like FCK emulate this by traping mouseup and keypress, using a timer and checking the selection.

IE has a onselectionchange event that is used for this purpose.

We should try to come up with a set of requirements for what's needed here.
Hmm, so what all events are there?
select, selectstart, selectionchange?
How are those all related?

Does anyone know if they are documented well anywhere?
They certainly aren't standardized.
Based on Tim's email to whatwg, IE's behavior seems to be the only reasonable
one.
Component: DOM: Events → Selection
QA Contact: events → selection
It seems a mess to me. Inconsistent implementations, no spec and probably more events than necessary. I was hoping someone (Aryeh Gregor?) would offer to spec it as part of the Range/Selection spec.

I don't have a strong opinion about which of the existing event(s) are implemented. What IE does that no other browser does is fire an event whenever the selection changes by whatever means (mouse, keyboard, menu option), and that's all I'm really interested in. Ryosuke mentioned on the whatwg list that WebKit now has onselectionchange, which sounds as good as any to me.
IE, Chrome and Safari have selectstart.
It is very useful if you want disable user selection.
It is bubbles and cancelable.
The relevant W3C spec bug is https://www.w3.org/Bugs/Public/show_bug.cgi?id=13952 — what is needed to get Gecko interested in this? If it's just waiting for someone to spec out existing behavior, I'd be interested to figure out how various browsers work and may even have a few hours of funding available via a client if I can convince them that it could lead to Firefox supporting this in the near future :-)
(In reply to Olli Pettay [:smaug] from comment #2)
> Based on Tim's email to whatwg, IE's behavior seems to be the only reasonable
> one.

For those following along at home, I presume http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031493.html is "Tim's email to the whatwg". The situation has advanced some since that, but TBH I haven't yet investigated the WebKit selectionchange addition because it's somewhat for naught if Firefox offers nothing of the sort — best case I have to implement the "nice" way AND the "workaround" way so I've stuck with polling on all browsers for now. (Use case is an e-reader which provides an interface in response to user highlights.)
This is something that I've wanted to see in Gecko for a long time, so if there's a good spec, we're willing to implement it.
Just in case this might help: My use case is that I want to highlight the block or inline tag that contains the user's input cursor inside a containing contenteditable div as the cursor moves around. Or I want to update a breadcrumb widget. Or if a user selects some text I want to display some styling/highlighting controls right next to the selection, so if the selection grows or shrinks, the controls will move to stay positioned. This is trivial to do with a document.onselectionchange, but I have to resort to inefficient polling in Firefox.
My use case is just like Le Roux’s (previous comment), but for an addon. selectionchange would be very useful for that. For now I have cobbled something together using mouseup, keypress, and timers, as FCK does.

I think selectstart is less urgent for a few reasons:
- There is already an event that fires when a selection begins (mousedown)
- Some consider its most common use case (preventing user selection) “evil”
  https://bugzilla.mozilla.org/show_bug.cgi?id=688599#c5
- CSS provides another way to prevent selection

The primary benefit of supporting selectstart at this point that I can see is providing a way to cancel the start of a selection without preventing a change of focus (which canceling mousedown also does).

Given that firefox already supports the select event, perhaps the scope of this bug should be decreased to adding support for selectionchange, or a new more specific issue could be filed for that.

Here is a link to the webkit issue for selectionchange, in case some of the discussion there might be helpful.
https://bugs.webkit.org/show_bug.cgi?id=45712
Here is a link to my polyfill, in case anyone might find it helpful.
https://github.com/2is10/selectionchange-polyfill/

I’ve listed all mechanisms I know of that can create or alter a selection in the README.
This W3C specification now describes the selectstart and selectionchange events.
http://w3c.github.io/selection-api/#user-interactions

It’s currently in “Unofficial Draft” status. Good enough to implement, Ehsan?
Flags: needinfo?(ehsan.akhgari)
(In reply to Jared Jacobs from comment #12)
> This W3C specification now describes the selectstart and selectionchange
> events.
> http://w3c.github.io/selection-api/#user-interactions
> 
> It’s currently in “Unofficial Draft” status. Good enough to implement, Ehsan?

Ryosuke, how stable is the current spec?  Is it ready for implementation?

I think it lacks some details.  For example, which node are we supposed to fire the selectstart event on when the current selection does not contain any ranges?  Also, are these events simple events or do they have their own event interfaces etc.?
Flags: needinfo?(ehsan.akhgari) → needinfo?(rniwa)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #13)
> (In reply to Jared Jacobs from comment #12)
> > This W3C specification now describes the selectstart and selectionchange
> > events.
> > http://w3c.github.io/selection-api/#user-interactions
> > 
> > It’s currently in “Unofficial Draft” status. Good enough to implement, Ehsan?
> 
> Ryosuke, how stable is the current spec?  Is it ready for implementation?

We haven't even published the first working draft, so it's probably not ready for implementations to match it but I'd appreciate if you could give us feedback.

> I think it lacks some details.  For example, which node are we supposed to
> fire the selectstart event on when the current selection does not contain
> any ranges?  Also, are these events simple events or do they have their own
> event interfaces etc.?

Sorry, that was poorly worded.  selectstart shouldn't fire for non-empty selection.
I've tried to clarify it in https://github.com/w3c/selection-api/commit/b9a773099b6b3ad816ef6873c60f47ee0ee48fdc.

These events are simple and it's implied by the concept fire in DOM4:
http://www.w3.org/TR/dom/#concept-event-fire

Please feel free to reopen the bug or file a new one if you feel my response was insufficient.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(rniwa)
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Oops, sorry, I thought this was a W3C bug on my spec.  It's a Gecko bug!
Sounds good!  I think we can start to implement if we find someone who is interested in writing the code!  If we hit other spec issues, we can always file them and give feedback as we go.
Assignee: nobody → michael
Depends on: 1193988
This is a WIP patch, I haven't gotten very thurough tests done yet (as in any tests), and there are still lots of problems with overzealous selectionchanged notifications, but it's getting there.

This patch fires the selectionstart notification when a user initiated event causes the selection to change from empty (no regions or a single collapsed region) to any other state. Canceling this event will cause the selection not to be changed.
Depends on: 1194396
This is the current patch I have going for the selection events.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=188e846a82b8
Attachment #8647204 - Attachment is obsolete: true
Attachment #8648101 - Flags: review?(ehsan)
There is one todo test in this patch, because I am not sure if we should be firing selectionchange when the user mutates the range object which is currently attached to the document's selection. We can't follow chrome on this, as they pass ranges by value, and the spec isn't super clear on this (IMO).

I've opened https://github.com/w3c/selection-api/issues/52 on the spec about it.
Attachment #8648102 - Flags: review?(ehsan)
Attachment #8648101 - Flags: review?(ehsan)
Attachment #8648102 - Flags: review?(ehsan)
Attachment #8648102 - Attachment is obsolete: true
Attachment #8648263 - Flags: review?(ehsan)
Should fix use after free crashes in the previous try run.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ce83c4af52
Attachment #8648262 - Attachment is obsolete: true
Attachment #8648262 - Flags: review?(ehsan)
Attachment #8648740 - Flags: review?(ehsan)
Comment on attachment 8648263 [details] [diff] [review]
Part 2: Tests for new select event behaviour

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

::: dom/tests/mochitest/general/frameSelectEvents.html
@@ +38,5 @@
> +
> +        var selectstart = 0;
> +        var selectionchange = 0;
> +        var cancel = false;
> +        document.addEventListener('selectstart', function(aEvent) {

Can you also ensure that these events are dispatched to the correct original target?  You can use the Event.originalTarget property for that.

@@ +79,5 @@
> +        function isCollapsed() { is(selection.isCollapsed, true, "Selection is collapsed"); }
> +        function isNotCollapsed() { is(selection.isCollapsed, false, "Selection is not collapsed"); }
> +
> +        // Focus the contenteditable text
> +        yield* mouseAction(elt("ce"), 100, undefined, 0, 1);

Nit: passing "click" instead of undefined would probably make this more readable, both here and below.
Attachment #8648263 - Flags: review?(ehsan) → review+
Comment on attachment 8648740 [details] [diff] [review]
Part 1: Implement selection events behind the dom.select_events.enabled pref

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

Looks good overall.  I have several minor comments below.  I'd like to get smaug's review on both the event handling pieces and the selection pieces since he will be back tomorrow and he knows both areas.

::: dom/base/nsGkAtomList.h
@@ +2377,5 @@
>  #endif
>  
>  GK_ATOM(vr_state, "vr-state")
> +GK_ATOM(onselectstart, "onselectstart")
> +GK_ATOM(onselectionchange, "onselectionchange")

Can you please move these two above into the large section for on* names, maintaining the sort order?

::: dom/base/nsRange.h
@@ +20,5 @@
>  #include "prmon.h"
>  #include "nsStubMutationObserver.h"
>  #include "nsWrapperCache.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/dom/Selection.h"

Please don't include this header inside another header.  ~nsRange is out of line so you should be able to get away with forward declaring this in the header.

@@ +148,5 @@
> +    // and a range can't be in more than one selection at a time,
> +    // thus mSelection must be null too.
> +    MOZ_ASSERT(!aSelection || !mSelection);
> +
> +    mSelection = aSelection;

As per above, if this assignment requires Selection.h, it may be better to un-inline this function into the .cpp file.

::: layout/generic/nsSelection.cpp
@@ +6265,5 @@
> +  mStartParent = aRange->GetStartContainer(rv);
> +  mEndParent = aRange->GetEndContainer(rv);
> +  mStartOffset = aRange->GetStartOffset(rv);
> +  mEndOffset = aRange->GetEndOffset(rv);
> +  rv.SuppressException();

I think you should do this after every call where you use rv above.

@@ +6276,5 @@
> +    mStartParent == aRange->GetStartContainer(rv) &&
> +    mEndParent == aRange->GetEndContainer(rv) &&
> +    mStartOffset == aRange->GetStartOffset(rv) &&
> +    mEndOffset == aRange->GetEndOffset(rv);
> +  rv.SuppressException();

Ditto.

@@ +6290,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsSelectionChangeListener)
> +  {
> +    for (uint32_t i = 0; i < tmp->mOldRanges.Length(); ++i) {
> +      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOldRanges[i].mStartParent);
> +      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOldRanges[i].mEndParent);

It would be nice if you could add an overload for ImplCycleCollectionTraverse on the RawRangeData type.  With that, you could replace all of this with

NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOldRanges)

@@ +6296,5 @@
> +  }
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsSelectionChangeListener)
> +  NS_INTERFACE_MAP_ENTRY(nsISelectionListener)

You should also support QIing to nsISupports.

::: layout/generic/nsSelectionChangeListener.h
@@ +26,5 @@
> +  // changed.
> +  struct RawRangeData {
> +    // XXX These nodes are currently forcing nsSelectionChangeListener to participate
> +    // in cycle collection, but they are never dereferenced. Do they need to be held as
> +    // strong references? or could we use weak references here?

If you just need them for pointer comparison, you can probably store a void* instead.
Attachment #8648740 - Flags: review?(ehsan) → feedback+
Attachment #8648740 - Attachment is obsolete: true
Attachment #8649290 - Flags: review?(bugs)
I couldn't seem to convince b2g to treat mouse movement as selection, so the test continues to fail on b2g. I've disabled it on b2g for now.
Attachment #8648263 - Attachment is obsolete: true
What spec defines that there should be
attribute EventHandler onselectionchange;
on the Document, but not on Window or Elements?
(In reply to Olli Pettay [:smaug] from comment #31)
> What spec defines that there should be
> attribute EventHandler onselectionchange;
> on the Document, but not on Window or Elements?

As you noticed & filed an issue for (I thought ehsan or I had already filed that, but apparently not :S), the spec doesn't define where the onselectionchange attribute should be present. As right now the event is always fired on the document object, I only attached the attribute to the document object, but once the spec contains the relevant information, it should be easy to change.
I had filed https://github.com/w3c/selection-api/issues/54.  That definitely blocks turning the pref on by default, but I think we can land what Michael has for now.
And since selectionchange doesn't bubble, we don't need event handler on the Window object... ok.

(will review the patch while going through the review queue)
(In reply to Olli Pettay [:smaug] from comment #35)
> And since selectionchange doesn't bubble, we don't need event handler on the
> Window object... ok.

Yep, that's the idea.
Blocks: 1196479
Comment on attachment 8649290 [details] [diff] [review]
Part 1: Implement selection events behind the dom.select_events.enabled pref

>+nsRange::nsRange(nsINode* aNode)
Don't know why you removed ctor to .cpp. But fine I guess.
>+  // Notify selection listeners
>+  if (mSelection) {
>+    mSelection->NotifySelectionListeners();
>+  }
>+
>   // This needs to be the last thing this function does.  See comment
>   // in ParentChainChanged.
>   mRoot = aRoot;

This looks a bit scary.
What if some selection listener ends up modifying the range. mRoot might get set to wrong value.
So, change the comment a bit and move NotifySelectionListeners call to happen after we have a new root.


>+++ b/layout/generic/nsFrameSelection.h
>@@ -747,11 +747,12 @@ private:
>   bool mChangesDuringBatching;
>   bool mNotifyFrames;
>   bool mDragSelectingCells;
>   bool mDragState;   //for drag purposes
>   bool mMouseDoubleDownState; //has the doubleclick down happened
>   bool mDesiredPosSet;
> 
>   int8_t mCaretMovementStyle;
>+  bool mSelectionEvents;
What does this member variable mean?
Perhaps call it mSelectionEventsEnabled


>+    bool newRangesNonempty = rangesToAdd.Length() > 1 ||
>+      (rangesToAdd.Length() == 1 && !rangesToAdd[0]->Collapsed());
>+    if (!aNoStartSelect && mType == nsISelectionController::SELECTION_NORMAL &&
>+        mFrameSelection->mSelectionEvents && Collapsed() && newRangesNonempty &&
>+        nsContentUtils::IsSafeToRunScript()) {
>+      // We consider a selection to be starting if we are currently collapsed,
>+      // and the selection is becoming uncollapsed, and this is caused by a user
>+      // initiated event.
>+      bool defaultAction = true;
>+      nsContentUtils::DispatchTrustedEvent(GetParentObject(),
>+                                           aItem->GetStartParent(),
>+                                           NS_LITERAL_STRING("selectstart"),
>+                                           true, true, &defaultAction);
>+
>+      if (!defaultAction) {
>+        return NS_OK;
>+      }
>+    }
>+
>     size_t newAnchorFocusIndex =
>       GetDirection() == eDirPrevious ? 0 : rangesToAdd.Length() - 1;
>     for (size_t i = 0; i < rangesToAdd.Length(); ++i) {

This looks scary. Remember, dispatching an event may destroy worlds.
So nothing guarantees that rangesToAdd points to any kind of valid ranges anymore, and if I read
AddItemInternal correctly that means that we may for example add ranges pointing to some other document to mRanges.
>+nsSelectionChangeListener::RawRangeData::RawRangeData(const nsRange* aRange) {
{ should be on its own line

>+nsSelectionChangeListener::RawRangeData::Equals(const nsRange* aRange) {
ditto

>+nsSelectionChangeListener::NotifySelectionChanged(nsIDOMDocument *aDoc,
>+                                                  nsISelection *aSel, int16_t aReason)
* goes with the type, not with variable name


>+{
>+  // This cast is valid as nsISelection is a builtinclass which is only
>+  // implemented by Selection.
>+  nsRefPtr<Selection> sel = static_cast<Selection *>(aSel);
extra space before *

>diff --git a/layout/generic/nsSelectionChangeListener.h b/layout/generic/nsSelectionChangeListener.h
I wouldn't use ns-prefix for the filename, nor classname

>+  struct RawRangeData {
{ to its own line


r- mainly because of the "selectstart" dispatch. It is not quite clear to me what all needs to be validated after the event dispatch, but better to test a bit other browser.
(and I wouldn't be surprised if you find crashers in other browsers while testing this).
Attachment #8649290 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (high review load) from comment #37)
> >+nsRange::nsRange(nsINode* aNode)
> Don't know why you removed ctor to .cpp. But fine I guess.
I don't remember why I did either - I assume it was because at some point added a field which didn't have it's constructor declared in the header... I'll try moving it out again.

> This looks a bit scary.
> What if some selection listener ends up modifying the range. mRoot might get
> set to wrong value.
> So, change the comment a bit and move NotifySelectionListeners call to
> happen after we have a new root.
Sounds good & makes sense.

> This looks scary. Remember, dispatching an event may destroy worlds.
> So nothing guarantees that rangesToAdd points to any kind of valid ranges
> anymore, and if I read
> AddItemInternal correctly that means that we may for example add ranges
> pointing to some other document to mRanges.

:S - I don't really know what the best way to deal with that would be... I'll look into it.

> r- mainly because of the "selectstart" dispatch. It is not quite clear to me
> what all needs to be validated after the event dispatch, but better to test
> a bit other browser.
> (and I wouldn't be surprised if you find crashers in other browsers while
> testing this).
Updated in response to review comments. I now re-do the work I had done previously in the AddItem method in case the DOM has changed and that work is now invalid. This will stop, for example, a selection being started on a node, and that node becoming user-select: none in the selectstart callback, and the selection still being created.
Attachment #8649290 - Attachment is obsolete: true
Attachment #8652909 - Flags: review?(bugs)
Comment on attachment 8652909 [details] [diff] [review]
Part 1: Implement selection events behind the dom.select_events.enabled pref

>+nsRange::SetSelection(mozilla::dom::Selection* aSelection)
>+{
>+  if (mSelection == aSelection) return;
always {} with if and return goes to its own line

>+  void UserSelectRangesToAdd(nsRange* aItem, nsTArray<nsRefPtr<nsRange> >& rangesToAdd);
s/> >/>>/
and aFoo for arguments.

>   /**
>    * True if the current selection operation was initiated by user action.
>-   * It determines whether we exclude -moz-user-select:none nodes or not.
>+   * It determines whether we exclude -moz-user-select:none nodes or not,
>+   * as well as whether selectstart events will be fired.
>    */
>   bool mApplyUserSelectStyle;
Please file a followup to change the name of this member variable.


> nsFrameSelection::Init(nsIPresShell *aShell, nsIContent *aLimiter)
> {
>   mShell = aShell;
>   mDragState = false;
>   mDesiredPosSet = false;
>   mLimiter = aLimiter;
>   mCaretMovementStyle =
>     Preferences::GetInt("bidi.edit.caret_movement_style", 2);
>+  mSelectionEventsEnabled =
>+    Preferences::GetBool("dom.select_events.enabled", false);
I would actually just add a static nsFrameSelection::sSelectionEventsEnabled and then just use
Preferences::AddBoolVarCache to initialized when first nsFrameSelection is created.
(You need to add a static variable to ::Init to protect that AddBoolVarCache is called only once.)


>+Selection::UserSelectRangesToAdd(nsRange* aItem, nsTArray<nsRefPtr<nsRange> >& rangesToAdd)
>> and aFoo

>+    bool newRangesNonempty = rangesToAdd.Length() > 1 ||
>+      (rangesToAdd.Length() == 1 && !rangesToAdd[0]->Collapsed());
>+    if (!aNoStartSelect && mType == nsISelectionController::SELECTION_NORMAL &&
>+        mFrameSelection->mSelectionEventsEnabled && Collapsed() && newRangesNonempty &&
>+        nsContentUtils::IsSafeToRunScript()) {
Could you perhaps move nsContentUtils::IsSafeToRunScript() to its own 'if', and MOZ_ASSERT(nsContentUtils::IsSafeToRunScript());
before it, since we clearly miss an event if it isn't safe to run scripts. And that assertion is hit, need to figure out how to run this stuff differently.



>+      // As we just dispatched an event to the DOM, something could have changed
>+      // under our feet. Re-generate the rangesToAdd array, and ensure that the
>+      // range we are about to add is still valid.
>+      if (!aItem->IsPositioned()) {
>+        return NS_ERROR_UNEXPECTED;
>+      }
>+      rangesToAdd.ClearAndRetainStorage();
>+      UserSelectRangesToAdd(aItem, rangesToAdd);
I guess this is somewhat reasonable :)

>+SelectionChangeListener::NotifySelectionChanged(nsIDOMDocument* aDoc,
>+                                                  nsISelection* aSel, int16_t aReason)
indentation of the 2nd argument


>+// selection events
>+#define NS_SELECT_EVENT_START    6200
>+#define NS_SELECT_START          (NS_SELECT_EVENT_START)
>+#define NS_SELECTION_CHANGE      (NS_SELECT_EVENT_START+1)
This stuff was just changed, but the merge should be trivial
Attachment #8652909 - Flags: review?(bugs) → review+
Blocks: 1199288
I just got around to running try on this again after making the changes.

Unfortunately, it seems as though nsRange::ExcludeNonSelectableNodes() mutates the range it is called on, which means that my idea of re-generating the range probably won't work :-/. My first idea would be to run ExcludeNonSelectableNodes on every node in the output array, and add every item in each of the resulting arrays, but that might not be the best option :-/

Secondly, I added the nsContentUtils::IsSafeToRunScript() assertion to the selectstart dispatch location, and it's definitely being hit. I'm not sure exactly where or why it's being hit. You can see the crash in this log: https://treeherder.mozilla.org/logviewer.html#?job_id=10818451&repo=try.

There are also other random test failures (also in the above log) which are being run into with the new changes. I'm not sure why those particular failures are occurring, but I will try to look into it soon.
I think this one might actually pass tests. I'd really like it if you'd take a look at it again, so I'm flagging you for review again.
Attachment #8652909 - Attachment is obsolete: true
Attachment #8655043 - Flags: review?(bugs)
Comment on attachment 8655043 [details] [diff] [review]
Part 1: Implement selection events behind the dom.select_events.enabled pref

>+++ b/editor/libeditor/nsTextEditRules.cpp
>@@ -826,16 +826,17 @@ nsTextEditRules::WillDeleteSelection(Selection* aSelection,
> 
>   // if there is only bogus content, cancel the operation
>   if (mBogusNode) {
>     *aCancel = true;
>     return NS_OK;
>   }
> 
>   nsresult res = NS_OK;
>+  AutoHideSelectionChanges hideSelection(aSelection);
I don't know what AutoHideSelectionChanges is. It isn't defined anywhere, as far as I see.

>+    nsAutoTArray<nsRefPtr<nsRange>, 4> ranges;
>     size_t newAnchorFocusIndex =
>       GetDirection() == eDirPrevious ? 0 : rangesToAdd.Length() - 1;
>     for (size_t i = 0; i < rangesToAdd.Length(); ++i) {
>-      int32_t index;
>-      nsresult rv = AddItemInternal(rangesToAdd[i], &index);
>-      NS_ENSURE_SUCCESS(rv, rv);
>-      if (i == newAnchorFocusIndex) {
>-        *aOutIndex = index;
>-        rangesToAdd[i]->SetIsGenerated(false);
>+      // Check if any of these ranges are overlapping with user-select: none
>+      // areas again. This could have changed due to an event. If
>+      // regenerateRanges is false, no event was fired, so we're OK.
>+      ranges.ClearAndRetainStorage();
>+      if (regenerateRanges) {
>+        UserSelectRangesToAdd(rangesToAdd[i], ranges);
>       } else {
>-        rangesToAdd[i]->SetIsGenerated(true);
>+        ranges.AppendElement(rangesToAdd[i]);
>+      }
>+
>+      size_t newAnchorFocusSubIndex =
>+        GetDirection() == eDirPrevious ? 0 : ranges.Length() - 1;
>+      for (size_t j = 0; j < ranges.Length(); ++j) {
>+        int32_t index;
>+        nsresult rv = AddItemInternal(ranges[j], &index);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        if (i == newAnchorFocusIndex && j == newAnchorFocusSubIndex) {
>+          *aOutIndex = index;
>+          ranges[j]->SetIsGenerated(false);
>+        } else {
>+          ranges[j]->SetIsGenerated(true);
>+        }
This setup needs some comment. Why couldn't we clone the initial range and then re-call UserSelectRangesToAdd or something?
Attachment #8655043 - Flags: review?(bugs) → review-
Attachment #8656222 - Flags: review?(bugs)
Comment on attachment 8656222 [details] [diff] [review]
Part 1: Implement selection events behind the dom.select_events.enabled pref

>+++ b/extensions/spellcheck/src/mozInlineSpellChecker.cpp
>@@ -941,23 +941,27 @@ mozInlineSpellChecker::ReplaceWord(nsIDOMNode *aNode, int32_t aOffset,
>   NS_ENSURE_TRUE(newword.Length() != 0, NS_ERROR_FAILURE);
> 
>   nsCOMPtr<nsIDOMRange> range;
>   nsresult res = GetMisspelledWord(aNode, aOffset, getter_AddRefs(range));
>   NS_ENSURE_SUCCESS(res, res); 
> 
>   if (range)
>   {
>+    nsCOMPtr<nsIDOMRange> editorRange;
>+    res = range->CloneRange(getter_AddRefs(editorRange));
Please add a short comment why we're doing the cloneRange here.


>+++ b/editor/libeditor/nsTextEditRules.cpp
>@@ -826,16 +826,17 @@ nsTextEditRules::WillDeleteSelection(Selection* aSelection,
> 
>   // if there is only bogus content, cancel the operation
>   if (mBogusNode) {
>     *aCancel = true;
>     return NS_OK;
>   }
> 
>   nsresult res = NS_OK;
>+  AutoHideSelectionChanges hideSelection(aSelection);
This could also use a comment, explaining the case where we don't want selectstart
(that backspace case)
Attachment #8656222 - Flags: review?(bugs) → review+
Depends on: 1204291
https://hg.mozilla.org/mozilla-central/rev/24515d221f0a
https://hg.mozilla.org/mozilla-central/rev/41ffed8cc5d3
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Yes, Document.onselectstart & GlobalEventHandlers.onselectionchange should be added to the spec. I *think* both IE and WebKit/Blink support that already so it's a spec bug.
Flags: needinfo?(rniwa)
Done: https://github.com/w3c/selection-api/issues/60

Note that Gecko is the only browser to implement onselectstart on GEH. Both Blink and Edge have it on Document (that implement GEH in Gecko too).
That's an uncanny difference. I suspect we should match Blink/Edge behaviors since they have been shipping that way for a while as far as I know (assuming Edge is matching the old Trident behavior).
If/when our behavior change, please add dev-doc-need on this bug/or the other bug :-) It will put it on the doc team radar :-)
(In reply to Jean-Yves Perrier [:teoli] from comment #53)
> Done: https://github.com/w3c/selection-api/issues/60
> 
> Note that Gecko is the only browser to implement onselectstart on GEH. Both
> Blink and Edge have it on Document (that implement GEH in Gecko too).

The reason for the extra onselectionchange handlers is bug 1196479 - the specced behavior is very weird on it's interaction with input fields, see https://github.com/w3c/selection-api/issues/53.
Blocks: 1214446
Hey. Thanks for a great work! Good to see contentEditable-related stuff slowly getting standardised across the browsers.

When do you plan to enable selection events by default? Actually, I'm more interested about selectionchange as this one is already widely supported by other browsers (except Safari ;().
I see that there's an attempt to enable selection events outside of nightly: https://bugzilla.mozilla.org/show_bug.cgi?id=1242718. Great news for us! :)
Blocks: 1309612
Depends on: 1350972
You need to log in before you can comment on or make changes to this bug.