Closed
Bug 1073449
Opened 10 years ago
Closed 10 years ago
Regression: the default favicon icon looks too big on phones
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox35 verified, fennec35+)
VERIFIED
FIXED
Firefox 35
People
(Reporter: lucasr, Assigned: mcomella)
References
Details
Attachments
(2 files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
Mike, I suspect this is regression from your changes in bug 1058909. Could you have a look?
Reporter | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
Thanks for filing, I also noticed this.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #0)
> Mike, I suspect this is regression from your changes in bug 1058909.
Backed out bug 1058909 locally and the issue still remains.
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
Regression window wanted - at Nightly increments should be fine.
Keywords: regressionwindow-wanted
Updated•10 years ago
|
tracking-fennec: --- → ?
Comment 5•10 years ago
|
||
Regression window:
mozilla central:
good build: 25-09
bad build: 26-09
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1735ff2bb23e&tochange=9e3d649b80a2
inbound:
good build: 1411644116
bad build: 1411649004
pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e2c803c2aeec&tochange=128d8d99b72d
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 6•10 years ago
|
||
Mike, this is what caused the regression:
https://hg.mozilla.org/mozilla-central/rev/540bc6af7020
More specifically, you removed the 4dp left padding from the favicon view (in toolbar_display_layout.xml) which ends up giving it more space for the image.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8496928 -
Flags: review?(lucasr.at.mozilla)
Reporter | ||
Updated•10 years ago
|
Attachment #8496928 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Mike, this is what caused the regression:
Yep, seems you are correct - I don't think I noticed the difference in size when testing. x_x
Here's a patch, however, ideally we'd use margins to offset the favicon so that the favicon size we specify is exact and when we put it into memory, we don't have to store larger favicons than necessary. We can't, however, because margins are not part of the hit area of a button (at least in my experiences, at least on older devices).
To avoid code duplication, I don't think it's worth having two resource values, however. We could add a Favicons.getFaviconSize method, but that seems messy considering we'd need to pull in the layout to get and subtract away padding.
Is this the optimal change, Lucas?
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #8)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > Mike, this is what caused the regression:
>
> Yep, seems you are correct - I don't think I noticed the difference in size
> when testing. x_x
>
> Here's a patch, however, ideally we'd use margins to offset the favicon so
> that the favicon size we specify is exact and when we put it into memory, we
> don't have to store larger favicons than necessary. We can't, however,
> because margins are not part of the hit area of a button (at least in my
> experiences, at least on older devices).
>
> To avoid code duplication, I don't think it's worth having two resource
> values, however. We could add a Favicons.getFaviconSize method, but that
> seems messy considering we'd need to pull in the layout to get and subtract
> away padding.
>
> Is this the optimal change, Lucas?
Not optimal but good enough. We'll need to revise the toolbar paddings/margins to ensure consistent hit areas throughout. File a follow-up?
Reporter | ||
Comment 10•10 years ago
|
||
FWIW, the reason I'm not too concerned about hit areas here is that the actual hit area is favicon+lock icon on phones.
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
status-firefox35:
--- → verified
Updated•10 years ago
|
tracking-fennec: ? → 35+
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
•