Closed Bug 984535 Opened 11 years ago Closed 11 years ago

different padding on buttons when social buttons enabled

Categories

(Firefox Graveyard :: SocialAPI, defect)

Product:
Firefox Graveyard
Component:
SocialAPI
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox28 unaffected, firefox29+ verified, firefox30+ verified, firefox31 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 31
Tracking Flags:
Tracking Status
firefox28 --- unaffected
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Australis:P3+])

Attachments

(4 files, 3 obsolete files)

withoutsocial.png
(deleted), image/png
Details
withsocial.png
(deleted), image/png
Details
withsocial2.png
(deleted), image/png
Details
osx padding fix
(deleted), patch
Gijs
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
It seems button padding changed again? Now there is different padding depending on whether social buttons are enabled. Images to follow.
Attached image withoutsocial.png (deleted) —
Attached image withsocial.png (deleted) —
Attached image withsocial2.png (deleted) —
I'm able to reproduce on completely clean profile with a build of fx-team just an hour or so ago. str: - create new profile with -ProfileManager - start fx - notice size of hover outlines and toolbar - modify social.directories pref with value http://mixedpuppy.github.io/socialapi-directory - go to above url, install G+ demo - notice during install the toolbar increases size - notice some buttons now have larger padding around image
I've tracked this down to too much margin-top and margin-bottom on badge-type toolbarbutton icons. There's 4px of padding on the top and bottom, and I think there should be 2px instead. I believe this is simply fallout from bug 980374.
Blocks: 980374
appears to only be an osx issue, verified correct appearance on win/lin baring a couple new bugs I discovered, including bug 984630
Attached patch fix padding (obsolete) (deleted) — — Splinter Review
Assignee: nobody → mixedpuppy
Attachment #8392559 - Flags: review?(mconley)
Shane, why do you think we should track it? It seems a bit minor to me and we would not block the release on this. Thanks :)
Maybe related to bug 968388.
Attached patch Patch v2: fix padding and toolbar button states (obsolete) (deleted) — — Splinter Review
Shane, I r+ your changes, but I also noticed that the updated toolbar styles also doesn't apply the hover & active states to social API buttons. That's because you use `disabled=false` instead of removing the attribute. This is completely legal, so I changed the CSS to have `[disabled=true]` checks instead of `[disabled]`.
Assignee: mixedpuppy → mdeboer
Attachment #8392559 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8392559 - Flags: review?(mconley)
Attachment #8392811 - Flags: review?(mixedpuppy)
Comment on attachment 8392811 [details] [diff] [review] Patch v2: fix padding and toolbar button states if we fix the disabled state here, we should do it for all platforms since I've seen this issue happen on linux and windows as well in bug 984628. I think it might be simpler to just fix the socialapi use of the disabled attribute, but also like this css change better.
Attachment #8392811 - Flags: review?(mixedpuppy) → feedback+
Would call this P2 except for it being SocialAPI specific. That said, it would be worth looking at what we historically did for this attribute. If it's one of those oddball attributes that just depend on presence but not value, 'twould be better to fix Social.
Whiteboard: [Australis:P3+]
fyi I have a fix coming separately for bug 984628 that addresses the disabled/hidden state. It's a bit more involved than just a css change, there is a bug here in the share button code. I'd prefer to do the smaller fix here.
Shane, r=me for the smaller fix (as mentioned in comment 10). You can check that in to resolve this bug.
(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > Comment on attachment 8392811 [details] [diff] [review] > Patch v2: fix padding and toolbar button states > > if we fix the disabled state here, we should do it for all platforms since > I've seen this issue happen on linux and windows as well in bug 984628. > > I think it might be simpler to just fix the socialapi use of the disabled > attribute, but also like this css change better. That is strange, because this was meant to be fixed by bug 973704. If bug 980374 regressed OS X, fine, but what regressed Windows/Linux? :|
(In reply to :Gijs Kruitbosch from comment #15) > (In reply to Shane Caraveo (:mixedpuppy) from comment #11) > > Comment on attachment 8392811 [details] [diff] [review] > > Patch v2: fix padding and toolbar button states > > > > if we fix the disabled state here, we should do it for all platforms since > > I've seen this issue happen on linux and windows as well in bug 984628. > > > > I think it might be simpler to just fix the socialapi use of the disabled > > attribute, but also like this css change better. > > That is strange, because this was meant to be fixed by bug 973704. If bug > 980374 regressed OS X, fine, but what regressed Windows/Linux? :| I have another fix coming for bug 984630 and bug 984628 that addresses this.
Attached patch osx padding fix (obsolete) (deleted) — — Splinter Review
This fix should make it easier to keep the buttons consistent.
Assignee: mdeboer → mixedpuppy
Attachment #8392811 - Attachment is obsolete: true
Attachment #8393721 - Flags: review?(mdeboer)
Comment on attachment 8393721 [details] [diff] [review] osx padding fix @Gijs can r? in place of mdeboer (who I think may be laptopless)
Attachment #8393721 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8393721 [details] [diff] [review] osx padding fix Review of attachment 8393721 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/browser.css @@ +1234,5 @@ > min-width: 28px; > } > > /* Help 16px icons fit: */ > +.toolbarbutton-1[cui-areatype="toolbar"]:not(:-moz-any(@primaryToolbarButtons@)) > .toolbarbutton-badge-container > .toolbarbutton-icon, Are these 16px icons? I mean, if so, r+, but the comment should continue applying, otherwise it'd make sense to keep this as a separate rule.
Attachment #8393721 - Flags: review?(mdeboer)
Attachment #8393721 - Flags: review?(gijskruitbosch+bugs)
Attachment #8393721 - Flags: review+
(In reply to :Gijs Kruitbosch from comment #19) > Comment on attachment 8393721 [details] [diff] [review] > Are these 16px icons? I mean, if so, r+, but the comment should continue > applying, otherwise it'd make sense to keep this as a separate rule. Well, they are typically 16px but we don't necessarily control that. Pocket is using 32px. However, while looking into this, I discovered I don't need any rule specific to the button here, simply removing the old css fixes the padding issue. New patch coming.
Attached patch osx padding fix (deleted) — — Splinter Review
Attachment #8393809 - Flags: review?(gijskruitbosch+bugs)
Attachment #8393809 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c8809e6c34ab
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8393721 - Attachment is obsolete: true
Comment on attachment 8393809 [details] [diff] [review] osx padding fix [Approval Request Comment] Bug caused by (feature/regressing bug #): australis styling User impact if declined: minor visual glitches when social providers are installed Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #8393809 - Flags: approval-mozilla-beta?
Attachment #8393809 - Flags: approval-mozilla-aurora?
Attachment #8393809 - Flags: approval-mozilla-beta?
Attachment #8393809 - Flags: approval-mozilla-beta+
Attachment #8393809 - Flags: approval-mozilla-aurora?
Attachment #8393809 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed using Firefox 29 beta 3, latest Aurora and latest Nightly on Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: