Closed Bug 803514 Opened 7 years ago Closed 7 years ago

Like button is too close to the bookmark star

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 804068
Tracking Status
firefox17 --- wontfix
firefox18 --- wontfix

People

(Reporter: verdi, Assigned: jaws)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

The Facebook like button is too close to the bookmark star. I've hit the Like button when trying to bookmark something. It's closer to the star than the other buttons at the end of the location bar are to each other.
Jared, should be easy to just add some padding here?
Yeah, this is a little unconventional since the dropdown marker is not a normal 16x16 icon. We can add some margin-end to this button though so that it fits in better visually.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #675672 - Flags: review?(felipc) → review+
Comment on attachment 675672 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: social api like button will be located too close to the bookmark star
Testing completed (on m-c, etc.): locally, just landed on m-i
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none
Attachment #675672 - Flags: approval-mozilla-beta?
Attachment #675672 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/36603a91c06f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Attachment #675672 - Flags: approval-mozilla-beta?
Attachment #675672 - Flags: approval-mozilla-beta+
Attachment #675672 - Flags: approval-mozilla-aurora?
Attachment #675672 - Flags: approval-mozilla-aurora+
(In reply to Jared Wein [:jaws] from comment #2)
> Yeah, this is a little unconventional since the dropdown marker is not a
> normal 16x16 icon. We can add some margin-end to this button though so that
> it fits in better visually.

That's what the urlbar-icon class is there for. Changing the margin for individual items doesn't really make sense.
Status: RESOLVED → REOPENED
Component: SocialAPI → Theme
Resolution: FIXED → ---
Attachment #675672 - Flags: checkin+
tracking multiple landings in a single bug just doesn't work very well.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(In reply to Dão Gottwald [:dao] from comment #8)
> That's what the urlbar-icon class is there for. Changing the margin for
> individual items doesn't really make sense.

Jared and I discussed this, and the reason that the solution is share button-specific is that it's the only icon in the location bar that is not controlled by us. The other icons in the location bar have internal padding (in the image), but I don't think we want to require that from provider images (that's impossible to enforce). Nor do I think we want to require an icon whose dimensions aren't 16x16px.

We could fix the other url bar icons to not having internal padding and instead be consistent with padding in CSS, but I don't think that needs to block this bug.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > That's what the urlbar-icon class is there for. Changing the margin for
> > individual items doesn't really make sense.
> 
> Jared and I discussed this, and the reason that the solution is share
> button-specific is that it's the only icon in the location bar that is not
> controlled by us. The other icons in the location bar have internal padding
> (in the image), but I don't think we want to require that from provider
> images (that's impossible to enforce). Nor do I think we want to require an
> icon whose dimensions aren't 16x16px.
> 
> We could fix the other url bar icons to not having internal padding and
> instead be consistent with padding in CSS, but I don't think that needs to
> block this bug.

If I remember correctly the last time I updated that area I removed the image padding (at least from the dropdown arrow, bookmark star and the popup blocked icon). The only thing that should have different rules is the stop/go/reload button. Anything with the urlbar-icon class should have 3px padding added on each side for a total of 6px of space between items. 

It's not clear to me from the screenshot why there is only 3px of spacing between the share icon and the bookmark star if it is using the urlbar-icon class.
(In reply to Stephen Horlander from comment #11)
> If I remember correctly the last time I updated that area I removed the
> image padding (at least from the dropdown arrow, bookmark star and the popup
> blocked icon).

This is correct.
So who can explain why this patch is correct?
Has anyone even tested this stuff before patching, reviewing and landing it? I suspect this was just a symptom of bug 804068. I also suspect the patch that landed here just broke the alignment again.
Depends on: 804068
No longer depends on: 806477
Backed out from mozilla-central. Jared, please take care of aurora and beta.

https://hg.mozilla.org/mozilla-central/rev/3dccec0b3d50
https://hg.mozilla.org/mozilla-central/rev/2cb6d26fcfe7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #675672 - Flags: checkin+
(In reply to Dão Gottwald [:dao] from comment #14)
> Has anyone even tested this stuff before patching, reviewing and landing it?

Yes, I tested it and it looked correct. I'm not sure who would be doing otherwise.

> I suspect this was just a symptom of bug 804068. I also suspect the patch
> that landed here just broke the alignment again.

You are probably right that it was a symptom of bug 804068.

https://hg.mozilla.org/releases/mozilla-beta/rev/3fc5bf64bac5
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b37f641dbc1
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → DUPLICATE
Target Milestone: Firefox 19 → ---
Duplicate of bug: 804068
You need to log in before you can comment on or make changes to this bug.