779930 - Text-selection handles flip and collide in RTL direction text

Status: RESOLVED FIXED

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

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (08/02) - Screenshot

See URL testcase

Begin a selection on the first word on the far right, first sentence in the paragraph and drag the left selection handle to the left. A couple words in and both handles will jump to the middle of the sentence collide and flip.

See screenshot

--
Samsung Galaxy Nexus (Android 4.1.1)
Nightly (08/02)

Comment 1

[Aaron Train [:aaronmt]]

I think this has something to do with the '18-' in my test case, since that's not RTL the handles get fooled.

Comment 2

[Aaron Train [:aaronmt]]

Renoming.

Comment 3

[Alex Keybl [:akeybl]]

(for context, we saw this coming up in Play feedback)

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/2): Selection at ends

For LTR:
Handles should be at left of start and right of end.
For RTL:
Handles should be at right of start and left of end.

This has been fixed in this patch.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/2): Reflecting images for RTL

This patch reflects the images for start and end for RTL (so they are outside the selection). Basically I created levels for handle_start and handle_end and flip the levels for RTL.

Comment 6

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

Comment on attachment 667199 [details] [diff] [review]
Patch (1/2): Selection at ends

Getting Wes to look at these so he gets ramped up on this code too.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 667199 [details] [diff] [review]
Patch (1/2): Selection at ends

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

I think this is fine.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 667219 [details] [diff] [review]
Patch (2/2): Reflecting images for RTL

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

I don't really like using levels for this stuff. I have a feeling its an optimization? If you feel strongly this is the right way to do things, it seems fine, but can you give some feedback about why?

::: mobile/android/base/TextSelectionHandle.java
@@ +105,4 @@
>          else if (mHandleType.equals(HandleType.MIDDLE))
>              left +=  (float) ((mWidth - mShadow) / 2);
>          else
> +            left += mIsRTL ? mWidth - mShadow : mShadow;

This code is duplicated in two places here. We should move it into a shared function.

@@ +131,5 @@
>          }
>  
>          mGeckoPoint = new PointF((float) left, (float) top);
> +        mIsRTL = rtl;
> +        setImageLevel(mIsRTL ? 1 : 0);

Why use this instead of setImageDrawable? This seems harder to understand to me. We should optimize to only change this when mIsRTL actually changes as well (i.e. wrap this in a if (mIsRTL != rtl) line).

Comment 9

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

(In reply to Wesley Johnston (:wesj) from comment #8)
> Comment on attachment 667219 [details] [diff] [review]
> Patch (2/2): Reflecting images for RTL
> 
> Review of attachment 667219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really like using levels for this stuff. I have a feeling its an
> optimization? If you feel strongly this is the right way to do things, it
> seems fine, but can you give some feedback about why?

> This code is duplicated in two places here. We should move it into a shared
> function.

> Why use this instead of setImageDrawable? This seems harder to understand to
> me. We should optimize to only change this when mIsRTL actually changes as
> well (i.e. wrap this in a if (mIsRTL != rtl) line).

Please deal with Wes' comments before landing, or make a new patch to tweak things based on his comments.

Comment 10

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5a1d02268e
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d449c76fe6

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fa5a1d02268e
https://hg.mozilla.org/mozilla-central/rev/24d449c76fe6