1090008 - [Text Selection]Refactor the TextDialog DOM event

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Peter Chang[:pchang]]

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.

Comment 1

[Peter Chang[:pchang]]

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

Comment 2

[(no longer active)]

If that means moving the logic from nsGlobalWindow::UpdateCommands to SelectionCarets, I think that's fine.

Comment 3

[Peter Chang[:pchang]]

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

Comment 4

[Peter Chang[:pchang]]

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 };
~

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

TextDialogEvent sounds good to me.

Comment 6

[Peter Chang[:pchang]]

This bug will cover two parts
a. rename the selectionchanges event to TextDialog event
b. refine the logic to dispatch the textDialog

Comment 7

[Peter Chang[:pchang]]

Attachment: rename SelectionChange to TextDialog DOM event

Comment 8

[Peter Chang[:pchang]]

Attachment: refactor the logic to dispatch TextDialog event

Comment 9

[Peter Chang[:pchang]]

Looks like I also need to change the test cases which use 'selectionchange' keyword, like dom/browser-element/mochitest/browserElement_CopyPaste.js.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 11

[Ting-Yu Lin [:TYLin] (UTC-8)]

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

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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.

Comment 13

[Ting-Yu Lin [:TYLin] (UTC-8)]

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

Comment 15

[Peter Chang[:pchang]]

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

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Discussed with peter offline. clear ni.

Comment 17

[Peter Chang[:pchang]]

Attachment: part 0 Rename webidl

Comment 18

[Peter Chang[:pchang]]

Attachment: part 1 v2 rename SelectionChange to TextDialog and move dispatch logic

Comment 19

[Peter Chang[:pchang]]

Attachment: part 1 v2 rename SelectionChange to TextDialog and move dispatch logic

Comment 20

[Peter Chang[:pchang]]

Attachment: part 2 v2 refactor the logic to dispatch TextDialog event

Comment 21

[Peter Chang[:pchang]]

Attachment: part 1 v2 rename SelectionChange to TextDialog and move dispatch logic

Comment 22

[Peter Chang[:pchang]]

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

Comment 23

[Peter Chang[:pchang]]

Attachment: part4 v1 rename to textdialogchange for copypaste mochitest

Comment 24

[Ting-Yu Lin [:TYLin] (UTC-8)]

Peter, part 0 patch lacks a valid e-mail address.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 26

[Peter Chang[:pchang]]

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.

Comment 27

[Peter Chang[:pchang]]

Attachment: part 0 v2 Rename webidl

add reviewer to comment

Comment 28

[Peter Chang[:pchang]]

Attachment: part 1 v3 rename SelectionChange to TextDialog and move dispatch logic

Comment 29

[Peter Chang[:pchang]]

Attachment: part 1 v3 rename SelectionChange to TextDialog and move dispatch logic

Upload the right one

Comment 30

[Peter Chang[:pchang]]

Attachment: part3 v2 rename to textdialogchange for copypaste mochitest

Comment 31

[Peter Chang[:pchang]]

Attachment: part 2 v3 refactor the logic to dispatch TextDialog event

Modify patch based on comment 26.

Comment 32

[Peter Chang[:pchang]]

Attachment: part 2 v4 refactor the logic to dispatch TextDialog event

Add reviewer into comment

Comment 33

[Peter Chang[:pchang]]

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

Try result is positive.

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Peter Chang[:pchang]]

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.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 37

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

Attachment: PR to gaia

Comment 38

[Peter Chang[:pchang]]

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

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree.

Comment 41

[Peter Chang[:pchang]]

(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

Comment 42

[Peter Chang[:pchang]]

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.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 47

[Peter Chang[:pchang]]

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

Comment 48

[Peter Chang[:pchang]]

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.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

So the event is about many different things. About selection changes, visibility, maybe some touch caret..

mozSelectionStateChanged then?

Comment 50

[Peter Chang[:pchang]]

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

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Sounds ok.
You may add new attributes to the webidl interface, like, attribute boolean collapsed if needed.

Comment 52

[Peter Chang[:pchang]]

Attachment: part 1 refactor the selectionchange dispatch

This patch is almost the same as attachment 8522737 [details] [diff] [review], but remove the SelectionChange rename part.

Comment 53

[Peter Chang[:pchang]]

Attachment: WIP part 2 Move the selectionchange filter logic to browserelementchild

Move the selectionchange filter logic to BrowserElementChildPreload.js

Comment 54

[Peter Chang[:pchang]]

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

Comment 55

[Peter Chang[:pchang]]

smaug, how do you think about comment 54?

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

(I still don't quite understand how you're going to deal with "reflow" case. It can be in _hot_ code paths.)

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

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 58

[Olli Pettay [:smaug][[e-mail]]]

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) {'

Comment 59

[Peter Chang[:pchang]]

(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

Comment 60

[Peter Chang[:pchang]]

Attachment: WIP-v2 part 2 Move the selectionchange filter logic to browserelementchild

Address reviewer's suggestion.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 63

[Peter Chang[:pchang]]

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

Comment 64

[Peter Chang[:pchang]]

Attachment: part2-v3 Move the selectionchange filter logic to browserelementchild

Update reviewer

Comment 65

[Peter Chang[:pchang]]

Attachment: WIP part3 Rename SelectionChangeEvent.webidl to SelectionStateEvent.webidl

Rename to SelectionStateEvent.webidl

Comment 66

[Peter Chang[:pchang]]

Attachment: WIP part4 Rename all SelectionChange to SelectionState

Comment 67

[Peter Chang[:pchang]]

Attachment: WIP part5 Dispatch SelectionState for blur

Carry the r+ from preview patch, and the difference is the DOM event name change to SelectionStateEvent.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

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 69

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 70

[Peter Chang[:pchang]]

Attachment: part3-v2 Rename SelectionChangeEvent.webidl to SelectionStateEvent.webidl

Rename to SelectionStateChanged

Comment 71

[Peter Chang[:pchang]]

Attachment: part4-v2 Rename all SelectionChange to SelectionStateChanged

Rename to SelectionStateChanged

Comment 72

[Peter Chang[:pchang]]

Attachment: part5-v2 Dispatch SelectionState for blur

Rename to SelectionStateChanged

Comment 73

[Peter Chang[:pchang]]

Attachment: part6 v2 rename to textdialogchange for copypaste mochitest

Rename to SelectionStateChanged and carry with r+ from previous patch.

Comment 74

[Peter Chang[:pchang]]

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

Comment 75

[Peter Chang[:pchang]]

Please help to land part1 to part 6.

Comment 76

[Carsten Book [:Tomcat]]

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

Comment 77

[Peter Chang[:pchang]]

Attachment: part1-v2 refactor the selectionchange dispatch

Rebase to latest inbound

Comment 78

[Peter Chang[:pchang]]

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

Comment 79

[Carsten Book [:Tomcat]]

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

Comment 80

[Phil Ringnalda (:philor)]

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

Comment 81

[Ting-Yu Lin [:TYLin] (UTC-8)]

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.

Comment 82

[(Inactive after June) George Duan [: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.

Comment 83

[Peter Chang[:pchang]]

Comment on attachment 8524264 [details] [review]
PR to gaia

r=me, if the comments in gaia pull request are fixed.

Comment 84

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

PR to gaia master: https://github.com/mozilla-b2g/gaia/commit/0ea472a5cf969e74f0b7d073253684bbb9c50f09