Closed Bug 1090008 Opened 5 years ago Closed 5 years ago

[Text Selection]Refactor the TextDialog DOM event

Categories

(Core :: Selection, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(7 files, 20 obsolete files)

46 bytes, text/x-github-pull-request
pchang
: review+
Details | 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
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.
Priority: -- → P3
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)
If that means moving the logic from nsGlobalWindow::UpdateCommands to SelectionCarets, I think that's fine.
Flags: needinfo?(ehsan.akhgari)
(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.
Blocks: 1089979
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)
Summary: [Text Selection]Rename selectionchange event to copypaste event → [Text Selection]Refactor the TextDialogChange DOM event
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
Attachment #8520454 - Flags: feedback?(tlin)
Attachment #8520454 - Flags: feedback?(mtseng)
Attachment #8520455 - Flags: feedback?(tlin)
Attachment #8520455 - Flags: feedback?(mtseng)
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+
Blocks: 1085200
Duplicate of this bug: 1085200
(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)
Attached patch part 0 Rename webidl (obsolete) — Splinter Review
Attachment #8520455 - Attachment is obsolete: true
Attachment #8521208 - Flags: review?(roc)
Attachment #8521250 - Flags: review?(roc)
Attachment #8521252 - Flags: review?(roc)
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: nobody → pchang
Priority: P3 → P1
Peter, part 0 patch lacks a valid e-mail address.
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-
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.
Attached patch part 0 v2 Rename webidl (obsolete) — Splinter Review
add reviewer to comment
Attachment #8521208 - Attachment is obsolete: true
Attachment #8522733 - Flags: review+
Attachment #8521252 - Attachment is obsolete: true
Attachment #8522734 - Flags: review+
Upload the right one
Attachment #8522734 - Attachment is obsolete: true
Attachment #8522737 - Flags: review+
Attachment #8522733 - Attachment description: bug_1090008_part0.patch → part 0 v2 Rename webidl
Attachment #8522624 - Attachment is obsolete: true
Attachment #8522740 - Flags: review+
Modify patch based on comment 26.
Attachment #8521250 - Attachment is obsolete: true
Attachment #8522746 - Flags: review?(roc)
Add reviewer into comment
Attachment #8522746 - Attachment is obsolete: true
Attachment #8523611 - Flags: review+
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
Flags: needinfo?(pchang)
Attachment #8522733 - Flags: review?(bzbarsky)
Attachment #8522737 - Flags: review?(bzbarsky)
Attachment #8522733 - Flags: review?(bzbarsky) → superreview?(bugs)
Attachment #8522737 - Flags: review?(bzbarsky) → superreview?(bugs)
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.
Attachment #8522733 - Flags: superreview?(bugs) → superreview-
Attachment #8522737 - Flags: superreview?(bugs) → superreview-
(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)
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)
(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 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.
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
(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 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?
(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)
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+
Move the selectionchange filter logic to BrowserElementChildPreload.js
(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.
smaug, how do you think about comment 54?
Flags: needinfo?(bugs)
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-
(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)
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+
(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.
Update reviewer
Attachment #8528182 - Attachment is obsolete: true
Attachment #8529593 - Flags: review+
Rename to SelectionStateEvent.webidl
Attachment #8529597 - Flags: review?(bugs)
Attachment #8529599 - Flags: review?(bugs)
Carry the r+ from preview patch, and the difference is the DOM event name change to SelectionStateEvent.
Attachment #8529603 - Flags: review+
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+
Attachment #8526737 - Attachment description: WIP part 1 refactor the selectionchange dispatch → part 1 refactor the selectionchange dispatch
Attachment #8529593 - Attachment description: WIP-v3 part 2 Move the selectionchange filter logic to browserelementchild → part2-v3 Move the selectionchange filter logic to browserelementchild
Rename to SelectionStateChanged
Attachment #8529597 - Attachment is obsolete: true
Attachment #8530012 - Flags: review+
Rename to SelectionStateChanged
Attachment #8529599 - Attachment is obsolete: true
Attachment #8530013 - Flags: review+
Rename to SelectionStateChanged
Attachment #8529603 - Attachment is obsolete: true
Attachment #8530015 - Flags: review+
Rename to SelectionStateChanged and carry with r+ from previous patch.
Attachment #8530016 - Flags: review+
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
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
Rebase to latest inbound
Attachment #8526737 - Attachment is obsolete: true
Flags: needinfo?(pchang)
Attachment #8530273 - Flags: review+
(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
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)
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+
You need to log in before you can comment on or make changes to this bug.