773819 - Avoid calls to getBoundingClientRect in text selection code

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Instead of calling getBoundingClientRect() on the handles before sending every mouse event, this patch caches the left/top values, and uses the aX/aY we get from the touch events to update the cache as we drag the handles.

Unfortunately, we do still need *some* calls to getBoundingClientRect(), such as after the page pans, or after we reverse the handle positions, but this patch significantly reduces the number of calls we're making.

I noticed a little bit of weirdness with the positioning of the handles when they reverse or when the selection completely collapses when they get right next to each other sometimes, but I verified those issues also exist on Nightly, so I can file other bugs about that.

Next, I'm going to work on a patch about the scrollX/scrollY problems.

Comment 1

[:Margaret Leibovic]

Attachment: patch

Comment 2

[:Margaret Leibovic]

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

Ugh, I'm testing this again after fixing some other patches in my queue, and it's being really laggy. I'll investigate more.

Comment 3

[:Margaret Leibovic]

Attachment: patch w/ flushLayout

I found the problem was some bad interaction with my patch for bug 773813. Testing that patch on its own, things were working fine, but now with this patch applied, I'm often getting the html element in _sendMouseEvents, so we're bailing before sending our mouse events.

Flipping the flushLayout param to true in that call is fixing the problem, so maybe we do need at least one layout flush for this to work right? :/

This at least minimizes the number of layout flushes while maintaining the the right behavior. We could investigate this more in a separate bug.

Comment 4

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

Comment on attachment 642132 [details] [diff] [review]
patch w/ flushLayout

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     // Initialize the cache
>-    this.cache = {};
>+    this.cache = { start: {}, end: {}};

Add a space-> end: {} };

>   moveSelection: function sh_moveSelection(aIsStartHandle, aX, aY) {
>     // Update the handle position as it's dragged
>     let leftTop = "left:" + (aX + this._view.scrollX - this._viewOffset.left) + "px;" +
>                   "top:" + (aY + this._view.scrollY - this._viewOffset.top) + "px;";

Still can remove the view.scrollX / Y

Comment 5

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #4)

> Still can remove the view.scrollX / Y

See bug 773864.

Comment 6

[:Margaret Leibovic]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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