773813 - Avoid using ElementTouchHelper methods in text selection code

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[:Margaret Leibovic]

Attachment: patch

We want to avoid any calls to getBoundingClientRect. Unfortunately, I found that I couldn't get rid of the call in endSelection, since that would break tapping to copy text in iframes, but I decided to just change the call to anyElementFromPoint, since all we want is the element's ownerDocument.

(I also decided to get rid of a chunk of old commented-out code that we've never used in fennec native. I can file another bug to look into migrating that functionality from XUL fennec.)

Comment 1

[:Margaret Leibovic]

Comment on attachment 642076 [details] [diff] [review]
patch

I'm being aggressive about reviews, since it would be nice to get this on mozilla-central before the merge...

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 642076 [details] [diff] [review]
patch


>   _sendMouseEvents: function sh_sendMouseEvents(cwu, useShift, x, y) {
>-    let contentWindow = BrowserApp.selectedBrowser.contentWindow;
>-    let element = ElementTouchHelper.elementFromPoint(contentWindow, x, y);
>-    if (!element)
>-      element = ElementTouchHelper.anyElementFromPoint(contentWindow, x, y);
>-
>+    let element = BrowserApp.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+                                                           getInterface(Ci.nsIDOMWindowUtils).
>+                                                           elementFromPoint(x, y, false, false);

"cwu" is already being passed in. Can you just use it? Using anyElementFromPoint is frame-sensitive. Do we need this use to be frame-sensitive?

Comment 3

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 642076 [details] [diff] [review]
> patch
> 
> 
> >   _sendMouseEvents: function sh_sendMouseEvents(cwu, useShift, x, y) {
> >-    let contentWindow = BrowserApp.selectedBrowser.contentWindow;
> >-    let element = ElementTouchHelper.elementFromPoint(contentWindow, x, y);
> >-    if (!element)
> >-      element = ElementTouchHelper.anyElementFromPoint(contentWindow, x, y);
> >-
> >+    let element = BrowserApp.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> >+                                                           getInterface(Ci.nsIDOMWindowUtils).
> >+                                                           elementFromPoint(x, y, false, false);
> 
> "cwu" is already being passed in. Can you just use it? Using
> anyElementFromPoint is frame-sensitive. Do we need this use to be
> frame-sensitive?

That cwu is for _view, but we were calling elementFromPoint/anyElementFromPoint with selectedBrowser.contentWindow. However, I'm not sure if this difference is important, especially since we're only using this to try to make sure the point isn't on top of that handle element, and that would be in the _view. For that reason, I also don't think we need to worry about frames at this call.

Comment 4

[:Margaret Leibovic]

I decided that using the cwu that got passed in is fine. I tested on a page with iframes, and it worked as expected.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c293947bf9c

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c293947bf9c

Comment 6

[:Margaret Leibovic]

Comment on attachment 642076 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this fixes a performance problem that was part of the original text selection implementation, but it affects the native handles implementation as well
User impact if declined: slightly worse text selection performance
Testing completed (on m-c, etc.): landed on m-c 7/14 for Firefox 16
Risk to taking this patch (and alternatives if risky): low-risk (some of the patch will be paved over by the native handles patch anyway)
String or UUID changes made by this patch: n/a

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 642076 [details] [diff] [review]
patch

approved for beta as part of the dep bugs for native handles for text selection.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/58f2aa45934f