Closed Bug 1118136 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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."
Attachment #8544393 - Flags: feedback?(tlin)
Attachment #8544393 - Flags: feedback?(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.
Attachment #8544393 - Flags: feedback?(pchang) → feedback-
(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?
Attachment #8544393 - Attachment is obsolete: true
Attachment #8544393 - Flags: feedback?(tlin)
Attachment #8544476 - Flags: feedback?(tlin)
Attachment #8544476 - Flags: feedback?(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?
(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.
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)
Attachment #8544982 - Flags: feedback?(tlin) → feedback+
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+
Attachment #8544982 - Flags: feedback?(tlin) → feedback+
Addressed to reviewer's comment.
Attachment #8544982 - Attachment is obsolete: true
Attachment #8545051 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/9f3ee2b72d35
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.