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)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox35 verified, fennec35+)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
fennec 35+ ---

People

(Reporter: lucasr, Assigned: mcomella)

References

Details

Attachments

(2 files)

Mike, I suspect this is regression from your changes in bug 1058909. Could you have a look?
Attached image Screenshot
Thanks for filing, I also noticed this.
(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
Regression window wanted - at Nightly increments should be fine.
tracking-fennec: --- → ?
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
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.
Attachment #8496928 - Flags: review?(lucasr.at.mozilla) → review+
(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?
(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?
FWIW, the reason I'm not too concerned about hit areas here is that the actual hit area is favicon+lock icon on phones.
https://hg.mozilla.org/mozilla-central/rev/20c7e70e1b1a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Status: RESOLVED → VERIFIED
tracking-fennec: ? → 35+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.