1065955 - [Text Selection]Filter the collapsed Selection change events

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[(Inactive after June) George Duan [:gduan] [:喬智]]

This bug is a follow-up of bug 1020801.

We should update the title once bug 1020801 is merged and identify what should be really fixed here.

Comment 1

[(Inactive after June) George Duan [:gduan] [:喬智]]

Here's what I found when testing the patch from bug 1020801


STR:
1. long press some characters, and it will show dialog, and remove finger
2. touch the screen again, and start to scroll
3. stop scrolling and remove finger

Log between step 1 and 2 is as below,

E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:199 in tsd_show: {"rect":{"width":58.66667175292969,"height":14.666671752929688,"top":76.33332824707031,"bottom":91,"left":112.33332824707031,"right":171},"commands":{"canSelectAll":true,"canCut":true,"canCopy":true,"canPaste":false},"zoomFactor":1,"reasons":["mouseup"],"isCollapsed":false,"msg_name":"selectionchange","offsetX":0,"offsetY":30}

Log between step 2 and 3 is as below,

E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:74 in tsd_handleEvent: selectionchange
E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:199 in tsd_show: {"rect":{"width":320,"height":303.33331298828125,"top":30,"bottom":333.33331298828125,"left":0,"right":320},"commands":{"canSelectAll":true,"canCut":false,"canCopy":true,"canPaste":false},"zoomFactor":1,"reasons":["drag","mousedown"],"isCollapsed":true,"msg_name":"selectionchange","offsetX":0,"offsetY":0}
E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:74 in tsd_handleEvent: selectionchange
E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:199 in tsd_show: {"rect":{"width":320,"height":303.33331298828125,"top":30,"bottom":333.33331298828125,"left":0,"right":320},"commands":{"canSelectAll":true,"canCut":false,"canCopy":true,"canPaste":false},"zoomFactor":1,"reasons":[],"isCollapsed":true,"msg_name":"selectionchange","offsetX":0,"offsetY":0}
E/GeckoConsole(  292): Content JS LOG at app://system.gaiamobile.org/js/text_selection_dialog.js:74 in tsd_handleEvent: scrollviewchange


1. the value of isCollapsed are different
"isCollapsed":false
"isCollapsed":true

I believe it should be always true, since the selected area I saw on the screen is not collapsed after scrolling.

2. commands are different
{"canSelectAll":true,"canCut":true,"canCopy":true,"canPaste":false}
{"canSelectAll":true,"canCut":false,"canCopy":true,"canPaste":false}

I think they should be the same, or some button might be not workable after scrolling

3. rects are different
{"rect":{"width":58.66667175292969,"height":14.666671752929688,"top":76.33332824707031,"bottom":91,"left":112.33332824707031,"right":171}
{"rect":{"width":320,"height":303.33331298828125,"top":30,"bottom":333.33331298828125,"left":0,"right":320}

with and height should be the same due to same selected area. and top/bottom/left/right should changed by scrolling.


4. tsd_handleEvent: scrollviewchange happen after selectionchange
It makes text_seleciton_dialog.js updated with incorrect message of above items.




We should have way to filter the selectionchange event when user start to scroll and make sure the commands are the same scrolling.

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

And another problem is,
evt.detail.scrollHeight should be yEnd - yStart, right?
I try to scroll from the end of the text, and the scrollHeight is its position y, have I misunderstood something?

Comment 3

[Peter Chang[:pchang]]

Based on comment 1, gecko sends out the following selectoinchange to gaia during the scrolling(before AsyncPanZoomStarted callback is received).

I can filter this redundant selectionchange in my local but I will also check the source to trigger this selection change. 
[Log]
09-15 04:53:05.960 I/Gecko   (17861): peter nsDocViewerSelectionListener::NotifySelectionChanged called
09-15 04:53:05.960 I/Gecko   (17861): peter UpdateCommands rect 0 30 320 303.333 reason 0x00000003
09-15 04:53:05.970 I/Gecko   (17861): peter send selectionchange event top 30 bottom 333.33331298828125 left 0 right 320 content.screen.width 320 innerWidth 320 content.screen.height 569
09-15 04:53:05.970 I/Gecko   (17861): peter nsDocViewerSelectionListener::NotifySelectionChanged called
09-15 04:53:05.970 I/Gecko   (17861): peter UpdateCommands rect 0 30 320 303.333 reason 0x00000000
09-15 04:53:05.970 I/Gecko   (17861): peter send selectionchange event top 30 bottom 333.33331298828125 left 0 right 320 content.screen.width 320 innerWidth 320 content.screen.height 569
09-15 04:53:06.100 I/Gecko   (18245): peter AsyncPanZoomStarted called
09-15 04:53:06.580 I/Gecko   (18245): peter AsyncPanZoomStopped called 0 113 
09-15 04:53:06.580 I/Gecko   (18245): peter AsyncPanZoomStarted called
09-15 04:53:06.590 I/Gecko   (18245): peter AsyncPanZoomStopped called 0 113

Comment 4

[Peter Chang[:pchang]]

It is easy to see the selection change events with no reason, not matter content is scrolled or not.

Comment 5

[Peter Chang[:pchang]]

There is another performance impaction that was caused by too many selection change events.

https://bugzilla.mozilla.org/show_bug.cgi?id=1066515#c43

Comment 6

[Peter Chang[:pchang]]

Attachment: bug_1065955.patch

Bypass the selection events with no reason and keypress reason to system app to avoid performance regression.

Comment 7

[(no longer active)]

Comment on attachment 8492984 [details] [diff] [review]
bug_1065955.patch

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

Instead of hacking something in like this, we should figure out where these events are coming from, and deciding whether we actually want to ignore them.  For example, I don't think ignoring selection changes through key events is the right thing to do.

Comment 8

[Peter Chang[:pchang]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> Comment on attachment 8492984 [details] [diff] [review]
> bug_1065955.patch
> 
> Review of attachment 8492984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of hacking something in like this, we should figure out where these
> events are coming from, and deciding whether we actually want to ignore
> them.  For example, I don't think ignoring selection changes through key
> events is the right thing to do.

Ehsan, there are lots of selection changes event with NO_REASON and KEYPRESS_REASON when choosing the word suggestion from keyboard app in bug 1066515. For the events with keypress reason, they are triggered from [1] and the keypress reason are appended inside nsFrameSelection::MoveCaret.

For FxOS or fennec, I think it is impossible for users to trigger nsFrameSelection::MoveCaret but you can use script to trigger, besides, current the management of selection bubble in gaia doesn't handle the keypress reason.
  
[1]http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js?rev=ae2e4ca015fd#1053
[2]http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp?rev=ae2e4ca015fd#926

Comment 9

[(no longer active)]

(In reply to peter chang[:pchang][:peter] from comment #8)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #7)
> > Comment on attachment 8492984 [details] [diff] [review]
> > bug_1065955.patch
> > 
> > Review of attachment 8492984 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Instead of hacking something in like this, we should figure out where these
> > events are coming from, and deciding whether we actually want to ignore
> > them.  For example, I don't think ignoring selection changes through key
> > events is the right thing to do.
> 
> Ehsan, there are lots of selection changes event with NO_REASON and
> KEYPRESS_REASON when choosing the word suggestion from keyboard app in bug
> 1066515. For the events with keypress reason, they are triggered from [1]
> and the keypress reason are appended inside nsFrameSelection::MoveCaret.

Thanks!  So, this is an API exposed to Web content allowing it to extend the selection.  Your patch breaks updating the selection handles when a web page does this, so it is not acceptable.

That being said, I don't understand why the forms.js code does things that way.  A much better way which would avoid all of these intermediate events would be something like:

sel.collapse(node, 0);
sel.extend(node, start);

or some such.

> For FxOS or fennec, I think it is impossible for users to trigger
> nsFrameSelection::MoveCaret

That function can be called from the Selection.extend method.

> but you can use script to trigger, besides,
> current the management of selection bubble in gaia doesn't handle the
> keypress reason.

Then that is a bug that we should fix as well.

Comment 10

[Peter Chang[:pchang]]

Bug 1066515 was landed but I still could reproduce the low performance issue from message app.
When the low performance happened, the CPU usage of b2g and message app were very high.
And I saw a lot of handleSelectionChange in BrowserElementParent.jsm.

Checking why so many handleSelectionChange happened.

User 60%, System 18%, IOW 0%, IRQ 0%
User 84 + Nice 52 + Sys 42 + Idle 47 + IOW 0 + IRQ 0 + SIRQ 1 = 226

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
19835 19835  1  33% R 230868K  85080K     root     b2g             /system/b2g/b2g
20275 20275  1  24% R 117152K  35944K     u0_a2027 Messages        /system/b2g/b2g
20479 20479  0   5% R   1416K    656K     root     top             top
  279   279  0   3% S   5620K    240K     root     adbd            /sbin/adbd



10-25 22:48:53.662 19835 19835 I Gecko   : peter NotifySelectionChange aReason 0x00000003
10-25 22:48:53.662 19835 19835 I Gecko   : peter NotifySelectionChange aReason 0x00000000
10-25 22:48:53.662 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:53.662 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:53.672 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:53.682 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:53.682 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:53.692 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:53.782 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:53.782 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:53.792 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:53.802 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:53.802 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
...
10-25 22:48:55.432 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.432 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.442 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.442 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.442 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.452 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.452 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.452 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.452 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.462 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.462 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.462 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.462 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.462 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.472 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.472 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.472 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.482 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.482 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.482 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:55.482 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:55.492 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:55.492 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
...
10-25 22:48:58.392 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:58.392 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:58.392 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:58.392 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:58.402 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:58.402 20275 20275 I Gecko   : peter NotifySelectionChange aReason 0x00000008
10-25 22:48:58.402 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:58.402 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:58.402 20275 20275 I Gecko   : peter NotifySelectionChange aReason 0x00000000
10-25 22:48:58.402 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-25 22:48:58.412 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-25 22:48:58.412 19835 19835 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event
10-25 22:48:58.412 20275 20275 I Gecko   : peter NotifySelectionChange aReason 0x00000008

Comment 11

[Peter Chang[:pchang]]

BTW, I tried to use gecko profiler to check the bottleneck and also found many sample time for handleSelectionChange.

Profiler for b2g main thread
http://people.mozilla.org/~bgirard/cleopatra/#report=058905773c0c412d25b75c7b7171700a4fc6b32c

Comment 12

[Peter Chang[:pchang]]

As I expected, the selection events in comment 10 were triggered with "no_resaon" and "keypress" reasons.

10-28 16:09:58.712 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event data{"rect":{"width":0,"height":16.666671752929688,"top":127.5,"bottom":144.1666717529297,"left":51,"right":51},"commands":{"canSelectAll":true,"canCut":false,"canCopy":false,"canPaste":false},"zoomFactor":1,"reasons":[],"isCollapsed":true,"msg_name":"selectionchange"}
10-28 16:09:58.712 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-28 16:09:58.712 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish
10-28 16:09:58.722 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to create event data{"rect":{"width":0,"height":16.666671752929688,"top":127.5,"bottom":144.1666717529297,"left":57,"right":57},"commands":{"canSelectAll":true,"canCut":false,"canCopy":false,"canPaste":false},"zoomFactor":1,"reasons":["keypress"],"isCollapsed":true,"msg_name":"selectionchange"}
10-28 16:09:58.722 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange start to dispatch event
10-28 16:09:58.722 22265 22265 I Gecko   : BrowserElementParent.jsm - handleSelectionChange finish

Comment 13

[Peter Chang[:pchang]]

Attachment: WIP v2

Instead of reducing the selection change events with no_reason and keypress, create this patch to filter the collapsed selection change events.

With this patch, it doesn't cause performance degradation when using word suggestion.

Comment 14

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #13)
> Created attachment 8509332 [details] [diff] [review]
> WIP v2
> 
> Instead of reducing the selection change events with no_reason and keypress,
> create this patch to filter the collapsed selection change events.
> 
> With this patch, it doesn't cause performance degradation when using word
> suggestion.

And this patch could reduce lots of selection change events in comment 10 and comment 12.

Comment 15

[(no longer active)]

Peter, can we please discuss a proper way to fix this bug first?  I hate to keep saying no to your patches after you've spent the time to write them...

I don't think your approach is a good one.  It is focused too much on fixing only one issue, and it's the wrong thing to do because it is crippling the selectionchange event for all other usages.

What is the ultimate source of this bug?  Do we still believe that the look in setSelectionRange inside forms.js is at fault here?  If yes, is it possible to rewrite that code to not modify the selection in a loop like it does today?  If not, I'd be fine with adding a hack which allows that code (and only that code) to suppress sending all of the intermediate selectionchange events, but deciding to not dispatch the events if the selection is collapsed and the touch caret is invisible is too big of a hammer here.

Comment 16

[(no longer active)]

Comment on attachment 8509332 [details] [diff] [review]
WIP v2

Clearing the request for now.

Comment 17

[Peter Chang[:pchang]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> Peter, can we please discuss a proper way to fix this bug first?  I hate to
> keep saying no to your patches after you've spent the time to write them...
> 
I think it's my fault, I should explain more detail about my patch.
> I don't think your approach is a good one.  It is focused too much on fixing
> only one issue, and it's the wrong thing to do because it is crippling the
> selectionchange event for all other usages.
> 
> What is the ultimate source of this bug?  Do we still believe that the look
> in setSelectionRange inside forms.js is at fault here?  If yes, is it
> possible to rewrite that code to not modify the selection in a loop like it
> does today?  If not, I'd be fine with adding a hack which allows that code
> (and only that code) to suppress sending all of the intermediate
> selectionchange events, but deciding to not dispatch the events if the
> selection is collapsed and the touch caret is invisible is too big of a
> hammer here.

With Bug 1066515, it adds the fast path to resolve original problem but it is easy to fall back to original path to hit the low performance problem[1].

Based on my measure in comment 10, the cost of sending DOM events cross IPC is not cheap.
In my opinion, even we fix the code in form.js but there may be another similar use case to hit this low performance problem and we have to turn off selection caret.

That's why I want to fix this problem in gecko side. Does it make sense to you?

After re-checking the code in [2], I could modify attachment 8509332 [details] [diff] [review] not to dispatch the events only when selection collapse flag is not changed and touch caret is invisible.

[1]http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1179
[2]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#3559

Comment 18

[(no longer active)]

(In reply to peter chang[:pchang][:peter] from comment #17)
> With Bug 1066515, it adds the fast path to resolve original problem but it
> is easy to fall back to original path to hit the low performance problem[1].

Oh, I wish I had reviewed that patch. :(

So, I don't think that was the right approach either.  As I said, given that we *know* that this is a performance problem in that code, we should just suppress sending the selection events in that code in general.  We can surround that loop with a suppressSendingSelectionChangeEvents/unsuppressSendingSelectionChangeEvents calls (hopefully you can think of better function names ;) and in UpdateState, check a boolean condition based on that.

> Based on my measure in comment 10, the cost of sending DOM events cross IPC
> is not cheap.
> In my opinion, even we fix the code in form.js but there may be another
> similar use case to hit this low performance problem and we have to turn off
> selection caret.
> 
> That's why I want to fix this problem in gecko side. Does it make sense to
> you?

Well, sure.  But the question is, should we cripple the event while doing so?  That is what I would like us to avoid.

> After re-checking the code in [2], I could modify attachment 8509332 [details] [diff] [review]
> [details] [diff] [review] not to dispatch the events only when selection
> collapse flag is not changed and touch caret is invisible.
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.
> js#1179
> [2]http://dxr.mozilla.org/mozilla-central/source/layout/base/
> nsDocumentViewer.cpp#3559

The select event is used to update parts of the UI that should be activated if there is *any* non-collapsed selection, so in that sense, dispatching that event *only* when we go from collapsed to non-collapsed and vice versa makes sense.  However, the selectionchange at least as I originally intended it is used for notifying about any changes to the selection.  So if we for example want to build some kind of UI which should respond to actual selection changes, doing what you suggest will make this event useless for that purpose.

But perhaps I'm stressing over the generality side too much.  If you really think there's going to be 0 use cases for a generic selection change event, perhaps we should give up on making one for now.  But I think we should at least rename the event to something that doesn't suggest that it works for selection changes...

Comment 19

[Peter Chang[:pchang]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18)
> (In reply to peter chang[:pchang][:peter] from comment #17)
> > With Bug 1066515, it adds the fast path to resolve original problem but it
> > is easy to fall back to original path to hit the low performance problem[1].
> 
> Oh, I wish I had reviewed that patch. :(
> 
> So, I don't think that was the right approach either.  As I said, given that
> we *know* that this is a performance problem in that code, we should just
> suppress sending the selection events in that code in general.  We can
> surround that loop with a
> suppressSendingSelectionChangeEvents/unsuppressSendingSelectionChangeEvents
> calls (hopefully you can think of better function names ;) and in
> UpdateState, check a boolean condition based on that.
> 
I think this suggestion is good and create bug 1089553 to follow up.
> > Based on my measure in comment 10, the cost of sending DOM events cross IPC
> > is not cheap.
> > In my opinion, even we fix the code in form.js but there may be another
> > similar use case to hit this low performance problem and we have to turn off
> > selection caret.
> > 
> > That's why I want to fix this problem in gecko side. Does it make sense to
> > you?
> 
> Well, sure.  But the question is, should we cripple the event while doing
> so?  That is what I would like us to avoid.
> 
> > After re-checking the code in [2], I could modify attachment 8509332 [details] [diff] [review]
> > [details] [diff] [review] not to dispatch the events only when selection
> > collapse flag is not changed and touch caret is invisible.
> > 
In my local testing, it caused trobule if we only dispatch selection change events when collapse state changed because there are several selection events which are not collapsed but with different reasons. Copypaste bubble needs those events to display the bubble.

> > [1]http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.
> > js#1179
> > [2]http://dxr.mozilla.org/mozilla-central/source/layout/base/
> > nsDocumentViewer.cpp#3559
> 
> The select event is used to update parts of the UI that should be activated
> if there is *any* non-collapsed selection, so in that sense, dispatching
> that event *only* when we go from collapsed to non-collapsed and vice versa
> makes sense.  However, the selectionchange at least as I originally intended
> it is used for notifying about any changes to the selection.  So if we for
> example want to build some kind of UI which should respond to actual
> selection changes, doing what you suggest will make this event useless for
> that purpose.
> 
> But perhaps I'm stressing over the generality side too much.  If you really
> think there's going to be 0 use cases for a generic selection change event,
> perhaps we should give up on making one for now.  But I think we should at
> least rename the event to something that doesn't suggest that it works for
> selection changes...

Right now the 'selectionchange' event here is used to notify the copypaste bubble and it works fine if we filter the collapsed selection events. And I still perfer to prevent sending lots of selection change events in gecko to prevent another similar use case to hit the performance problem. 

As you mention above, the meaning of 'selectionchange' implies we need to notify the copypaste bubble for any selection changes. But actually the copypaste bubble is not interested at those collapsed events, they also have some event filter logic. Therefore, I suggest to change the 'selectionchange' event to 'copypaste' event. Since this modifcation requires lots of file changes, I would prefer to fix the naming issue in another. Ehsan, please let me know if you prefer to fix inside this bug.

Comment 20

[(no longer active)]

OK, let's rename the event and tailor it to the copy/paste bubble use case.  I'll leave it up to you which bug to do the work in.  :-)

Comment 21

[Peter Chang[:pchang]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #20)
> OK, let's rename the event and tailor it to the copy/paste bubble use case. 
> I'll leave it up to you which bug to do the work in.  :-)

I just create the bug 1090008 to rename the event.

Comment 22

[Peter Chang[:pchang]]

Attachment: WIP v3

Filter collapsed selection change events but exclude the shortcut mode that the touch caret is visible.

As I mention in comment 19, it caused some problem if we dispatch the selection chagne events when collapse state changed like select event[1] did. Therefore, the patch only targets at the collapsed selection change events.


[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?from=nsDocumentViewer.cpp#3559

Comment 23

[(no longer active)]

Comment on attachment 8512412 [details] [diff] [review]
WIP v3

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

::: dom/base/nsGlobalWindow.cpp
@@ +9357,5 @@
>      init.mBubbles = true;
>      if (aSel) {
> +      bool isCollapsed = false;
> +      bool isTouchCaretVisible = false;
> +      aSel->GetIsCollapsed(&isCollapsed);

Nit: instead, do:

bool isCollapsed = aSel->Collapsed();

Comment 24

[Peter Chang[:pchang]]

Attachment: WIP v4

Fix the nits.

And the try result is positive.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1d1e9d6b3813

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/03600247f685

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03600247f685