1331431 - Faded Text View on the URL bar should also fade the bottom part of letters such as 'g', 'y' etc

Status: RESOLVED FIXED

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

Description

[Itiel]

Attachment: Screenshot

STR:
1. Open Nightly
2. Open a website whose URL has letters such as 'y', 'g' etc where the text would get faded out. For the sake of this STR, open a fake URL, something like www.yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy.com
3. Observe the faded out text

Actual results:
The bottom part of the letter 'y' isn't being faded out.

Expected results:
It should.

See screenshot attached.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This is a regression.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Could be caused by bug 1320605?

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

Review commit: https://reviewboard.mozilla.org/r/105424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105424/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Maybe this should track 52. It's a regression that started in 52 and we should avoid letting it ride to release.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

https://reviewboard.mozilla.org/r/105424/#review106242

This patch contains a patch file. :)

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105424/diff/1-2/

Comment 7

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> Maybe this should track 52. It's a regression that started in 52 and we
> should avoid letting it ride to release.

agree+1

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105424/diff/2-3/

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Textview doesn't who text in the center but on baseline. I've made a new patch.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

https://reviewboard.mozilla.org/r/105424/#review106644

::: mobile/android/base/java/org/mozilla/gecko/widget/FadedMultiColorTextView.java:20
(Diff revision 3)
>  import android.graphics.LinearGradient;
>  import android.graphics.Paint;
> +import android.graphics.Rect;
>  import android.graphics.Shader;
>  import android.util.AttributeSet;
> +import android.util.Log;

nit: unnecessary import - from debugging? :)

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105424/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21e0b0c5284a
Use baseline and FontMetrics instead of textview's center and text size to get the real text height. r=sebastian

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21e0b0c5284a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 15

[Itiel]

I can confirm this is fixed on latest Nightly.

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

Approval Request Comment
[Feature/Bug causing the regression]:Faded Text View on the URL bar should cover letters with descending ('g', 'y')
[User impact if declined]: User will see broken UI
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just fixing UI
[String changes made/needed]: no

Comment 17

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8827838 [details]
Bug 1331431 - Use baseline and FontMetrics instead of textview's center and text size to get the real text height.

fix fennec regression in beta52

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c822f8fbd65

Comment 19

[Sorina Florean [:sflorean]]

Verified as fixed in build 52.0b1 (52 Beta 1);
Device: Huawei MediaPad M2 (Android 5.1.1).