1338231 - [RTL] FadedSingleColorTextView isn't fading RTL text

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Tom Klein]

Attachment: 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

[Itiel]

(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

[Itiel]

Attachment: RTL text not being faded

Comment 3

[Tom Klein]

Attachment: 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

Comment 4

[Max Liu [:maliu]]

(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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 6

[Jan Henning [:JanH]]

(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> (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.

For implementing bug 1271998, I've wrapped the URL bar display text (which is a FadedMultiColorTextView) into a HorizontalScrollView. This means that the TextView must display it's full width and can no longer use it's own fading, so instead I've enabled the ScrollView's fading edge.

After enabling GPU profiling, I noticed that with my patches for bug 1271998, the GPU usage was somewhat higher, especially when scrolling up as compared to scrolling down. I eventually realised that this was tied to the URL bar: When scrolling down, it soon gets hidden (and GPU usage falls), while it's always shown when scrolling back up.

For further testing, I've used a simple document stored under two different URLs:
- one short enough to fit into the URL bar without requiring fading (http://www.buttercookie.de/t/a.htm)
- and a longer one which does get faded out, at least on my phone (http://www.buttercookie.de/t/basic_article_mobile.html).

I also disabled fullscreen browsing so as to avoid any noise caused by the URL bar being shown or not. The result was that with fading active, GPU usage was indeed noticeably higher than without "fadingEdge" fading.

Comment 7

[Jan Henning [:JanH]]

Attachment: without fading.png

Comment 8

[Jan Henning [:JanH]]

Attachment: with fading.png

Test device was a Moto G4 Play with Android 6.0.1.

Comment 9

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.