765057 - Reverse text selection handles when the start handle goes past the end of the selection and vice versa

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

Splitting this off from bug 695173, since the patch in there is getting pretty big.

Comment 1

[:Margaret Leibovic]

Attachment: patch

(Depends on an updated version of the patch in bug 695173)

I found that this runs into problems if the handles overlap each other, but that problem is also present with just the patch from bug 695173, so I think we can track that separately.

Comment 2

[:Margaret Leibovic]

Attachment: patch

(Un-bitrotted)

Comment 3

[:Margaret Leibovic]

Attachment: patch

Whoops, messed up with some copy/pasting.

Also, I'm downgrading this request to feedback, since I noticed that there are some edge case problems with this approach. I think it would be better to actually check coordinates of the selection itself to see if it reversed, rather than the coordinates of the handles. I'm going to try that now.

Comment 4

[Matt Brubeck (:mbrubeck)]

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

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

::: mobile/android/chrome/content/browser.js
@@ +1510,5 @@
> +    if (aIsStartHandle) {
> +      // Check to see if the start handle went below the end handle
> +      endRect = this._end.getBoundingClientRect();
> +      if (aY > endRect.top || (aY == endRect.top && aX > endRect.left))
> +        handlesFlipped = true;

So they'll flip if the start handle gets lower than the end by one pixel?  That seems a little too touchy to me (though I haven't finished building this patch to see it in action).

Ideally we could check if the start handle had moved to a lower line than the end handle, but I'm not sure that's feasible.  Maybe instead there could be a threshold of about 10px before flipping them?

Comment 5

[:Margaret Leibovic]

Attachment: (Part 1) Refactor updateCacheFromRange and moveSelection to update the cache on each touchmove

I made a new patch that checks the actual selection coordinates to see if it's reversed, but for that to work I first needed to do a little bit of refactoring. This patch udpates the cached start/end values on touchmove, instead of touchend, since we need that to decide whether or not the selection has reversed itself. I also found that I could get rid of finishMoveSelection, since we'll already be updating the cache on touchmove (and we don't actually need to update the offset on touchend, since we already update it on touchstart).

Comment 6

[:Margaret Leibovic]

Attachment: (Part 2) Reverse handles if necessary

I'm pretty happy with this approach. I've found it works really well, although we still need to deal with bug 765072 if we want super precision.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 634630 [details] [diff] [review]
(Part 2) Reverse handles if necessary

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

These patches look good, but they make some existing bugs a bit easier to trigger or more visible; not sure if you want to address those here or in a separate bug.

Comment 8

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de6e7c9a28c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca47af3af87

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de6e7c9a28c0
https://hg.mozilla.org/mozilla-central/rev/6ca47af3af87

Comment 10

[:Margaret Leibovic]

Uplifted to aurora as part of a roll-up patch:
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fb0a358eaf6

Comment 11

[Aaron Train [:aaronmt]]

Looks good, verifying this (15.0 Beta 2, Build #2), and via dailys