1117130 - URL bar border slightly covered by fading edge of title

Status: RESOLVED FIXED

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

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screenshot_2015-01-02-08-35-51.png

Noticed this today, is it just me? There's a break in the outline of the URL bar which starts/stops when the fading text stops (and icons like reader mode appear)

Attaching screenshot.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Regression window:
good build: 31-12-2014
bad build: 01-01-2015
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88037f94b7d7&tochange=3c296aa11c51
Is it Bug 1105472?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

With Mike on PTO for a while, I'm NI'ing Martyn.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Top and bottom in FadedMultiColorTextView.onDraw needs to be changed - I'm not sure the best way to find this value as the values I used looked fine on my N7.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: URL bar border slightly covered by fading edge of title

The fade was a little too big and was overlapping the textview border by a pixel at the top and bottom.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8544602 [details] [diff] [review]
URL bar border slightly covered by fading edge of title


>+            final float top = center - getTextSize() + 1;
>+            final float bottom = center + getTextSize() - 1;

How about a comment about adding a 1 px (or is it dp) padding to prevent overlapping the border ?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

I bet this will need uplift to Fx36 too since bug 1105472 is on Fx36 now too.

Comment 8

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/ca5523cac817

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: URL bar border slightly covered by fading edge

Approval Request Comment
[Feature/regressing bug #]:
Reduce the height of the gradient used in the url bar.

[User impact if declined]:
Toolbar will border will have a visual artifact where the gradient is used

[Describe test coverage new/current, TBPL]:
None

[Risks and why]: 
It's a very small change in which we add or subtract 1 from a value used to calculate the height of the gradient.  The only thing that could go wrong is that the gradient may be incorrectly drawn when in use.

[String/UUID change made/needed]:
N/A

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: FadedMultiColoTextView is a generic class and I'm not sure this a generic solution, particularly because the size seems to change across devices (see bug 1105272 comment 18).

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, what device is your screenshot from?

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Nexus 5, Nightly. I've seen it on a Nexus 6 before too.

Comment 13

[Aaron Train [:aaronmt]]

Margaret saw this today too not sure which device she was using

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca5523cac817

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: URL bar border slightly covered by fading edge of title

mcomella: The bug you linked to in comment 10 was the wrong one I think?  Anyway have worked out a more generic approach which is way less hacky.

Comment 16

[Martyn Haigh (:mhaigh)]

Because the last fix didn't cover all cases

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 8545281 [details] [diff] [review]
URL bar border slightly covered by fading edge of title

Review of attachment 8545281 [details] [diff] [review]:
-----------------------------------------------------------------

Question before I +

::: mobile/android/base/widget/FadedMultiColorTextView.java
@@ +54,5 @@
>  
>              updateGradientShader(needsEllipsis, right);
> +            
> +            final Paint.FontMetrics fontMetrics = getPaint().getFontMetrics();
> +            final float totalFontHeight = fontMetrics.bottom - fontMetrics.top;

From the docs it sounds like these are the distance from the baseline. i.e. They're not both measured in the same coordinate system? Should this be?

fontMetrics.bottom + fontMetrics.top

You have a sec to post a pic where you paint the overlay rect rgba(255,0,0,0.5) or something?

Comment 18

[Martyn Haigh (:mhaigh)]

From http://developer.android.com/reference/android/graphics/Paint.FontMetrics.html: "Remember, Y values increase going down, so those values will be positive, and values that measure distances going up will be negative."  

In my tests bottom is coming out as a positive number and top is a negative number because bottom is "The maximum distance below the baseline for the lowest glyph in the font at a given text size." and top is "The maximum distance above the baseline for the tallest glyph in the font at a given text size" - hence I'm subtracting the top from the bottom.

For reference I'm getting values of 17.343 for bottom and -67.593 for top.

Comment 19

[Martyn Haigh (:mhaigh)]

Attachment: Small font size

Comment 20

[Martyn Haigh (:mhaigh)]

Attachment: Medium font size

Comment 21

[Martyn Haigh (:mhaigh)]

Attachment: Huge font size

Comment 23

[Aaron Train [:aaronmt]]

Confirming that this is on 36.0b1.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To follow up on comment 17 and comment 18, here is a programmatic example: http://porcupineprogrammer.blogspot.com/2012/10/android-font-metrics-for-dummies.html

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8545281 [details] [diff] [review]
URL bar border slightly covered by fading edge of title

Review of attachment 8545281 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ the suggested change, or a good explanation why not. :)

I also noticed that this assumes the text is centered vertically in the given View - can you add a comment to that effect in FadedMultiColorTextView's class javadoc? Also, file a followup to fix this (or just fix it here and skip the comment - the link in comment 24 might have useful info).

::: mobile/android/base/widget/FadedMultiColorTextView.java
@@ +53,4 @@
>              final float left = right - fadeWidth;
>  
>              updateGradientShader(needsEllipsis, right);
> +            

nit: whitespace at start of line

@@ +59,2 @@
>              final float center = getHeight() / 2;
> +            final float top = center - (totalFontHeight / 2);

Why don't we use fontMetrics.top directly here? We might be missing the top or bottom part of a glyph if the font is particularly unbalanced.

Same with bottom.

Comment 26

[Martyn Haigh (:mhaigh)]

Having discussed this, we think that the first patch is good enough for the moment as it seems to have fixed the issue, albeit in a bit of a hacky manner, so will flag that for uplift.  

Have created bug 1122081 to see the more generic approach through.

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Since we're going to land as is (comment 14), this is fixed.

NI for uplift.

Comment 28

[Martyn Haigh (:mhaigh)]

Attachment: URL bar border slightly covered by fading edge of title

Approval Request Comment
[Feature/regressing bug #]:
Reduce the height of the gradient used in the url bar.

[User impact if declined]:
Toolbar will border will have a visual artifact where the gradient is used

[Describe test coverage new/current, TBPL]:
None

[Risks and why]: 
It's a very small change in which we add or subtract 1 from a value used to calculate the height of the gradient.  The only thing that could go wrong is that the gradient may be incorrectly drawn when in use.

[String/UUID change made/needed]:
N/A

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca609e2e5bea

Comment 30

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in Firefox for Android 37.0a2 (2015-01-19);
Device: Samsung Galaxy S4 (Android 4.4.2).

Comment 31

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed on Firefox for Android 36 Beta 2;
Device: Motorola Razr (Android 4.1.2).

Comment 32

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screenshot_2015-01-27-09-58-59.png

I updated to Nightly today on my Nexus 6 and I'm still seeing this.. is it just me?

Comment 33

[Aaron Train [:aaronmt]]

Same here, it doesn't look fixed.

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8550213 [details] [diff] [review]
URL bar border slightly covered by fading edge of title

See comment 28.

Will land a second patch to fix the reopen issue.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Offset overlap by an additional pixel for fade in toolbar. r=trivial

Damn, :antlam has some hawk eyes.

I just subtracted an additional unit off the end of the overlay, but we may end
up going to far in the opposite direction (starting to uncover text) on certain
devices.

Let me know if you see anything.

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8555517 [details] [diff] [review]
Offset overlap by an additional pixel for fade in toolbar. r=trivial

Approval Request Comment
[Feature/regressing bug #]: This bug

[User impact if declined]:
  Users on some devices (perhaps only the N6 with it's ultra high density screen) will see a slight gap in the URL bar where the overlay is visible.

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  We're universally subtracting 1 more pixel from the height in both directions which may start to uncover the text we're using the overlay to hide on potentially more devices than we're having the issue causing the reopen on. Alternatively, we should really be determining the offset dynamically but we don't really have time to do that.
  But since we're just changing arithmetic, there are functionality should not be affected negatively.

[String/UUID change made/needed]: None

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #36)
> Alternatively, we should really be determining the offset
> dynamically but we don't really have time to do that.

bug 1122081.

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/86cba9403d81

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86cba9403d81

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8af71c5b3b6f
https://hg.mozilla.org/releases/mozilla-beta/rev/4b9480a0f719

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8555517 [details] [diff] [review]
Offset overlap by an additional pixel for fade in toolbar. r=trivial

I forgive you Ryan ;)