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.

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?

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

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.

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

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 ?

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

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

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

Anthony, what device is your screenshot from?

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

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

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.

Because the last fix didn't cover all cases

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?

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.

Confirming that this is on 36.0b1.

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

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.

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

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

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

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

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

Same here, it doesn't look fixed.

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.

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

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

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

Comment on attachment 8555517 [details] [diff] [review] Offset overlap by an additional pixel for fade in toolbar. r=trivial I forgive you Ryan ;)