Closed
Bug 1117130
Opened 10 years ago
Closed 10 years ago
URL bar border slightly covered by fading edge of title
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(firefox36 fixed, firefox37 fixed, firefox38 fixed, fennec36+)
RESOLVED
FIXED
Firefox 38
People
(Reporter: antlam, Assigned: mhaigh)
References
Details
(Keywords: regression, reproducible)
Attachments
(4 files, 6 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
tracking-fennec: --- → ?
Keywords: regression,
regressionwindow-wanted
Comment 2•10 years ago
|
||
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?
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Blocks: new-tablet-v1
Flags: needinfo?(mhaigh)
Comment 4•10 years ago
|
||
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•10 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 6•10 years ago
|
||
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+
Comment 7•10 years ago
|
||
I bet this will need uplift to Fx36 too since bug 1105472 is on Fx36 now too.
Updated•10 years ago
|
tracking-fennec: ? → 36+
status-firefox36:
--- → affected
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 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?
Comment 10•10 years ago
|
||
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).
Reporter | ||
Comment 12•10 years ago
|
||
Nexus 5, Nightly. I've seen it on a Nexus 6 before too.
Flags: needinfo?(alam)
Comment 13•10 years ago
|
||
Margaret saw this today too not sure which device she was using
Assignee | ||
Updated•10 years ago
|
Attachment #8544686 -
Flags: approval-mozilla-aurora?
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee | ||
Comment 15•10 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•10 years ago
|
||
Because the last fix didn't cover all cases
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•10 years ago
|
Attachment #8545281 -
Flags: review?(wjohnston)
Comment 17•10 years ago
|
||
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•10 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•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(wjohnston)
Updated•10 years ago
|
Keywords: regressionwindow-wanted → reproducible
Comment 24•10 years ago
|
||
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•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(wjohnston)
Assignee | ||
Comment 26•10 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.
Comment 27•10 years ago
|
||
Since we're going to land as is (comment 14), this is fixed.
NI for uplift.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(mhaigh)
Resolution: --- → FIXED
Updated•10 years ago
|
Hardware: x86 → All
Updated•10 years ago
|
Attachment #8544686 -
Attachment is obsolete: false
Updated•10 years ago
|
Attachment #8545281 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8545856 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8545857 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8545858 -
Attachment is obsolete: true
Assignee | ||
Comment 28•10 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?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8550213 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•10 years ago
|
||
Comment 30•10 years ago
|
||
Verified as fixed in Firefox for Android 37.0a2 (2015-01-19);
Device: Samsung Galaxy S4 (Android 4.4.2).
Comment 31•10 years ago
|
||
Verified as fixed on Firefox for Android 36 Beta 2;
Device: Motorola Razr (Android 4.1.2).
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 32•10 years ago
|
||
I updated to Nightly today on my Nexus 6 and I'm still seeing this.. is it just me?
Comment 33•10 years ago
|
||
Same here, it doesn't look fixed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Updated•10 years ago
|
Comment 34•10 years ago
|
||
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?
Comment 35•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: mhaigh → michael.l.comella
Status: REOPENED → ASSIGNED
Updated•10 years ago
|
Assignee: michael.l.comella → mhaigh
Comment 36•10 years ago
|
||
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?
Comment 37•10 years ago
|
||
(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•10 years ago
|
||
Comment 39•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Attachment #8550213 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Target Milestone: Firefox 37 → Firefox 38
Comment 40•10 years ago
|
||
Comment 41•10 years ago
|
||
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+
Updated•9 years ago
|
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•