1118136 - [CopyPaste] Too many updateposition and scroll end event if there are several iframes.

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #1105547 +++

Quote from bug 1105547 comment 10.

"After bug 1114853 and bug 1110963 are landed, we remove handler for scrollend event, instead, we only redisplay the bubble by selectionstatechanged event after scrolling. 

However, I found out that we will receive two scroll start/end action when scrolling browser page, one is navigation bar and one is page itself. That's why cause this issue."

Comment 1

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

Attachment: Dispatch scroll end only when carets are visible.

Comment 2

[Peter Chang[:pchang]]

Comment on attachment 8544393 [details] [diff] [review]
Dispatch scroll end only when carets are visible.

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

::: layout/base/SelectionCarets.cpp
@@ +1163,3 @@
>  
> +    DispatchScrollViewChangeEvent(mPresShell, dom::ScrollState::Stopped, aScrollPos);
> +  }

I don't recommend to filter the scrollviewchange event based on the visibility of selectioncarets in gecko, just like we didn't filter the selectionchange events in NotifySelectionChanges.

Comment 3

[Peter Chang[:pchang]]

(In reply to Morris Tseng [:mtseng] from comment #0)
> +++ This bug was initially created as a clone of Bug #1105547 +++
> 
> Quote from bug 1105547 comment 10.
> 
> "After bug 1114853 and bug 1110963 are landed, we remove handler for
> scrollend event, instead, we only redisplay the bubble by
> selectionstatechanged event after scrolling. 
> 
> However, I found out that we will receive two scroll start/end action when
> scrolling browser page, one is navigation bar and one is page itself. That's
> why cause this issue."
Can we pass the identify information about the navigation bar or page itself to the scrollviewchange event?

Comment 4

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

Attachment: Check if targets match before forcing dispatch selection state changed event.

Comment 5

[Peter Chang[:pchang]]

Comment on attachment 8544476 [details] [diff] [review]
Check if targets match before forcing dispatch selection state changed event.

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +600,5 @@
>      let isMouseUp = (e.states.indexOf('mouseup') == 0);
>      let canPaste = this._isCommandEnabled("paste");
>  
> +    if (!this._forceDispatchSelectionStateChanged ||
> +        (this._forceDispatchSelectionStateChanged && (this._cachedTarget != e.target))) {

Does it work if we only check the target is matched or not?

Comment 6

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #5)
> Comment on attachment 8544476 [details] [diff] [review]
> Check if targets match before forcing dispatch selection state changed event.
> 
> Review of attachment 8544476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +600,5 @@
> >      let isMouseUp = (e.states.indexOf('mouseup') == 0);
> >      let canPaste = this._isCommandEnabled("paste");
> >  
> > +    if (!this._forceDispatchSelectionStateChanged ||
> > +        (this._forceDispatchSelectionStateChanged && (this._cachedTarget != e.target))) {
> 
> Does it work if we only check the target is matched or not?

As we discussed, please cache the event target in forceDsipatchSelectionStateChaged but you might need to rename the name first.

Comment 7

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

Attachment: Check if targets match before forcing dispatch selection state changed event v2.

Update to addressed comment.

Comment 8

[Peter Chang[:pchang]]

Comment on attachment 8544982 [details] [diff] [review]
Check if targets match before forcing dispatch selection state changed event v2.

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

f=me if the following comments are addressed.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +599,5 @@
>      let isCollapsed = (e.selectedText.length == 0);
>      let isMouseUp = (e.states.indexOf('mouseup') == 0);
>      let canPaste = this._isCommandEnabled("paste");
>  
> +    if (this._targetForForcingDispatchSelectionStateChanged != e.target) {

You also need to rename in function BrowserElementChild().

@@ +623,5 @@
>          }
>        }
>      }
>  
> +    // If we select something and selection range is visible, we cached current

s/cached/cache

@@ +625,5 @@
>      }
>  
> +    // If we select something and selection range is visible, we cached current
> +    // event's target to targetForForcingDispatchSelectionStateChanged.
> +    // And dispatch the next SelectionStateChagne event if target matched, so

if target is matched

@@ +631,2 @@
>      if (e.visible && !isCollapsed) {
> +      this._targetForForcingDispatchSelectionStateChanged = e.target;

Original the forceDsipatchSelectionStateChanged flag is used to dispatch the next SelectionStateChanged event(with the collapsed) to hide the text dialog.

Right now you are trying to combine this flag and the event target. I would suggest to name as SelectionStateChangedTarget and clear this target when selectioncarets is invisible(!e.visible) because no need to send out the SelectionStateChanged event.

Comment 9

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

Attachment: Check if targets match before forcing dispatch selection state changed event v3.

Addressed to reviewer's comment.

Comment 10

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

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3ee2b72d35

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f3ee2b72d35