Remove etch from the Keyhole for Firelight

VERIFIED FIXED in Firefox 3.6a1

Status

()

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: faaborg, Assigned: dao)

Tracking

({verified1.9.1})

Trunk
Firefox 3.6a1
All
Mac OS X
verified1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [icon-shiretoko][icon-complete])

Attachments

(3 attachments, 14 obsolete attachments)

33.97 KB, image/png
Details
34.51 KB, image/png
Details
98.07 KB, patch
mstange
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Currently one of the most common complaints about our OS X theme is the etch we created to try to group the back and forward controls to the drop down arrow.  It makes the controls look a little more complicated, and this type of navigation drop down isn't found elsewhere in the OS.  Also, the etch seems to provide a visual affordance that you can slide the forward button left and right, which wasn't helped by the release of the iPhone (where you actually have this kind of control).

The cleanest way to solve this problem is to completely remove both the etch and the drop down.  Overall I think this is the best idea, and (if I remember correctly) beltzner agrees with removing the control on OS X since this is more platform native.  Beltzner: can you comment to make sure this is the case.

Alternatively I guess we could make the etch much lighter, and less of a hard line and more of a gradual push back into the surface.  We can modify the bug to go with this solution if beltzner doesn't want the drop down control removed.

Regardless of if we keep the etch or not, I think we should remove the left side, since the curve intersects the back button a little strangely.  We are planning on removing that side of the etch on windows as well.
(Reporter)

Comment 1

10 years ago
comments from beltzner sent in email:

>I'm more ok with it on OSX than w32, yes, though I remain hesitant. I think 
>we need to present reasons for doing so, as we're basically trading pixel 
>space for a far less discoverable ui (the only other OSX app that behaves 
>that way is Safari) and the question of why that bit as opposed to just 
>making the keyhole smaller will definitely come up. It's probably ok, 
>though.
(Reporter)

Comment 2

10 years ago
My reply:

>the question of why that bit as opposed to just
>making the keyhole smaller will definitely come up.

It isn't about reducing size but visual complexity, there is a whole lot going on with the etch and the drop down right now.

Arguments in favor of the change:

I think users will be able to discover right click (if they are windows people) or click and hold, if they really want to find if the functionality is still there.  If someone just switched to a Mac the absence of a drop down arrow in Firefox isn't going to be the most jarring part of the overall transition to the new OS.

I think we should drop the control for OS X based on the "when in rome" argument, although traditionally we have tried hard to have a consistent interactive design across platforms.
I'll be happy to take this once we get a new image to drop in.  I'll also have to adjust the CSS of the other images that come after the back/forward buttons if the width ends up changing.  Knowing what the dimensions of each region is a big help there (I have no good tools to determine that information).
(Reporter)

Comment 4

10 years ago
Stephen has the source material so he will need to attach the new toolbar.png
Created attachment 348763 [details]
Toolbar.png without etch

Here is a new Toolbar.png without the etch.
I'll also need a new rtl image for this.
Assignee: nobody → sdwilsh

Updated

10 years ago
Summary: Remove etch and dropdown from the Keyhole for Fireflight → Remove etch and dropdown from the Keyhole for Firelight
Created attachment 348877 [details] [diff] [review]
v0.1

Need the rtl image to finish this bug, but this is the ltr stuff.
Created attachment 348896 [details]
Toolbar-rtl.png without etch.

Here is the RTL version. Sorry about that.
(Reporter)

Comment 9

10 years ago
Follow up bug 465667 with several alignment tweaks.
Created attachment 349018 [details] [diff] [review]
v1.0

I think this get's rtl.  I ended up changing the chromedir manually on the buttons and switched their order.  Nobody was around on irc that could tell me how to actually change the entire UI to use rtl for the whole UI to make sure it looks right.

Once I find that out, I'll know for sure.
Attachment #348877 - Attachment is obsolete: true
Attachment #349018 - Flags: ui-review?(beltzner)
Attachment #349018 - Flags: review?(gavin.sharp)

Updated

10 years ago
Status: NEW → ASSIGNED
Created attachment 349028 [details]
screenshot (ltr)
Attachment #349028 - Flags: ui-review?(beltzner)
Created attachment 349029 [details]
screenshot (rtl)
Attachment #349029 - Flags: ui-review?(beltzner)

Updated

10 years ago
Blocks: 465667
Comment on attachment 349018 [details] [diff] [review]
v1.0

forgot about small icons in rtl...
Attachment #349018 - Attachment is obsolete: true
Attachment #349018 - Flags: ui-review?(beltzner)
Attachment #349018 - Flags: review?(gavin.sharp)
Created attachment 349052 [details]
screenshot small icons (ltr)
Attachment #349052 - Flags: ui-review?(beltzner)
Created attachment 349054 [details]
screenshot small icons (rtl)
Attachment #349054 - Flags: ui-review?(beltzner)
Created attachment 349055 [details] [diff] [review]
v1.1
Attachment #349055 - Flags: review?(gavin.sharp)

Updated

10 years ago
Attachment #349055 - Flags: review?(gavin.sharp) → review?(dao)
Created attachment 349094 [details] [diff] [review]
v1.2

minor change so I don't have to change one of the selector rules that is part of the default buttons (possible perf hit).
Attachment #349055 - Attachment is obsolete: true
Attachment #349094 - Flags: review?(dao)
Attachment #349055 - Flags: review?(dao)
(Assignee)

Updated

10 years ago
Attachment #349094 - Flags: review?(dao) → review+
(Assignee)

Comment 19

10 years ago
Comment on attachment 349094 [details] [diff] [review]
v1.2

> toolbar[mode="icons"] #unified-back-forward-button > #forward-button[disabled="true"] {
>- -moz-image-region: rect(33px, 566px, 66px, 539px);
>+ -moz-image-region: rect(33px, 561px, 66px, 536px);
> }
> 
> toolbar[mode="icons"] #unified-back-forward-button > #forward-button[disabled="true"][chromedir="rtl"] {
>- -moz-image-region: rect(33px, 547px, 66px, 519px);
>+ -moz-image-region: rect(33px, 529px, 66px, 504px);

While you're at it, you could fix the indention...
(In reply to comment #19)
> While you're at it, you could fix the indention...
Fixed in a few other places that I'm touching, and I added vim and emacs modelines to the file.

Updated

10 years ago
Whiteboard: [has patch][needs ui-r beltzner]
(Reporter)

Comment 21

10 years ago
Since this bug is pending ui-r I wanted to jump in and provide some quick design rationale.  There are two design attributes in contention here: creating a discoverable and easy to use interface, vs. creating an interface that is visually clean.

So I believe that it isn't the case that one approach is right and the other wrong, it's just a choice of how we want to balance discoverability of features with visual design.  Also, I don't think that user's necessarily value discoverability so much more than visual design that this is a major mistake, or the other way around.  Both solutions are balanced tradeoffs.

However, I think by not being willing to adjust this balance to the balance established by each platform, we end up being a foreign application (even if we got all the colors and gradients perfect).  Asserting that a particular balance between discoverabilty and visual design is universally correct for all platforms seems to disregard that platforms have expected norms, and users of those platforms have established values.  In particular I believe that one of the things preventing us from taking more market share away from Safari on OS X is the perception that Firefox is "easy to use, but messy" when users actually want "harder to use, but clean."  They aren't arguing against the fact that seeing the drop down makes it easy to discover, they are asserting that you only have to discover the interaction once, but you have to actually look at the thing thousands of times, and they expect us to take a different balance.
Alex - thanks for the context. Based on your rationale, taking this bug independent of other bugs that work to overhaul the look of default chrome might leave us in a neither-this-or-that state in terms of choosing discoverability over visual cleanliness. Is that right?

Two additional questions come to mind:

 - is this the first time we would present different default chrome based on the operating system? aside from icons, we've been trying to create a XPP Firefox look using shape, haven't we? wouldn't this change that?

 - click-and-hold on the back button would now potentially show entries that would take the user forward; if we implement this change, should we not ensure that the lists are separated out again?
(In reply to comment #22)
>  - is this the first time we would present different default chrome based on
> the operating system? aside from icons, we've been trying to create a XPP
> Firefox look using shape, haven't we? wouldn't this change that?
We still have the keyhole design, it just get's rid of the non-osxy groove (for lack of a better word) that they sit in.  Based on shape, this changes nothing.

>  - click-and-hold on the back button would now potentially show entries that
> would take the user forward; if we implement this change, should we not ensure
> that the lists are separated out again?
We already do this now, so I'm not really sure how this is an argument against taking this...
(Reporter)

Comment 24

10 years ago
>Alex - thanks for the context. Based on your rationale, taking this bug
>independent of other bugs that work to overhaul the look of default chrome
>might leave us in a neither-this-or-that state in terms of choosing
>discoverability over visual cleanliness. Is that right?

There are a range of options from easier to use to visual cleanliness:

-current UI [disoverablity]
-much softer etch
-much softer etch,just one side
-drop down floating in space next to the keyhole
-no drop down [visual cleanliness]

> - is this the first time we would present different default chrome based on
>the operating system? 

The current changes between all platforms are of course things like action button placement on dialogs, "Preferences" vs. "Options," and using native print and open dialogs.  But I think as precedent I would probably cite not using the keyhole at all on Linux.  Just as GTK icons were the native choice for that platform, trying to push the bounds of visual cleanliness is the native choice for OS X.

>aside from icons, we've been trying to create a XPP
>Firefox look using shape, haven't we? wouldn't this change that?

They keyhole is iconic enough by itself, in fact deemphasizing the etch and drop down probably makes it more iconic.

> - click-and-hold on the back button would now potentially show entries that
>would take the user forward; if we implement this change, should we not ensure
>that the lists are separated out again?

I think ideally we would depress both buttons and show the integrated list when you pressed and held either.  Bringing back the separate lists would solve the general UI problem and would still be better than an integrated list without depressing both buttons.
(In reply to comment #24)
> I think ideally we would depress both buttons and show the integrated list when
> you pressed and held either.  Bringing back the separate lists would solve the
> general UI problem and would still be better than an integrated list without
> depressing both buttons.
We'd need an additional image with both buttons depressed for this, as well as modifying the controls of the buttons (all doable, but perhaps follow-up?).
Someone else can drive this in if ui-r feedback is negative, but I've got to spend time on other stuff.
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
We've decided to keep the drop down arrow so we aren't doing an interactive chance, but remove the etch to streamline the appearance.  I'll post the new toolbar.png files.
Whiteboard: [has patch][needs ui-r beltzner] → [has patch][needs ui-r beltzner][icon-shiretoko][icon-complete]
(Reporter)

Updated

9 years ago
Summary: Remove etch and dropdown from the Keyhole for Firelight → Remove etch from the Keyhole for Firelight
Created attachment 379516 [details]
Toolbar without keyhole etch (but with drop down)
Attachment #348763 - Attachment is obsolete: true
Created attachment 379518 [details]
Toolbar-rtl without keyhole etch (but with drop down)
Attachment #348896 - Attachment is obsolete: true
If anyone is able to pick this up very quickly so that we can get it into 3.5, please do.  Also note a few alignment tweaks from the current patch detailed in bug 465667.
Uh... are the color profiles attacking again? The new images look darker than the current Toolbar.png. And the current Toolbar-rtl.png is too dark, too...
Created attachment 379524 [details] [diff] [review]
image region changes

These are the CSS image region changes. My build is still compiling, but I have to leave now, so this is basically untested... I won't be able to work on this for the next 9 hours, so whoever can, please take over.
(Assignee)

Updated

9 years ago
Attachment #349094 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #349054 - Attachment is obsolete: true
Attachment #349054 - Flags: ui-review?(beltzner)
(Assignee)

Updated

9 years ago
Attachment #349052 - Attachment is obsolete: true
Attachment #349052 - Flags: ui-review?(beltzner)
(Assignee)

Updated

9 years ago
Attachment #349029 - Attachment is obsolete: true
Attachment #349029 - Flags: ui-review?(beltzner)
(Assignee)

Updated

9 years ago
Attachment #349028 - Attachment is obsolete: true
Attachment #349028 - Flags: ui-review?(beltzner)
(Assignee)

Updated

9 years ago
Assignee: nobody → dao
Whiteboard: [has patch][needs ui-r beltzner][icon-shiretoko][icon-complete] → [icon-shiretoko][icon-complete]
(Assignee)

Comment 33

9 years ago
(In reply to comment #31)
> Uh... are the color profiles attacking again? The new images look darker than
> the current Toolbar.png. And the current Toolbar-rtl.png is too dark, too...

Indeed. The gAMA chunk is missing, used to be 0.55000.
(Assignee)

Comment 34

9 years ago
Created attachment 379534 [details]
Toolbar.png (gAMA added)
Attachment #379516 - Attachment is obsolete: true
(Assignee)

Comment 35

9 years ago
Created attachment 379535 [details]
Toolbar-rtl.png (gAMA added)

The old Toolbar-rtl.png didn't have a gAMA chunk, but it needs one to be consistent with Toolbar.png.
Attachment #379518 - Attachment is obsolete: true
(Assignee)

Comment 36

9 years ago
Created attachment 379536 [details] [diff] [review]
patch
Attachment #379524 - Attachment is obsolete: true
Attachment #379536 - Flags: review?(gavin.sharp)
(Assignee)

Comment 37

9 years ago
Created attachment 379537 [details] [diff] [review]
patch v2

forgot about the click-and-hold behavior on OS X...
Attachment #379536 - Attachment is obsolete: true
Attachment #379537 - Flags: review?(gavin.sharp)
Attachment #379536 - Flags: review?(gavin.sharp)
Comment on attachment 379537 [details] [diff] [review]
patch v2

Looks like you found all my mistakes.
Attachment #379537 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #379537 - Flags: review?(gavin.sharp)
(Assignee)

Comment 39

9 years ago
http://hg.mozilla.org/mozilla-central/rev/32029d575146
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
(Assignee)

Updated

9 years ago
Attachment #379537 - Flags: approval1.9.1?
Comment on attachment 379537 [details] [diff] [review]
patch v2

a191=beltzner; this change is going to shock a bunch of nightly users, so you might want to make sure that there's a blog entry that goes along with it so we don't get a lot of "OMG BUG!" reports.
>this change is going to shock a bunch of nightly users, so you
>might want to make sure that there's a blog entry that goes along with it so we
>don't get a lot of "OMG BUG!" reports.

sure thing, I'll watch this bug to get the post up before the nightly it lands in.

Updated

9 years ago
Depends on: 494832
This breaks Personas, at least, pretty badly:

http://www.grabup.com/uploads/cb3a86731ec9571b3bcdf780c85314dd.png

Which makes me suspect that it may also affect other themes.

It's a little-to-a-lot late to be hitting theme compatibility. Did we give the Personas guys a heads-up? If we suspect that this will affect compatibility in the slightest, then we need to back it out and wait until next time.

Re-opening and blocking until those questions are answered.
Status: RESOLVED → REOPENED
Flags: blocking-firefox3.5?
Keywords: fixed1.9.1
Resolution: FIXED → ---
(Assignee)

Comment 44

9 years ago
Nope, this can't possibly affect other themes. But personas is an extension that hacks the theme, so yes, it's no surprise that personas is affected. Should be pretty easy for them to update their custom keyhole images, though.
Blocks: 494925
(Assignee)

Comment 45

9 years ago
... or if it's somehow hard or undesirable to update the image, they could keep the etch and explicitly use the old coordinates for their old Toolbar.png.
No longer blocks: 494925
On OSX with today's (20090926) shiretoko build and the removal of the etch overnight

http://i43.tinypic.com/2psn53r.png

it reverts back with a personas theme as in comment #43 to look like 
http://www.grabup.com/uploads/cb3a86731ec9571b3bcdf780c85314dd.png
^20090526
As Dão notes, Personas is particularly aggressive with toolbarbuttons on Mac OS X, providing its own versions of all the standard buttons in order to set their transparency and antialiasing appropriately so they look good when a persona is selected.

Thus the regression here is likely to be fairly specific to Personas rather than a general regression that affects many extensions, and the onus should probably be on Personas to update its custom toolbarbutton images to reflect the change in the default Firefox theme.

I have filed bug 494925 on fixing this problem in Personas.  I think no additional work is needed in this bug, and it can be resolved fixed again.

(FWIW, those transparency/antialiasing fixes should probably get pushed into core so Personas doesn't have to ship its own versions of the icons, but that's a separate bug; filed that one as bug 494927).

Dão: what's the problem with this bug blocking bug 494925?  That bug does depend on this one, since it makes no sense to change the Personas images if the Firefox images don't change.
(Assignee)

Comment 49

9 years ago
(In reply to comment #48)
> Dão: what's the problem with this bug blocking bug 494925?

Mid-air? I haven't touched the dependency fields intentionally.
(Assignee)

Updated

9 years ago
Blocks: 494925
Based on Comment 48 resolving fixed.
(In reply to comment #49)
> Mid-air? I haven't touched the dependency fields intentionally.

Ah, sorry, that must've been it!


(In reply to comment #50)
> Based on Comment 48 resolving fixed.

Erm, this is still open; resolving.

Note: I've pushed the fix for Personas, so you can confirm that Firefox works fine with the fixed version of Personas by testing this Personas development build:

https://people.mozilla.com/~cbeard/personas/dist/personas-dev.xpi
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.1
Verified fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214
Status: RESOLVED → VERIFIED
Flags: blocking-firefox3.5?
Keywords: fixed1.9.1 → verified1.9.1
Hardware: x86 → All
Comment on attachment 379537 [details] [diff] [review]
patch v2

a191=beltzner post-facto and post-fixo
Attachment #379537 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 54

9 years ago
Approval was already given in comment 40, fwiw.
You need to log in before you can comment on or make changes to this bug.