Closed Bug 779930 Opened 12 years ago Closed 12 years ago

Text-selection handles flip and collide in RTL direction text

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox15 affected, firefox16 affected, firefox17 affected, firefox18 affected, firefox19 fixed, fennec18+)

RESOLVED FIXED
Firefox 19
Tracking Status
firefox15 --- affected
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected
firefox19 --- fixed
fennec 18+ ---

People

(Reporter: aaronmt, Assigned: sriram)

References

()

Details

Attachments

(3 files)

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)
I think this has something to do with the '18-' in my test case, since that's not RTL the handles get fooled.
tracking-fennec: ? → 15+
Assignee: margaret.leibovic → nobody
Component: General → Text Selection
Renoming.
tracking-fennec: 15+ → ?
(for context, we saw this coming up in Play feedback)
Assignee: nobody → sriram
tracking-fennec: ? → 18+
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.
Attachment #667199 - Flags: review?(mark.finkle)
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.
Attachment #667219 - Flags: review?(mark.finkle)
Status: NEW → ASSIGNED
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.
Attachment #667199 - Flags: review?(wjohnston)
Attachment #667219 - Flags: review?(wjohnston)
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.
Attachment #667199 - Flags: review?(wjohnston) → review+
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).
Attachment #667219 - Flags: review?(wjohnston) → review+
Attachment #667219 - Flags: review?(mark.finkle) → review+
Attachment #667199 - Flags: review?(mark.finkle) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/fa5a1d02268e
https://hg.mozilla.org/mozilla-central/rev/24d449c76fe6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: