URL bar border slightly covered by fading edge of title

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: antlam, Assigned: mhaigh)

Tracking

(Blocks 1 bug, {regression, reproducible})

unspecified
Firefox 38
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed, fennec36+)

Details

Attachments

(4 attachments, 6 obsolete attachments)

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.
tracking-fennec: --- → ?
Duplicate of this bug: 1117251
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?
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
With Mike on PTO for a while, I'm NI'ing Martyn.
Flags: needinfo?(mhaigh)
Assignee

Updated

5 years ago
Flags: needinfo?(mhaigh)
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.
Blocks: 1105472
Assignee

Comment 5

5 years ago
The fade was a little too big and was overlapping the textview border by a pixel at the top and bottom.
Assignee: michael.l.comella → mhaigh
Attachment #8544602 - Flags: review?(mark.finkle)
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 ?
Attachment #8544602 - Flags: review?(mark.finkle) → review+
I bet this will need uplift to Fx36 too since bug 1105472 is on Fx36 now too.
tracking-fennec: ? → 36+
Assignee

Comment 9

5 years ago
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
Attachment #8544602 - Attachment is obsolete: true
Attachment #8544686 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(alam)
Nexus 5, Nightly. I've seen it on a Nexus 6 before too.
Flags: needinfo?(alam)
Margaret saw this today too not sure which device she was using
Assignee

Updated

5 years ago
Attachment #8544686 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ca5523cac817
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee

Comment 15

5 years ago
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.
Attachment #8544686 - Attachment is obsolete: true
Attachment #8545281 - Flags: review?(michael.l.comella)
Assignee

Comment 16

5 years ago
Because the last fix didn't cover all cases
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

5 years ago
Attachment #8545281 - Flags: review?(wjohnston)
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?
Assignee

Comment 18

5 years ago
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.
Assignee

Comment 19

5 years ago
Posted image Small font size (obsolete) —
Assignee

Comment 20

5 years ago
Posted image Medium font size (obsolete) —
Assignee

Comment 21

5 years ago
Posted image Huge font size (obsolete) —
Assignee

Updated

5 years ago
Flags: needinfo?(wjohnston)
Duplicate of this bug: 1120226
Confirming that this is on 36.0b1.
Status: REOPENED → ASSIGNED
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.
Attachment #8545281 - Flags: review?(wjohnston)
Attachment #8545281 - Flags: review?(michael.l.comella)
Attachment #8545281 - Flags: review+
Flags: needinfo?(wjohnston)
Assignee

Comment 26

5 years ago
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(mhaigh)
Resolution: --- → FIXED
Assignee

Comment 28

5 years ago
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
Attachment #8544686 - Attachment is obsolete: true
Flags: needinfo?(mhaigh)
Attachment #8550213 - Flags: approval-mozilla-beta?
Attachment #8550213 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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).
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: 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.
Attachment #8550213 - Flags: approval-mozilla-aurora?
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.
Assignee: mhaigh → michael.l.comella
Status: REOPENED → ASSIGNED
Assignee: michael.l.comella → mhaigh
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
Attachment #8555517 - Flags: approval-mozilla-beta?
Attachment #8555517 - Flags: approval-mozilla-aurora?
(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/mozilla-central/rev/86cba9403d81
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Attachment #8550213 - Flags: approval-mozilla-aurora?
Target Milestone: Firefox 37 → Firefox 38
Comment on attachment 8555517 [details] [diff] [review]
Offset overlap by an additional pixel for fade in toolbar. r=trivial

I forgive you Ryan ;)
Attachment #8555517 - Flags: approval-mozilla-beta?
Attachment #8555517 - Flags: approval-mozilla-beta+
Attachment #8555517 - Flags: approval-mozilla-aurora?
Attachment #8555517 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.