[Text Selection]Refactor the TextDialog DOM event

RESOLVED FIXED in mozilla37

Status

()

Core
Selection
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

Trunk
mozilla37
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 20 obsolete attachments)

46 bytes, text/x-github-pull-request
pchang
: review+
Details | Review | Splinter Review
3.12 KB, patch
pchang
: review+
Details | Diff | Splinter Review
1.18 KB, patch
pchang
: review+
Details | Diff | Splinter Review
17.32 KB, patch
pchang
: review+
Details | Diff | Splinter Review
5.90 KB, patch
pchang
: review+
Details | Diff | Splinter Review
2.70 KB, patch
pchang
: review+
Details | Diff | Splinter Review
14.75 KB, patch
pchang
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
The bug was issued according to bug 1065955 comment 19 and I copy the related information to below.

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.
(Assignee)

Updated

3 years ago
Blocks: 1023688
(Assignee)

Updated

3 years ago
Priority: -- → P3
(Assignee)

Comment 1

3 years ago
This bug is going to rename selectionchange to copypaste - besides, I also want to remove the UpdateCommands for in nsDocumentViewer[1] and dispatch the copypaste DOM event directly in SelectoinCarets[2]. Then we could maintain the logic of sending copypaste event inside selectioncaret.

ehsan, how do you think?

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

[2]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?from=SelectionCarets.cpp&case=true#951
Flags: needinfo?(ehsan.akhgari)

Comment 2

3 years ago
If that means moving the logic from nsGlobalWindow::UpdateCommands to SelectionCarets, I think that's fine.
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Comment 3

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #2)
> If that means moving the logic from nsGlobalWindow::UpdateCommands to
> SelectionCarets, I think that's fine.

Yes, I will move the logic from nsGLobalWindow::UpdateCommands to SelectionCaret. And then we could handle copypaste event inside SelectionCarets::NotifySelectionChanged instead of handling from nsDocumentViewer::NotifySelectionChanged and nsTextInputListener::NotifySelectionChanged.

I will start to work on the patch now.
(Assignee)

Updated

3 years ago
Blocks: 1089979
(Assignee)

Comment 4

3 years ago
roc, based on comment 1, I'm going to rename the 'selectionchange' dom events. Since this dom event is used to notify system app to show/hide the copy/paste/selectall dialog, I tried to rename to CopyPasteEvent but it may lead some misunderstanding that it is related to 'CopyPaste', like the copied string something.

BTW, we also have plan to merge touch caret and selection caerts together as the CopyPasteCarets, that's why I use CopyPasteEvent in my first patch.

Right I have several options, would you let me know your comment on the name?

a. CopyPasteDialogEvent
b. TextDialogEvent

The following are the data for the CopyPasteEvent.

 23 [Constructor(DOMString type, optional CopyPasteEventInit eventInit),
 24  ChromeOnly]
 25 interface CopyPasteEvent : Event {
 26   readonly attribute DOMString selectedText;
 27   readonly attribute DOMRectReadOnly? boundingClientRect;
 28   [Cached, Pure] readonly attribute sequence<CopyPasteEventReason> reasons;
 29 };
~
Flags: needinfo?(roc)
TextDialogEvent sounds good to me.
Flags: needinfo?(roc)
(Assignee)

Updated

3 years ago
Summary: [Text Selection]Rename selectionchange event to copypaste event → [Text Selection]Refactor the TextDialogChange DOM event
(Assignee)

Comment 6

3 years ago
This bug will cover two parts
a. rename the selectionchanges event to TextDialog event
b. refine the logic to dispatch the textDialog
Summary: [Text Selection]Refactor the TextDialogChange DOM event → [Text Selection]Refactor the TextDialog DOM event
(Assignee)

Comment 7

3 years ago
Created attachment 8520454 [details] [diff] [review]
rename SelectionChange to TextDialog DOM event
Attachment #8520454 - Flags: feedback?(tlin)
Attachment #8520454 - Flags: feedback?(mtseng)
(Assignee)

Comment 8

3 years ago
Created attachment 8520455 [details] [diff] [review]
refactor the logic to dispatch TextDialog event
Attachment #8520455 - Flags: feedback?(tlin)
Attachment #8520455 - Flags: feedback?(mtseng)
(Assignee)

Comment 9

3 years ago
Looks like I also need to change the test cases which use 'selectionchange' keyword, like dom/browser-element/mochitest/browserElement_CopyPaste.js.
Comment on attachment 8520454 [details] [diff] [review]
rename SelectionChange to TextDialog DOM event

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

Looks good!

::: dom/base/nsGlobalWindow.cpp
@@ +190,5 @@
>  #include "mozilla/dom/GamepadService.h"
>  #endif
>  
>  #include "nsRefreshDriver.h"
>  

We don't need "nsISelectionListener.h" and "nsCaret.h" as well.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +161,5 @@
>                       /* useCapture = */ true,
>                       /* wantsUntrusted = */ false);
>  
> +    addEventListener('moztextdialogchange',
> +                     this._textdialogchangeHandler.bind(this),

function name should be upper case for each word except first word.

this._textDialogChangeHandler.bind(this).

@@ +590,5 @@
>      };
>      sendAsyncMsg('scrollviewchange', detail);
>    },
>  
> +  _textdialogchangeHandler: function(e) {

ditto.

::: dom/webidl/TextDialogEvent.webidl
@@ +26,5 @@
> +interface TextDialogEvent : Event {
> +  readonly attribute DOMString selectedText;
> +  readonly attribute DOMRectReadOnly? boundingClientRect;
> +  [Cached, Pure] readonly attribute sequence<TextDialogEventReason> reasons;
> +};

Should we use "hg mv" here to generate better patch?

::: layout/base/SelectionCarets.cpp
@@ +980,2 @@
>  {
> +  if (aReason & SelectionCarets::DRAG) {

Why not use nsISelectionListener::DRAG_REASON directly? So that you don't need define same thing at SelectionCarets.h.

@@ +1041,5 @@
> +  nsCOMPtr<nsIDocument> doc = mPresShell->GetDocument();
> +
> +  MOZ_ASSERT(doc);
> +
> +  TextDialogEventInit init = {};

"= {}" is no need.

@@ +1095,5 @@
> +{
> +  SELECTIONCARETS_LOG("aSel (%p), Reason=%d", aSel, aReason);
> +  if (!aReason || (aReason & (nsISelectionListener::DRAG_REASON |
> +                               nsISelectionListener::KEYPRESS_REASON |
> +                               nsISelectionListener::MOUSEDOWN_REASON))) {

nit: indent.
Attachment #8520454 - Flags: feedback?(mtseng) → feedback+
Comment on attachment 8520454 [details] [diff] [review]
rename SelectionChange to TextDialog DOM event

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

::: layout/base/SelectionCarets.cpp
@@ +1035,5 @@
> +
> +void
> +SelectionCarets::DispatchTextDialogEvent(nsISelection* aSel, int16_t aReason)
> +{
> +  SELECTIONCARETS_LOG_STATIC("aReason %d\n", aReason);

Nit: SELECTIONCARETS_LOG append \n automatically.

@@ +1037,5 @@
> +SelectionCarets::DispatchTextDialogEvent(nsISelection* aSel, int16_t aReason)
> +{
> +  SELECTIONCARETS_LOG_STATIC("aReason %d\n", aReason);
> +
> +  nsCOMPtr<nsIDocument> doc = mPresShell->GetDocument();

Nit: We could use nsIDocument* here
Attachment #8520454 - Flags: feedback?(tlin) → feedback+
Comment on attachment 8520455 [details] [diff] [review]
refactor the logic to dispatch TextDialog event

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

f=me once the redundant define in SelectionCarets.h is removed.

::: layout/base/SelectionCarets.h
@@ +71,5 @@
>    static const short KEYPRESS = nsISelectionListener::KEYPRESS_REASON;               //0x8
>    static const short SELECTALL = nsISelectionListener::SELECTALL_REASON;             //0x10
>    static const short COLLAPSETOSTART = nsISelectionListener::COLLAPSETOSTART_REASON; //0x20
>    static const short COLLAPSETOEND = nsISelectionListener::COLLAPSETOEND_REASON;     //0x40
> +  static const short BLUR = 0x8000;

Ah, I see what you are trying to do here. You shouldn't define this because TextDialogEvent.webidl has the same data. You can just use TextDialogEventReason.

@@ +83,5 @@
>    // nsIScrollObserver
>    virtual void ScrollPositionChanged() MOZ_OVERRIDE;
>  
> +  //TODO add comment
> +  void NotifyChanged(int16_t aReason);

Ditto. You can use TextDialogEventReason as parameter type.
Attachment #8520455 - Flags: feedback?(mtseng) → feedback+
Comment on attachment 8520455 [details] [diff] [review]
refactor the logic to dispatch TextDialog event

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

::: layout/base/SelectionCarets.h
@@ +245,5 @@
>    bool mAPZenabled;
>    bool mEndCaretVisible;
>    bool mStartCaretVisible;
>    bool mVisible;
> +  bool mVisibilityGotChanged;

How about name it mShouldDispatchTextDialogEvent and reset it to false in DispatchTextDialogEvent()? In this way, we don't need to reset it whenever we call DispatchTextDialogEvent()
Attachment #8520455 - Flags: feedback?(tlin)
Attachment #8520455 - Flags: feedback?(mtseng)
Attachment #8520455 - Flags: feedback+
Attachment #8520455 - Flags: feedback?(mtseng) → feedback+
(Assignee)

Updated

3 years ago
Blocks: 1085200
(Assignee)

Updated

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

Comment 15

3 years ago
(In reply to Morris Tseng [:mtseng] from comment #12)
> Comment on attachment 8520455 [details] [diff] [review]
> refactor the logic to dispatch TextDialog event
> 
> Review of attachment 8520455 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me once the redundant define in SelectionCarets.h is removed.
> 
> ::: layout/base/SelectionCarets.h
> @@ +71,5 @@
> >    static const short KEYPRESS = nsISelectionListener::KEYPRESS_REASON;               //0x8
> >    static const short SELECTALL = nsISelectionListener::SELECTALL_REASON;             //0x10
> >    static const short COLLAPSETOSTART = nsISelectionListener::COLLAPSETOSTART_REASON; //0x20
> >    static const short COLLAPSETOEND = nsISelectionListener::COLLAPSETOEND_REASON;     //0x40
> > +  static const short BLUR = 0x8000;
> 
> Ah, I see what you are trying to do here. You shouldn't define this because
> TextDialogEvent.webidl has the same data. You can just use
> TextDialogEventReason.
> 
There is no need for others to know the TextDialog event when they are outside of selectioncarets. Therefore, I clone the reasons from nsISelectionListener and add one additional blur as the change reason of selectioncarets. The following is an example that is called in nsFocusManager.

  But I could remove the reasons from nsISelectionListener because NotifySelectionChange is passing the nsISelectionListener::XXX_REASON, not the SelectionCarets::xxx reasons. How do you think?

 if (selectionCarets) {
   selectionCarets->NotifyChanged(SelectionCarets::BLUR);
}		}


> @@ +83,5 @@
> >    // nsIScrollObserver
> >    virtual void ScrollPositionChanged() MOZ_OVERRIDE;
> >  
> > +  //TODO add comment
> > +  void NotifyChanged(int16_t aReason);
> 
> Ditto. You can use TextDialogEventReason as parameter type.
Flags: needinfo?(mtseng)
Discussed with peter offline. clear ni.
Flags: needinfo?(mtseng)
(Assignee)

Comment 17

3 years ago
Created attachment 8521208 [details] [diff] [review]
part 0 Rename webidl
(Assignee)

Comment 18

3 years ago
Created attachment 8521212 [details] [diff] [review]
part 1 v2 rename SelectionChange to TextDialog and move dispatch logic
Attachment #8520454 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8521249 [details] [diff] [review]
part 1 v2 rename SelectionChange to TextDialog and move dispatch logic
Attachment #8521212 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8521250 [details] [diff] [review]
part 2 v2 refactor the logic to dispatch TextDialog event
Attachment #8520455 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8521252 [details] [diff] [review]
part 1 v2 rename SelectionChange to TextDialog and move dispatch logic
Attachment #8521249 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8521208 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8521250 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8521252 - Flags: review?(roc)
(Assignee)

Comment 22

3 years ago
roc, this bug is trying to rename the SelectionChangeEvent to TextDialogEvent because TextDialog is not interested in every selection change notification. And gecko needs to have some selection change filter logic before dispatching the DOM event to TextDialog(system app). If we don't have this filter logic, it may cause some performance impact if there are lots of selection changes in a short time(too many DOM events send out in short time). After rename, I also move the TextDialogEvent inside SelectionCarets because we should align the visibility of TextDialogEvent with SelectionCarets(or TouchCaret).

part 0 - rename the SelectionChangeEvent.webidl to TextDialogEvent.webidl
part 1 - rename all SelectionChangeEvent to TextDialogEvent and move the dispatch logic into SelectionCarets
part 2 - refactor the TextDialog dispatch logic
(Assignee)

Updated

3 years ago
Assignee: nobody → pchang
(Assignee)

Updated

3 years ago
Priority: P3 → P1
(Assignee)

Comment 23

3 years ago
Created attachment 8522624 [details] [diff] [review]
part4 v1 rename to textdialogchange for copypaste mochitest
Attachment #8522624 - Flags: review?(roc)
Peter, part 0 patch lacks a valid e-mail address.
Attachment #8521208 - Flags: review?(roc) → review+
Attachment #8521252 - Flags: review?(roc) → review+
Attachment #8522624 - Flags: review?(roc) → review+
Comment on attachment 8521250 [details] [diff] [review]
part 2 v2 refactor the logic to dispatch TextDialog event

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

::: layout/base/SelectionCarets.h
@@ +73,5 @@
>  
> +  // Here shares the event reasons from TextDialogEvent.webidl as
> +  // the change reasons for SelectionCarets and others can call
> +  // SelectionCarets::NotifyChanged API to affect the visibility
> +  // of SelectionCarets

This isn't really clear. Are you saying other code should call NotifyChanged to change the visibility of the selection carets?

We're not using ChangeReason other than for Blur right now. None of the other patches do either, right? So we could just make this NotifyBlur for now. Why don't we do that?
Attachment #8521250 - Flags: review?(roc) → review-
(Assignee)

Comment 26

3 years ago
Comment on attachment 8521250 [details] [diff] [review]
part 2 v2 refactor the logic to dispatch TextDialog event

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

::: layout/base/SelectionCarets.h
@@ +73,5 @@
>  
> +  // Here shares the event reasons from TextDialogEvent.webidl as
> +  // the change reasons for SelectionCarets and others can call
> +  // SelectionCarets::NotifyChanged API to affect the visibility
> +  // of SelectionCarets

Yes, you are right. Outside of selection caerts, we only have one place to change the visibility of selection carets when the element loses focus.

Agree to change to NotifyBlur which is simpler to maintain the text dialog dispatch logic.
(Assignee)

Comment 27

3 years ago
Created attachment 8522733 [details] [diff] [review]
part 0 v2 Rename webidl

add reviewer to comment
Attachment #8521208 - Attachment is obsolete: true
Attachment #8522733 - Flags: review+
(Assignee)

Comment 28

3 years ago
Created attachment 8522734 [details] [diff] [review]
part 1 v3 rename SelectionChange to TextDialog and move dispatch logic
Attachment #8521252 - Attachment is obsolete: true
Attachment #8522734 - Flags: review+
(Assignee)

Comment 29

3 years ago
Created attachment 8522737 [details] [diff] [review]
part 1 v3 rename SelectionChange to TextDialog and move dispatch logic

Upload the right one
Attachment #8522734 - Attachment is obsolete: true
Attachment #8522737 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8522733 - Attachment description: bug_1090008_part0.patch → part 0 v2 Rename webidl
(Assignee)

Comment 30

3 years ago
Created attachment 8522740 [details] [diff] [review]
part3 v2 rename to textdialogchange for copypaste mochitest
Attachment #8522624 - Attachment is obsolete: true
Attachment #8522740 - Flags: review+
(Assignee)

Comment 31

3 years ago
Created attachment 8522746 [details] [diff] [review]
part 2 v3 refactor the logic to dispatch TextDialog event

Modify patch based on comment 26.
Attachment #8521250 - Attachment is obsolete: true
Attachment #8522746 - Flags: review?(roc)
Attachment #8522746 - Flags: review?(roc) → review+
(Assignee)

Comment 32

3 years ago
Created attachment 8523611 [details] [diff] [review]
part 2 v4 refactor the logic to dispatch TextDialog event

Add reviewer into comment
Attachment #8522746 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8523611 - Flags: review+
(Assignee)

Comment 33

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bc0d1aaee9e2

Try result is positive.
Keywords: checkin-needed
seems you need dom peer reviews here:

remote: 
remote: WebIDL file dom/webidl/TextDialogEvent.webidl altered in changeset 4e5d5c0a8239 without DOM peer review
remote: WebIDL file dom/webidl/SelectionChangeEvent.webidl altered in changeset 4ca68d3ae192 without DOM peer review
remote: WebIDL file dom/webidl/TextDialogEvent.webidl altered in changeset 4ca68d3ae192 without DOM peer review
Flags: needinfo?(pchang)
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Flags: needinfo?(pchang)
Attachment #8522733 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8522737 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8522733 - Flags: review?(bzbarsky) → superreview?(bugs)
(Assignee)

Updated

3 years ago
Attachment #8522737 - Flags: review?(bzbarsky) → superreview?(bugs)
(Assignee)

Comment 35

3 years ago
Olli, please help to review the webidl changes in my patch part0 and part1.
Basically I just rename the DOM event not touch the logic.
I don't understand the name TextDialog.
The fact that "dom event is used to notify system app to show/hide the copy/paste/selectall dialog," is not a reason to use TextDialog in the name. The event name should tell what it is notifying about, 
not what some event listener happens to do with the event.

And why the name
DispatchTextDialogEventIfNeeded? That method seems to always dispatch an event.

Updated

3 years ago
Attachment #8522733 - Flags: superreview?(bugs) → superreview-

Updated

3 years ago
Attachment #8522737 - Flags: superreview?(bugs) → superreview-
Created attachment 8524264 [details] [review]
PR to gaia
Blocks: 1067728
(Assignee)

Comment 38

3 years ago
(In reply to Olli Pettay [:smaug] from comment #36)
> I don't understand the name TextDialog.
> The fact that "dom event is used to notify system app to show/hide the
> copy/paste/selectall dialog," is not a reason to use TextDialog in the name.
> The event name should tell what it is notifying about, 
> not what some event listener happens to do with the event.
> 
Understood your above concern. Current the changes of text dialog are aligned to the changes of touch/selection carets in gecko, for example scroll/reflow/blur happens.

In the future, we are going to rename the touch/selection carets to 'AccessibleCarets' which could run as touch or selection mode depend on conditions.

Therefore, how about we create a DOM event called 'AccessibleCaretsChange' here?
And we could embedded the detail reason inside this DOM event.

roc, smaung, please feel free to let me know your comment.

> And why the name
> DispatchTextDialogEventIfNeeded? That method seems to always dispatch an
> event.
In attachment 8523611 [details] [diff] [review], I added some condition check before dispatching events.
No longer blocks: 1067728
Flags: needinfo?(roc)
Flags: needinfo?(bugs)
(Assignee)

Updated

3 years ago
Blocks: 1067728
mozselectionchange is still pretty close what the event is about.
Do we need to filter out collapsed selections? If we didn't, mozselectionchange would be even more
correct name. The event could even have a flag to tell whether it is for a collapsed selection and
then a listener could filter it out easily.
Flags: needinfo?(bugs)
I agree.
Flags: needinfo?(roc)
(Assignee)

Comment 41

3 years ago
(In reply to Olli Pettay [:smaug] from comment #39)
> mozselectionchange is still pretty close what the event is about.
> Do we need to filter out collapsed selections? If we didn't,
> mozselectionchange would be even more
Yes, we did filter out the collapsed selections in [1]. If not, we might send out lots of selectionchanges events because lots of collapsed selectionchange events were receive in a short time, like bug 1066515. And these DOM events across IPC cause the performance issue.

Therefore, this is the reason I want to rename the selectionchange DOM event.

> correct name. The event could even have a flag to tell whether it is for a
> collapsed selection and
> then a listener could filter it out easily.
As I mentioned above, it causes performance issues if we don't filter it in the sender side.

[1]http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?from=nsGlobalWindow.cpp&case=true#9380
(Assignee)

Comment 42

3 years ago
Comment on attachment 8522737 [details] [diff] [review]
part 1 v3 rename SelectionChange to TextDialog and move dispatch logic

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

::: dom/webidl/TextDialogEvent.webidl
@@ +11,5 @@
>    "keypress",
>    "selectall",
>    "collapsetostart",
> +  "collapsetoend",
> +  "blur"

Before I figure out the proper DOM event name, I could keep current selectionchange and send out a custom event with blur. But sooner, we need to add the reflow event with a rect info for text dialog when the orientation is changed.

Updated

3 years ago
Duplicate of this bug: 1089979
(In reply to peter chang[:pchang][:peter] from comment #41)
> Yes, we did filter out the collapsed selections in [1]. If not, we might
> send out lots of selectionchanges events because lots of collapsed
> selectionchange events were receive in a short time, like bug 1066515. And
> these DOM events across IPC cause the performance issue.
DOM events don't do any IPC. If some event listener ends up sending all the events through message manager or so, that
is a different issue. That event listener could just filter out events for collapsed selections. 

> As I mentioned above, it causes performance issues if we don't filter it in
> the sender side.
DOM events aren't doing IPC stuff, so at least they don't cause any IPC trafic.

It is not quite clear to me from bug 1066515 what the actual performance issue was.
Did we send all the events through IPC and then event listener was doing something heavy?
Or if we really must not dispatch the event for collapsed selections, then
the event type could be something like mozNonCollapsedSelectionChange
(yes that is a long one, but at least it would tell what it is about)
(In reply to Olli Pettay [:smaug] from comment #44)
> Did we send all the events through IPC and then event listener was doing
> something heavy?
Looks like http://hg.mozilla.org/mozilla-central/annotate/b4fbeba78a7d/dom/browser-element/BrowserElementChildPreload.js#l594 could do some
filtering
(Assignee)

Comment 47

3 years ago
(In reply to Olli Pettay [:smaug] from comment #44)
> (In reply to peter chang[:pchang][:peter] from comment #41)
> > Yes, we did filter out the collapsed selections in [1]. If not, we might
> > send out lots of selectionchanges events because lots of collapsed
> > selectionchange events were receive in a short time, like bug 1066515. And
> > these DOM events across IPC cause the performance issue.
> DOM events don't do any IPC. If some event listener ends up sending all the
> events through message manager or so, that
> is a different issue. That event listener could just filter out events for
> collapsed selections. 
> 
> > As I mentioned above, it causes performance issues if we don't filter it in
> > the sender side.
> DOM events aren't doing IPC stuff, so at least they don't cause any IPC
> trafic.
> 
> It is not quite clear to me from bug 1066515 what the actual performance
> issue was.
> Did we send all the events through IPC and then event listener was doing
> something heavy?
Turn on SelectionCarets by default was rollback because of bug 1066515 and then I did some profiling in bug 1065955 comment 11.

The following is the simple flow for selectionchanges.
SelectionCarets.cpp--DispatchSelectionChanges-->BrowserElementChildPreload.js--sendAsyncMsg-->BrowserElementParent.jsm-->dispatchEvent-->shell.js---sendChromeEvent-->System APP

Based on the profiling, B2G main thread was spending CPU during dispatchEvent and sendChromeEvent in above flow if we didn't filter out the collapsed events.

This is the reason I tried to filter out the collapsed selectionchanges in bug 1066515.

(In reply to Olli Pettay [:smaug] from comment #46)
> (In reply to Olli Pettay [:smaug] from comment #44)
> > Did we send all the events through IPC and then event listener was doing
> > something heavy?
> Looks like
> http://hg.mozilla.org/mozilla-central/annotate/b4fbeba78a7d/dom/browser-
> element/BrowserElementChildPreload.js#l594 could do some
> filtering

As you mentioned, I could did the same thing in BrowserElementChildpreload.js, but I might need to check the cost to dispatch the DOM event in content side.
(Assignee)

Comment 48

3 years ago
Comment on attachment 8523611 [details] [diff] [review]
part 2 v4 refactor the logic to dispatch TextDialog event

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

::: layout/base/SelectionCarets.cpp
@@ +1008,5 @@
> +  }
> +
> +  if (!mShouldDispatchTextDialogEvent && !GetVisibility() && !isTouchCaretVisible) {
> +    return;
> +  }

After working on refactoring the selectionChange more, I think the simple way is to align the visibility change of selection/touch Caret which may dispatch collapsed selectionchange event, for example when the visibility of selection caret becomes hidden because collapse is true. And the text dialog needs this selectionchange event to hidden itself. Therefore, this is the story I want to rename original selectionchange event.
So the event is about many different things. About selection changes, visibility, maybe some touch caret..

mozSelectionStateChanged then?
(Assignee)

Comment 50

3 years ago
(In reply to Olli Pettay [:smaug] from comment #49)
> So the event is about many different things. About selection changes,
> visibility, maybe some touch caret..
> 
> mozSelectionStateChanged then?

Had some discussion with smaug on IRC.

Since the text dialog is not interested in every selectionchange event and it needs to listen other activities, like scroll/blur/reflow...

Then we will rename original dom event from selectionchange to selectionstate and add selectionstateHandler to handle this new dom event in BrowserElementChildPreload.

With the new selectionstate, I will include original selectionchange reasons but also blur.
P.S. The reflow use case will be covered in bug 1063266.

In BrowserElementChildPreload, I will also try to filter the collapsed selection change event in selectionstateHandler instead of inside selectioncarets.

smaug, please feel free to correct above comment if I am wrong.
Flags: needinfo?(bugs)
Sounds ok.
You may add new attributes to the webidl interface, like, attribute boolean collapsed if needed.
Flags: needinfo?(bugs)
(Assignee)

Comment 52

3 years ago
Created attachment 8526737 [details] [diff] [review]
part 1 refactor the selectionchange dispatch

This patch is almost the same as attachment 8522737 [details] [diff] [review], but remove the SelectionChange rename part.
Attachment #8522733 - Attachment is obsolete: true
Attachment #8522737 - Attachment is obsolete: true
Attachment #8522740 - Attachment is obsolete: true
Attachment #8523611 - Attachment is obsolete: true
Attachment #8526737 - Flags: review+
(Assignee)

Comment 53

3 years ago
Created attachment 8526739 [details] [diff] [review]
WIP part 2 Move the selectionchange filter logic to browserelementchild

Move the selectionchange filter logic to BrowserElementChildPreload.js
(Assignee)

Comment 54

3 years ago
(In reply to peter chang[:pchang][:peter] from comment #50)
> (In reply to Olli Pettay [:smaug] from comment #49)
> > So the event is about many different things. About selection changes,
> > visibility, maybe some touch caret..
> > 
> > mozSelectionStateChanged then?
> 
> Had some discussion with smaug on IRC.
> 
> Since the text dialog is not interested in every selectionchange event and
> it needs to listen other activities, like scroll/blur/reflow...
> 

> Then we will rename original dom event from selectionchange to
> selectionstate and add selectionstateHandler to handle this new dom event in
> BrowserElementChildPreload.
> 
> With the new selectionstate, I will include original selectionchange reasons
> but also blur.
> P.S. The reflow use case will be covered in bug 1063266.
When I tried to create the SelectionState.webidl, the original selectionchange reason is not proper to name as a state but 'blur' is fine from my point of view.

Therefore, I gave up to merge SelectionChane.webidl with SelectionState.webidl.
In attachment 8526739 [details] [diff] [review], I already move the selectionchange filter logic to BrowserElementChidPreload and now is not necessary to rename SelectionChange.webidl since there is no filter logic before dispatching.

For the blur/reflow case, I think it is good to add into SelectionState.webidl. I will work on the part 3 patch to support the blur case.

enum SelectionState {
  "drag",
  "mousedown",
  "mouseup",
  "keypress",
  "selectall",
  "collapsetostart",
  "collapsetoend",
  "blur"
};
> 
> In BrowserElementChildPreload, I will also try to filter the collapsed
> selection change event in selectionstateHandler instead of inside
> selectioncarets.
> 
> smaug, please feel free to correct above comment if I am wrong.
(Assignee)

Comment 55

3 years ago
smaug, how do you think about comment 54?
Flags: needinfo?(bugs)
(Assignee)

Updated

3 years ago
Attachment #8526739 - Flags: review?(bugs)
(I still don't quite understand how you're going to deal with "reflow" case. It can be in _hot_ code paths.)
Flags: needinfo?(bugs)
I don't know what comment 54 is trying to say...
I'd prefer keeping this all in one .webidl. Creating a new .webidl just for an enum would be overkill.
So, don't you want SelectionStateChanged interface for the mozSelectionStateChanged event.

And yes, BrowserElementChild should filter out events.
Comment on attachment 8526739 [details] [diff] [review]
WIP part 2 Move the selectionchange filter logic to browserelementchild

>+    let isCollapsed = (e.selectedText.length == 0);
>+    let isMouseUp = (e.reasons.indexOf('mouseup') == 0);
>+    let canPaste = this._isCommandEnabled("paste");
>+
>+    if (!this._forceDispatchSelectionChange) {
>+      // SelectionChange events with the following reasons are not
>+      // necessary to trigger the text dialog, bypass these events
>+      // by default. But they would be dispatched to hidden the text dialog
to hide


>+    // The non-collapsed selection event with mouseup means there are
>+    // some texts are selected and force to dispath the next SelectionChange
dispatch

>+    // events to hidden the visible text dialog
event to hide

But this code really isn't about hiding some text dialog. This code is about notifying the parent side of some state change.
So, perhaps say 
"...and force to dispatch the next selectionchange event so that parent side can for example
hide the visible text dialog"



>+    if (isMouseUp && !isCollapsed) {
>+      this._forceDispatchSelectionChange = true;
>+    }
I think the code would be a bit easier to understand if you'd
have 
} else {
  this._forceDispatchSelectionChange = false;
}

and not this._forceDispatchSelectionChange = false; before the if. 

But I don't quite understand why and when forceDispatchSelectionChange is true.
Could you improve the comment above 'if (isMouseUp && !isCollapsed) {'
Attachment #8526739 - Flags: review?(bugs) → review-
(Assignee)

Comment 59

3 years ago
(In reply to Olli Pettay [:smaug] from comment #57)
> I don't know what comment 54 is trying to say...
> I'd prefer keeping this all in one .webidl. Creating a new .webidl just for
> an enum would be overkill.
> So, don't you want SelectionStateChanged interface for the
> mozSelectionStateChanged event.
> 
smaug, the following is the original enum for selectionchange reasons in SelectionChangeEvent.webidl. As we discussed before, we decide to change this enum 
SelectionChangeReason to SelectionState. But I think it is not proper to treat 'mousedown' as a state(the same to mouseup/keypress), that's why I want to create another SelectionState webidl. But if you think that's ok, then I keep them all in one SelectionState.webidl.


enum SelectionChangeReason {
  "drag",
  "mousedown",
  "mouseup",
  "keypress",
  "selectall",
  "collapsetostart",
  "collapsetoend",
  "blur"  //This will be a new one I added in this bug
};


> And yes, BrowserElementChild should filter out events.

(In reply to Olli Pettay [:smaug] from comment #56)
> (I still don't quite understand how you're going to deal with "reflow" case.
> It can be in _hot_ code paths.)

For the reflow case, the selection carets already registered the reflow callback in [1]. And I didn't see too many reflow calls are received when you rotate your phone. To handle the reflow for text dialog, I also plan to dispatch the selection dom event in [1]. Does it make sense to you?

[Reflow log]
//Select word and selection carets are displayed
11-21 09:09:55.643  2537  2537 I PRLog   : -1225703084[b434a080]: SelectionCarets (b041e340): Reflow:1185 : Update selection carets after reflow!

//Rotate the screen
11-21 09:09:58.603  2537  2537 I PRLog   : -1225703084[b434a080]: SelectionCarets (b041e340): Reflow:1185 : Update selection carets after reflow!
11-21 09:09:58.613  2537  2537 I PRLog   : -1225703084[b434a080]: SelectionCarets (b041e340): Reflow:1185 : Update selection carets after reflow!


[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp#1090
Flags: needinfo?(bugs)
(Assignee)

Comment 60

3 years ago
Created attachment 8528182 [details] [diff] [review]
WIP-v2 part 2 Move the selectionchange filter logic to browserelementchild

Address reviewer's suggestion.
Attachment #8526739 - Attachment is obsolete: true
Attachment #8528182 - Flags: review?(bugs)
(In reply to peter chang[:pchang][:peter] from comment #59)
> smaug, the following is the original enum for selectionchange reasons in
> SelectionChangeEvent.webidl. As we discussed before, we decide to change
> this enum 
> SelectionChangeReason to SelectionState.

Ah, I this all this should be changed to *State.
SelectionStateEvent etc.
Flags: needinfo?(bugs)
Comment on attachment 8528182 [details] [diff] [review]
WIP-v2 part 2 Move the selectionchange filter logic to browserelementchild

I think we should call it _forceDispatchSelectionState and rename the message and event too.
Attachment #8528182 - Flags: review?(bugs) → review+
(Assignee)

Comment 63

3 years ago
(In reply to Olli Pettay [:smaug] from comment #62)
> Comment on attachment 8528182 [details] [diff] [review]
> WIP-v2 part 2 Move the selectionchange filter logic to browserelementchild
> 
> I think we should call it _forceDispatchSelectionState and rename the
> message and event too.

Sure, I will create another patch for renaming.
(Assignee)

Comment 64

3 years ago
Created attachment 8529593 [details] [diff] [review]
part2-v3 Move the selectionchange filter logic to browserelementchild

Update reviewer
Attachment #8528182 - Attachment is obsolete: true
Attachment #8529593 - Flags: review+
(Assignee)

Comment 65

3 years ago
Created attachment 8529597 [details] [diff] [review]
WIP part3 Rename SelectionChangeEvent.webidl to SelectionStateEvent.webidl

Rename to SelectionStateEvent.webidl
Attachment #8529597 - Flags: review?(bugs)
(Assignee)

Comment 66

3 years ago
Created attachment 8529599 [details] [diff] [review]
WIP part4 Rename all SelectionChange to SelectionState
Attachment #8529599 - Flags: review?(bugs)
(Assignee)

Comment 67

3 years ago
Created attachment 8529603 [details] [diff] [review]
WIP part5 Dispatch SelectionState for blur

Carry the r+ from preview patch, and the difference is the DOM event name change to SelectionStateEvent.
Attachment #8529603 - Flags: review+

Updated

3 years ago
Attachment #8529597 - Flags: review?(bugs) → review+
Comment on attachment 8529597 [details] [diff] [review]
WIP part3 Rename SelectionChangeEvent.webidl to SelectionStateEvent.webidl

I would have call it SelectionStateChangedEvent.
With that, r+
Comment on attachment 8529599 [details] [diff] [review]
WIP part4 Rename all SelectionChange to SelectionState

Same here, mozbrowserselectionstatechanged, 
and 'selectionstate' -> 'selectionstatechanged'

Makes code easier to understand.

_forceDispatchSelectionState -> _forceDispatchSelectionStateChanged

SelectionStateEventInit -> SelectionStateChangedEventInit

DispatchSelectionStateEvent -> DispatchSelectionStateChangedEvent


I know it is a bit long, but code should be made for reading.
So, with those changes, r+
Attachment #8529599 - Flags: review?(bugs) → review+
(Assignee)

Updated

3 years ago
Attachment #8526737 - Attachment description: WIP part 1 refactor the selectionchange dispatch → part 1 refactor the selectionchange dispatch
(Assignee)

Updated

3 years ago
Attachment #8529593 - Attachment description: WIP-v3 part 2 Move the selectionchange filter logic to browserelementchild → part2-v3 Move the selectionchange filter logic to browserelementchild
(Assignee)

Comment 70

3 years ago
Created attachment 8530012 [details] [diff] [review]
part3-v2 Rename SelectionChangeEvent.webidl to SelectionStateEvent.webidl

Rename to SelectionStateChanged
Attachment #8529597 - Attachment is obsolete: true
Attachment #8530012 - Flags: review+
(Assignee)

Comment 71

3 years ago
Created attachment 8530013 [details] [diff] [review]
part4-v2 Rename all SelectionChange to SelectionStateChanged

Rename to SelectionStateChanged
Attachment #8529599 - Attachment is obsolete: true
Attachment #8530013 - Flags: review+
(Assignee)

Comment 72

3 years ago
Created attachment 8530015 [details] [diff] [review]
part5-v2 Dispatch SelectionState for blur

Rename to SelectionStateChanged
Attachment #8529603 - Attachment is obsolete: true
Attachment #8530015 - Flags: review+
(Assignee)

Comment 73

3 years ago
Created attachment 8530016 [details] [diff] [review]
part6 v2 rename to textdialogchange for copypaste mochitest

Rename to SelectionStateChanged and carry with r+ from previous patch.
Attachment #8530016 - Flags: review+
(Assignee)

Comment 74

3 years ago
The failed test for B2G Desktop OS X opt is not related to this bug.
Therefore, the try server result is positive.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bdaac97f6a60
(Assignee)

Comment 75

3 years ago
Please help to land part1 to part 6.
Keywords: checkin-needed
1 out of 6 hunks FAILED -- saving rejects to file dom/base/nsGlobalWindow.cpp.rej

Peter could you take a look, seems the patch part 1 didn't apply cleanly.

Thanks
Flags: needinfo?(pchang)
Keywords: checkin-needed
(Assignee)

Comment 77

3 years ago
Created attachment 8530273 [details] [diff] [review]
part1-v2 refactor the selectionchange dispatch

Rebase to latest inbound
Attachment #8526737 - Attachment is obsolete: true
Flags: needinfo?(pchang)
Attachment #8530273 - Flags: review+
(Assignee)

Comment 78

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #76)
> 1 out of 6 hunks FAILED -- saving rejects to file
> dom/base/nsGlobalWindow.cpp.rej
> 
> Peter could you take a look, seems the patch part 1 didn't apply cleanly.
> 
> Thanks

Carsten, could you check again? In my side, I can apply these six patches now. Thanks for help.
Keywords: checkin-needed
Hey Peter this time it worked! thanks!

remote: You can view your changes at the following URLs:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e36fa1642349
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06dce22f4199
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34705861ea0d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/16063be273d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f56d00dd0fdc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e92aee06a1bf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e36fa1642349
https://hg.mozilla.org/mozilla-central/rev/06dce22f4199
https://hg.mozilla.org/mozilla-central/rev/34705861ea0d
https://hg.mozilla.org/mozilla-central/rev/16063be273d6
https://hg.mozilla.org/mozilla-central/rev/f56d00dd0fdc
https://hg.mozilla.org/mozilla-central/rev/e92aee06a1bf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
George, 

The gecko part had been landed, we have to modify the Gaia side to make the text selection dialog work. The event name 'selectionchange' had been changed to 'selectionstatechanged', and the "detail.reasons" to "detail.states" in patch part4. Thanks.
Flags: needinfo?(gduan)
Comment on attachment 8524264 [details] [review]
PR to gaia

Hi Peter,
the name and detail of event has been updated, I think you can help to review it. Thanks.
Flags: needinfo?(gduan)
Attachment #8524264 - Flags: review?(pchang)
(Assignee)

Comment 83

3 years ago
Comment on attachment 8524264 [details] [review]
PR to gaia

r=me, if the comments in gaia pull request are fixed.
Attachment #8524264 - Flags: review?(pchang) → review+
Blocks: 1092437
PR to gaia master: https://github.com/mozilla-b2g/gaia/commit/0ea472a5cf969e74f0b7d073253684bbb9c50f09
Duplicate of this bug: 1073318
You need to log in before you can comment on or make changes to this bug.