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

RESOLVED FIXED in mozilla36

Status

()

P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mtseng, Assigned: mtseng)

Tracking

unspecified
mozilla36
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8510911 [details] [diff] [review]
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.
Attachment #8510911 - Flags: review?(roc)
After applying this patch, SelectionCarets shows correctly after scrolling in the UITest app.
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?
Attachment #8510911 - Flags: review?(roc) → review-
(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.
Created attachment 8512441 [details] [diff] [review]
also-dispatch-to-docshell-child v2

Remove temporary array.
Attachment #8510911 - Attachment is obsolete: true
Attachment #8512441 - Flags: review?(roc)
Priority: -- → P2
Blocks: 1087193
(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.
Keywords: checkin-needed
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!
Flags: needinfo?(mtseng)
Created attachment 8513317 [details] [diff] [review]
also-dispatch-to-docshell-child v3 (carry r+: roc)

Rebased. This patch should be applied.
Attachment #8512441 - Attachment is obsolete: true
Flags: needinfo?(mtseng)
https://hg.mozilla.org/mozilla-central/rev/c48116c4a568
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.