Closed Bug 635675 Opened 13 years ago Closed 13 years ago

OS X Add-ons Manager polish

Categories

(Toolkit :: Themes, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: polish)

Attachments

(7 files, 1 obsolete file)

Attached image screenshot with patch
In this bug I'll attach a few Mac OS X-only polish patches for the Add-ons manager. Among other things they'll also fix the Mac part of bug 623199.

Boriss, I'm requesting ui-review on this screenshot as a proxy for all the patches here. In a few hours you'll be able to test the changes in this tryserver build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-d7863897bd57
/tryserver-macosx64/firefox-4.0b12pre.en-US.mac.dmg

I'll describe the exact changes I'm making when I attach the patches.

(If somebody can't wait, here's an older, slightly outdated, build: 
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mstange@themasta.com-2980e7b2b980/tryserver-macosx64/firefox-4.0b12pre.en-US.mac.dmg - it's only missing the updated dropdown arrow on the wrench button next to the searchfield and it's based on a two-week old mozilla-central)
Attachment #513953 - Flags: ui-review?(jboriss)
This replaces all light outer box shadows and text shadows with @loweredShadow@, i.e. 40% white, 1px down, no blur. Getting rid of the blurring results in noticeably better scrolling performance and is more consistent with OS X.
Before this patch, some shadows even had horizontal offsets which is completely wrong because the light comes straight from the top on OS X.
Attachment #513956 - Flags: review?(dao)
This fixes the header buttons and is the Mac part of bug 623199. It also moves the wrench icon 1px higher to match the mockups.
Attachment #513957 - Flags: review?(dao)
Attached patch part 3, v1: search field (obsolete) — Splinter Review
The search field. I had to make a change to the test due to the new border radius.
Attachment #513958 - Flags: review?(dao)
This fixes the small buttons in the list view. We get rid of the hover effect, make the buttons smaller, tweak the colors and add a text shadow.

I didn't remove the min-width completely because that led to shifting when disabling and re-enabling restartless add-ons: The text changes between "Disable" and "Enable" which would affect the button width. Keeping a minimum width prevents this, at least in English builds.
Attachment #513960 - Flags: review?(dao)
This puts a correct dropmarker image on the button with the wrench.

This is the last patch in my series for this bug.
Attachment #513961 - Flags: review?(dao)
Attachment #513956 - Flags: review?(dao) → review+
Comment on attachment 513957 [details] [diff] [review]
part 2, v1: polish header buttons

You can probably use [disabled] and [open] without ="true"
Attachment #513957 - Flags: review?(dao) → review+
Comment on attachment 513958 [details] [diff] [review]
part 3, v1: search field

>+@namespace html url("http://www.w3.org/1999/xhtml");

This only really makes sense if you set the default namespace.
Attachment #513960 - Flags: review?(dao) → review+
(In reply to comment #7)
> Comment on attachment 513958 [details] [diff] [review]
> part 3, v1: search field
> 
> >+@namespace html url("http://www.w3.org/1999/xhtml");
> 
> This only really makes sense if you set the default namespace.

Interesting! What would make more sense, adding the line
@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
or just leaving out the html|* in the .textbox-input selector?
(In reply to comment #6)
> Comment on attachment 513957 [details] [diff] [review]
> part 2, v1: polish header buttons
> 
> You can probably use [disabled] and [open] without ="true"

I'll make that change for [open], but http://hg.mozilla.org/mozilla-central/annotate/b5a403b079d3/toolkit/mozapps/extensions/content/extensions.js#l1109 sets disabled="false".
> Interesting! What would make more sense, adding the line
> @namespace
> url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> or just leaving out the html|* in the .textbox-input selector?

You should set the default namespace, as this is cleaner for tag name matches.
Attachment #513958 - Attachment is obsolete: true
Attachment #513958 - Flags: review?(dao)
Attachment #513987 - Flags: review?(dao)
Attachment #513961 - Flags: review?(dao) → review+
Comment on attachment 513987 [details] [diff] [review]
part 3, v2: search field

>+#header-search[focused="true"] {

[focused]
Attachment #513987 - Flags: review?(dao) → review+
Attachment #513956 - Flags: approval2.0?
Attachment #513957 - Flags: approval2.0?
Attachment #513960 - Flags: approval2.0?
Attachment #513961 - Flags: approval2.0?
Attachment #513987 - Flags: approval2.0?
Thanks for the fast reviews, Dão!
Boriss: do you agree with these changes? Your ui-r is notably missing.
Comment on attachment 513956 [details] [diff] [review]
part 1, v1: consistent unblurred shadows

removing approval requests until Boriss' weigh-in
Attachment #513956 - Flags: approval2.0?
Comment on attachment 513953 [details]
screenshot with patch

This look fantastic, Markus, and is a hugely important step to final polish.  The widgets themselves look great on display and feel awesome on click.  

There's just one other change that could make this even better, if it's possible - lining up the widgets in the add-on bar with those of Firefox in general.

The navigation widgets in the add-on manager, both in the nightlies and in your patch, are currently slightly lower than the navigation widgets in Firefox. 
While this placement looks fine within the add-ons manager, when switching between tabs it gives the widgets the appearance of jumping slightly down and back up.

The add-on manager's widgets should be moved up so that they align completely
with Firefox's widgets.  If the user is using regular rather than small icons,
the back button will be slightly larger (and more round) than in the manager.  This isn't such a big problem - if the forward arrow aligns it should not be too jarring and, more importantly, shouldn't require the user to move their cursor when pressing back multiple times.
Attachment #513953 - Flags: ui-review?(jboriss) → ui-review-
(In reply to comment #16)
> There's just one other change that could make this even better, if it's
> possible - lining up the widgets in the add-on bar with those of Firefox in
> general.

This seems only tangentially related to this bug's changes. Really shouldn't prevent this patch from landing.
(In reply to comment #18)
> (In reply to comment #16)
> > There's just one other change that could make this even better, if it's
> > possible - lining up the widgets in the add-on bar with those of Firefox in
> > general.
> 
> This seems only tangentially related to this bug's changes. Really shouldn't
> prevent this patch from landing.

I'm happy to give this r+ and address positioning in another bug.  Just thought it might save time to combine them. How do you feel, Markus?
It won't save any time to combine them. I'd rather make that change in a separate bug (bug 630682 apparently).
Comment on attachment 513953 [details]
screenshot with patch

We're going to address positioning of widgets in another bug, so I'm +'ing this for the styling fixes.  Great work, Markus!
Attachment #513953 - Flags: ui-review- → ui-review+
Attachment #513956 - Flags: approval2.0?
Attachment #513957 - Flags: approval2.0?
Attachment #513960 - Flags: approval2.0?
Attachment #513961 - Flags: approval2.0?
Attachment #513987 - Flags: approval2.0?
Comment on attachment 513956 [details] [diff] [review]
part 1, v1: consistent unblurred shadows

a=beltzner
Attachment #513956 - Flags: approval2.0? → approval2.0+
Attachment #513957 - Flags: approval2.0? → approval2.0+
Attachment #513960 - Flags: approval2.0? → approval2.0+
Attachment #513961 - Flags: approval2.0? → approval2.0+
Attachment #513987 - Flags: approval2.0? → approval2.0+
Keywords: polish
A couple parts needed a small unbitrot, I also addressed comment 12.
While the unbitrot was nothing major (4 lines), please check that recent changes that were made don't conflict with any of new changes.

http://hg.mozilla.org/mozilla-central/rev/d35818ad449b
http://hg.mozilla.org/mozilla-central/rev/21ebc5dc59e0
http://hg.mozilla.org/mozilla-central/rev/3bf54ae3415f
http://hg.mozilla.org/mozilla-central/rev/6eb09fa2d543
http://hg.mozilla.org/mozilla-central/rev/9de437da1546
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Target Milestone: mozilla2.0b12 → mozilla2.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: