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

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
Text Selection
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 16
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox15 verified, firefox16 verified, firefox17 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Splitting this off from bug 695173, since the patch in there is getting pretty big.
(Assignee)

Comment 1

6 years ago
Created attachment 633302 [details] [diff] [review]
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.
Assignee: nobody → margaret.leibovic
Attachment #633302 - Flags: review?(mbrubeck)
(Assignee)

Comment 2

6 years ago
Created attachment 633680 [details] [diff] [review]
patch

(Un-bitrotted)
Attachment #633302 - Attachment is obsolete: true
Attachment #633302 - Flags: review?(mbrubeck)
Attachment #633680 - Flags: review?(mbrubeck)
(Assignee)

Comment 3

6 years ago
Created attachment 633694 [details] [diff] [review]
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.
Attachment #633680 - Attachment is obsolete: true
Attachment #633680 - Flags: review?(mbrubeck)
Attachment #633694 - Flags: feedback?(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?
Attachment #633694 - Flags: feedback?(mbrubeck) → feedback+
(Assignee)

Comment 5

6 years ago
Created attachment 634627 [details] [diff] [review]
(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).
Attachment #633694 - Attachment is obsolete: true
Attachment #634627 - Flags: review?(mbrubeck)
(Assignee)

Comment 6

6 years ago
Created attachment 634630 [details] [diff] [review]
(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.
Attachment #634630 - Flags: review?(mbrubeck)
Attachment #634627 - Flags: review?(mbrubeck) → review+
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.
Attachment #634630 - Flags: review?(mbrubeck) → review+

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/de6e7c9a28c0
https://hg.mozilla.org/mozilla-central/rev/6ca47af3af87
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

6 years ago
Uplifted to aurora as part of a roll-up patch:
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fb0a358eaf6
status-firefox15: --- → fixed
status-firefox16: --- → fixed
Component: General → Text Selection
Looks good, verifying this (15.0 Beta 2, Build #2), and via dailys
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: --- → verified
You need to log in before you can comment on or make changes to this bug.