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

RESOLVED FIXED in mozilla37

Status

()

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

People

(Reporter: mtseng, Assigned: mtseng)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

+++ 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."
Created attachment 8544393 [details] [diff] [review]
Dispatch scroll end only when carets are visible.
Attachment #8544393 - Flags: feedback?(tlin)
Attachment #8544393 - Flags: feedback?(pchang)

Comment 2

3 years ago
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.
Attachment #8544393 - Flags: feedback?(pchang) → feedback-

Comment 3

3 years ago
(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?
Created attachment 8544476 [details] [diff] [review]
Check if targets match before forcing dispatch selection state changed event.
Attachment #8544393 - Attachment is obsolete: true
Attachment #8544393 - Flags: feedback?(tlin)
Attachment #8544476 - Flags: feedback?(tlin)
Attachment #8544476 - Flags: feedback?(pchang)

Comment 5

3 years ago
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

3 years ago
(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.
Created attachment 8544982 [details] [diff] [review]
Check if targets match before forcing dispatch selection state changed event v2.

Update to addressed comment.
Attachment #8544476 - Attachment is obsolete: true
Attachment #8544476 - Flags: feedback?(tlin)
Attachment #8544476 - Flags: feedback?(pchang)
Attachment #8544982 - Flags: feedback?(tlin)
Attachment #8544982 - Flags: feedback?(pchang)

Comment 8

3 years ago
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.
Attachment #8544982 - Flags: feedback?(tlin)
Attachment #8544982 - Flags: feedback?(pchang)
Attachment #8544982 - Flags: feedback+
Created attachment 8545051 [details] [diff] [review]
Check if targets match before forcing dispatch selection state changed event v3.

Addressed to reviewer's comment.
Attachment #8544982 - Attachment is obsolete: true
Attachment #8545051 - Flags: review?(roc)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f3ee2b72d35
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.