[RTL] FadedSingleColorTextView isn't fading RTL text

NEW
Unassigned

Status

()

Firefox for Android
Theme and Visual Design
6 months ago
5 months ago

People

(Reporter: Tom Klein, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 affected)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 months ago
Created attachment 8835595 [details]
The title on the last tab doesn't fade to the left

Noticed in the first screenshot in bug 1337897: in the last (selected) tab in that screenshot, the RTL tab title should fade as it approaches the close button.

From ItielMaN's bug 1337897 comment 6: "AFAIK this happens only to Hebrew and Arabic characters (it may happen for other langs as well)."

Comment 1

6 months ago
(In reply to Tom Klein from comment #0)
> Created attachment 8835595 [details]
> The title on the last tab doesn't fade to the left
> 
> Noticed in the first screenshot in bug 1337897: in the last (selected) tab
> in that screenshot, the RTL tab title should fade as it approaches the close
> button.
> 
> From ItielMaN's bug 1337897 comment 6: "AFAIK this happens only to Hebrew
> and Arabic characters (it may happen for other langs as well)."

This issue is visible not only on tab titles but in other components such as search suggestions and hisotry.

Comment 2

6 months ago
Created attachment 8835604 [details]
RTL text not being faded
(Reporter)

Comment 3

6 months ago
Created attachment 8835812 [details]
Using "requiresFadingEdge" works

Thanks for the extra info.

Looks like FadedSingleColorTextView doesn't fade when the text is RTL, even though it internally checks for and changes behavior for RTL text.  I played around a little bit with changing the order of arguments and values to the LinearGradient we use to do the fade at [1]; nothing helped, but some things made it worse.

If I fix isRTL=false in that constructor call (isRTL just determines in which direction to specify the linear gradient line, so should just change whether the fade occurs on the left or right) then RTL text disappears altogether but LTR fades on the right as expected, whereas if I fix isRTL=true then RTL text appears but with no fade (as we've already seen) and LTR text appears with a fade on the left side, as expected given the arguments. Also, setting the positions value in the constructor to null is supposed to evenly spread the supplied colors along the gradient line, which it does for LTR text, but RTL text just appears normally with no gradient (which isn't very surprising, but verifies that it wasn't the order of the positions values causing this to not work for RTL). So my current impression is that Android just doesn't fully support applying a gradient to RTL text in this way (which is a little confusing given that FadedSingleColorTextView seems to have been built to support RTL - maybe it depends on API version? Maybe I'm just missing something).

If I change the FadedSingleColorTextView to a TextView at [2] and add the attributes:
  android:requiresFadingEdge="horizontal"
  android:fadingEdgeLength="15dp"
  android:ellipsize="none"
then I get the attached screenshot - it looks like it works.  I'm not sure if we'll want to just discard all FadedSingleColorTextViews (and FadedMultiColorTextViews?) and replace them with requiresFadingEdge though; someone else will have to make that call.  Or even better, maybe someone will have an idea of how to fix FadedSingleColorTextView. :)

Max, any ideas here?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/FadedSingleColorTextView.java#76

[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/tabs_layout_item_view.xml#23
Flags: needinfo?(max)
(Reporter)

Updated

6 months ago
Summary: [RTL] Long tab titles don't fade in tabs grid layouts → [RTL] FadedSingleColorTextView isn't fading RTL text

Comment 4

6 months ago
(In reply to Tom Klein from comment #3)

> positions values causing this to not work for RTL). So my current impression
> is that Android just doesn't fully support applying a gradient to RTL text
> in this way (which is a little confusing given that FadedSingleColorTextView
I made the same conjecture when I patch FadedSingleColorTextView to support RTL last time.

> seems to have been built to support RTL - maybe it depends on API version?
> Maybe I'm just missing something).
It is not built to support RTL in the first place. And I couldn't figure out the root cause of above symptom, so I only fix the TextDirection part last time.
I guess it's related to how skia apply gradient on text with RTL direction.

> If I change the FadedSingleColorTextView to a TextView at [2] and add the
> attributes:
>   android:requiresFadingEdge="horizontal"
>   android:fadingEdgeLength="15dp"
>   android:ellipsize="none"
> then I get the attached screenshot - it looks like it works.  I'm not sure

Looks like it did work. But using fadingEdge could possible cause some scrolling performance issue, and that is the improvement which describes in javadoc.
```
 * This implementation is an improvement over Android's built-in fadingEdge
 * and the fastest of Fennec's implementations.
```
https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/mobile/android/base/java/org/mozilla/gecko/widget/FadedSingleColorTextView.java#23

Hi Sebastian,
  At that time when FadedSingleColorTextView is created(3+yr ago), our min sdk is 9, and it is 15 now.
  In order to support RTL, do we still have scrolling performance concern if we fallback to use fadingEdge?

> if we'll want to just discard all FadedSingleColorTextViews (and
> FadedMultiColorTextViews?) and replace them with requiresFadingEdge though;
> someone else will have to make that call.  Or even better, maybe someone
> will have an idea of how to fix FadedSingleColorTextView. :)

Replace FadedSingleColorTextView to TextView might break the Light/Dark/Private Theme function since they extend from ThemedTextView.
Quick workaround could be enable and set fading edge in constructor.
https://hg.mozilla.org/try/rev/ba7d13496f665b448faacbf68262109e860b9753
Flags: needinfo?(max) → needinfo?(s.kaspari)
(In reply to Max Liu [:maliu] from comment #4)
> Hi Sebastian,
>   At that time when FadedSingleColorTextView is created(3+yr ago), our min
> sdk is 9, and it is 15 now.
>   In order to support RTL, do we still have scrolling performance concern if
> we fallback to use fadingEdge?

I don't know! But let's try and test.

I found some history in bug 1105472 and sriram's blog, who worked at Mozilla at that time:
https://sriramramani.wordpress.com/2013/06/06/ellip-sis/

See the comment section of the blog posting which describes the issues seen back then. Should be straight-forward to do the same tests on a new device.
Flags: needinfo?(s.kaspari)
Blocks: 1319302
You need to log in before you can comment on or make changes to this bug.