1320605 - Fade to white for border around URL box stuck for long URLs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Arvinder Bhathal]

Attachment: Screenshot_2.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/53.0.2785.34 Safari/537.36
Build ID: 20161114144856

Steps to reproduce:

[Nexus 5X, Android 7.0, Build NRD91N, Firefox Nightly v.53.0a1]

Visit any website where the URL is longer than the space available inside the URL box.


Actual results:

The border around the URL box transitions to white along with the text, seemingly to indicate the URL is longer than the space available. However, the right-side of the border is not subject to the transition effect and the resulting overall transition looks unnatural. If Firefox offers a reading view for the website, the transition ends just before the reading view icon.

I am not entirely sure this is a bug as the resulting effect may have been intended.

The attached image shows a website with Firefox offering a reading view icon.


Expected results:

The text of the URL should transition to white to indicate a long URL but the border should remain one colour.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

I can't see this on a Pixel XL.

@Max: random guess, could this be a regression from the RTL patches?

Comment 2

[Max Liu [:maliu]]

Not likely to be related to RTL patches, RTL only update the left/right to start/end. Also, RTL patch landed on 11/24. And the reported version is build on 11/14.

Comment 3

[Max Liu [:maliu]]

STR:
Change overall system font size will reproduce the symptom.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Max Liu [:maliu] from comment #3)
> STR:
> Change overall system font size will reproduce the symptom.

Ah, thanks! Now I can see it. And it is reproducible in all release channels.

Comment 5

[Arvinder Bhathal]

Ah yes, system font size set to "Large".

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1320605 - The shrunk height just nee to cover the text itself.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

https://reviewboard.mozilla.org/r/97116/#review97826

::: mobile/android/base/java/org/mozilla/gecko/widget/FadedMultiColorTextView.java:60
(Diff revision 1)
> -            final float top = center - getTextSize() + 2;
> -            final float bottom = center + getTextSize() - 2;
> +            // The shrunk size just nee to cover the text itself.
> +            final float top = center - getTextSize()/2 ;
> +            final float bottom = center + getTextSize()/2 ;

The patch looks good, but the formatting is off. It should look like this:
> getTextSize() / 2;

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

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

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

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

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Fixed in the 3rd patch. Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/594afa97ff78
The shrunk height just nee to cover the text itself. r=sebastian

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/594afa97ff78

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Sebastian, you marked it as tracking for 51. 
It might be too late for 51 but is it something you would like to uplift to 52? Thanks

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Yes, let's uplift this. Nevin, can you request it?

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

Approval Request Comment
[Feature/regressing bug #]:  The shrunk height just need to cover the text itself.

[User impact if declined]: User sees white gradient cause the height is larger than the input box height.

[Describe test coverage new/current, TreeHerder]: manually check the gradient color and height

[Risks and why]: Low. It just changes gradient color and its height

[String/UUID change made/needed]: -

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

fix fennec url box gradient for aurora52

FWIW both in the commit message and code comment, you wrote "nee" for "needs".

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6180973a5211

Comment 18

[Oana Horvath [:ohorvath]]

Verified as fixed on Aurora 52.0a2 (2017-01-13) and Nightly 53 (2017-01-13), with the following devices:
-LG G4 (Android 5.1) phone;
-HTC Nexus 9 (Android 7.0) tablet;
-Asus Transformer Pad (Android 4.2.1) tablet.

Comment 20

[Mike Kaply [:mkaply]]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

Approval Request Comment
[Feature/regressing bug #]:  The shrunk height just need to cover the text itself.
[User impact if declined]: User sees white gradient cause the height is larger than the input box height.
[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]: In bug
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Very low.
[Why is the change risky/not risky?]: It just changes gradient color and its height
[String changes made/needed]: None

I realize this is a long shot at this point, but I thought no harm in trying. This was a partner reported bug, and they are shipping 51 so I would love to see this in 51 if we can. It's a two line change.

Comment 21

[Julien Cristau [:jcristau]]

I'm confused by this beta approval request.  beta is 52 which had this patch uplifted already in comment 17.  51 is release, and 2 weeks before the 52 release it's fairly unlikely there's be another 51 build.

Comment 22

[Mike Kaply [:mkaply]]

I guess I didn't realize how close we were to 51.

If it's a no go for 51, that's OK. just thought I would try.

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8816354 [details]
Bug 1320605 - The shrunk height just nee to cover the text itself.

This is already in beta...

Comment 24

[Mike Kaply [:mkaply]]

Sorry, got my releases confused.