1088559 - [Text Selection] Selection carets are missing after scrolling inside iframe.

Status: RESOLVED FIXED

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

Description

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

STR

1. Open UITest app, switch to CopyPaste tab.
2. Long tap to select a word.
3. Scroll the page

Expected result:
Selection carets should remain on screen.

Actual result:
Selection carets are missing.

Comment 1

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

Attachment: also-dispatch-to-docshell-child

NotifyAsyncPanZoomStarted/Stopped only dispatch to top level nsDocShell. So if selection carets are inside iframe then those carets never receive NotifyAsyncPanZoomStarted/Stopped event and carets position never get updated.

This patch dispatch this event to nsDocShell's child as well.

Comment 2

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

After applying this patch, SelectionCarets shows correctly after scrolling in the UITest app.

Comment 3

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

Comment on attachment 8510911 [details] [diff] [review]
also-dispatch-to-docshell-child

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

::: docshell/base/nsDocShell.cpp
@@ +3120,5 @@
> +    uint32_t n = mChildList.Length();
> +    kids.SetCapacity(n);
> +    for (uint32_t i = 0; i < n; i++) {
> +        kids.AppendElement(do_QueryInterface(ChildAt(i)));
> +    }

Why are we using this temporary array?

Comment 4

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 8510911 [details] [diff] [review]
> also-dispatch-to-docshell-child
> 
> Review of attachment 8510911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +3120,5 @@
> > +    uint32_t n = mChildList.Length();
> > +    kids.SetCapacity(n);
> > +    for (uint32_t i = 0; i < n; i++) {
> > +        kids.AppendElement(do_QueryInterface(ChildAt(i)));
> > +    }
> 
> Why are we using this temporary array?

I just followed nsDocShell::FirePageHideNotification. Maybe we don't need this temporary array here. I'll attach new patch later.

Comment 5

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

Attachment: also-dispatch-to-docshell-child v2

Remove temporary array.

Comment 6

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

(In reply to Morris Tseng [:mtseng] from comment #4)
> I just followed nsDocShell::FirePageHideNotification. Maybe we don't need
> this temporary array here. I'll attach new patch later.

FirePageHideNotification needs a temporary array because it runs arbitrary JS which can alter the docshell tree. I don't think your notifications have that problem.

Comment 7

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

try run

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

Comment 8

[Carsten Book [:Tomcat]]

Hi Morris, this patch failed to apply:

Hunk #1 FAILED at 883
1 out of 1 hunks FAILED -- saving rejects to file layout/base/SelectionCarets.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh also-dispatch-to-docshell-child

could you take a look? Thanks!

Comment 9

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

Attachment: also-dispatch-to-docshell-child v3 (carry r+: roc)

Rebased. This patch should be applied.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c48116c4a568

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c48116c4a568