Closed Bug 856665 Opened 7 years ago Closed 6 years ago

Australis toolbar buttons for OS X

Categories

(Firefox :: Theme, enhancement)

All
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jsbruner, Assigned: mconley)

References

(Depends on 1 open bug, )

Details

(Keywords: ux-minimalism, Whiteboard: [Australis:M7])

Attachments

(2 files, 22 obsolete files)

688.42 KB, image/png
Details
128.30 KB, patch
mconley
: review+
Details | Diff | Splinter Review
As seen from Stephen Horlander's concepts, we should streamline the toolbar icons. Patch coming in a few minutes...
Attached image Streamlined window
Screenshot showing off the change.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Attached patch The Change. (obsolete) — Splinter Review
Small change, big difference.
Comment on attachment 731924 [details] [diff] [review]
The Change.

Asking for ui-review from Stephen.
Attachment #731924 - Flags: ui-review?(shorlander)
Thanks for picking this up. I thought there was an existing bug on this but I can't find one.

Does this patch darken the icon on button press as seen in the spec?
Summary: Streamline toolbar icons for Australis → Australis toolbar buttons for OS X
(In reply to Matthew N. [:MattN] from comment #4)
> Thanks for picking this up. I thought there was an existing bug on this but
> I can't find one.
> 
> Does this patch darken the icon on button press as seen in the spec?

Not currently, but I can definitely do that tomorrow.
Next step after main toolbar would be to apply this new style to the whole UI (e.g. Bookmarks Toolbar). I've filed bug 767319 some time ago for all platforms.
Attached patch The Change. (obsolete) — Splinter Review
Updated patch which first lightens up the buttons as seen from Stephen's mockup, and then darkens on press.

Carrying ui-review? flag and requesting review from Dão.

I'm not sure if the new button arrows are part of this or not, if they are I will need whatever image is needed at least at retina size.
Attachment #731924 - Attachment is obsolete: true
Attachment #731924 - Flags: ui-review?(shorlander)
Attachment #732309 - Flags: ui-review?(shorlander)
Attachment #732309 - Flags: review?(dao)
Attached patch The Change. (obsolete) — Splinter Review
Updated patch. This adds the new style for the toolbar icons. Carrying review flags.
Attachment #732309 - Attachment is obsolete: true
Attachment #732309 - Flags: ui-review?(shorlander)
Attachment #732309 - Flags: review?(dao)
Attachment #734725 - Flags: ui-review?(shorlander)
Attachment #734725 - Flags: review?(dao)
Blocks: 859776
Attached patch The Change. (obsolete) — Splinter Review
Updated patch. Does a few things.

A. Does not change the opacity if the buttons are disabled (as they should be)

B. Buttons are now streamlined even when themes are applied (Back/Forward button/s excluded). However, right now, OS X does not have an inverted toolbar, so on darker themes the buttons become hard to see. I'm assuming we want these new buttons even with custom themes, but to keep from them being broken, another bug should be filed to allow an inverted toolbar.

Is this the right idea for Australis Stephen, should the buttons be changed even with themes?
Attachment #734725 - Attachment is obsolete: true
Attachment #734725 - Flags: ui-review?(shorlander)
Attachment #734725 - Flags: review?(dao)
Attachment #735494 - Flags: ui-review?(shorlander)
Attachment #735494 - Flags: review?(dao)
Attached patch The Change. (obsolete) — Splinter Review
Geez. Sorry for the spam guys.

Updated patch fixes oversight where the back/forward buttons was getting faded like all other buttons.
Attachment #735494 - Attachment is obsolete: true
Attachment #735494 - Flags: ui-review?(shorlander)
Attachment #735494 - Flags: review?(dao)
Attachment #736384 - Flags: ui-review?(shorlander)
Attachment #736384 - Flags: review?(dao)
Comment on attachment 736384 [details] [diff] [review]
The Change.

Review of attachment 736384 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few things:

- The icons should be full opacity not .8
- From Toolbar-lion.png the first row should be the default icon and the second row is the pressed state
- For toggled items like Bookmarks and History sidebars the toggled state is the third row in Toolbar-lion.png and they don't need a background image

Thanks! :)
Attachment #736384 - Flags: ui-review?(shorlander) → ui-review-
Comment on attachment 736384 [details] [diff] [review]
The Change.

cancelling review request due to ui-review-
Attachment #736384 - Flags: review?(dao)
Attached patch The Change. V2 (obsolete) — Splinter Review
Updated patch addresses ui-review feedback. This also moves the "panel menu" icon into the toolbar.png with all three states. However, the toggled state is actually not implemented in this patch. It is going to take a little more work then I thought at first, so we can just deal with that in another bug.

I still need to do some code cleanup, but am going to upload this and ask for ui-review in the meantime.
Attachment #736384 - Attachment is obsolete: true
Attachment #742456 - Flags: ui-review?(shorlander)
Attached patch The Change. V2.1 (obsolete) — Splinter Review
Updated patch. Now ready for review. Carrying ui-review flag. Adding review flag.
Attachment #742456 - Attachment is obsolete: true
Attachment #742456 - Flags: ui-review?(shorlander)
Attachment #743040 - Flags: ui-review?(shorlander)
Attachment #743040 - Flags: review?(dao)
Comment on attachment 743040 [details] [diff] [review]
The Change. V2.1

Looks good! The only thing I noticed is that the Sidebar toggle state isn't using the blue icon. ui-r+ with that.
Attachment #743040 - Flags: ui-review?(shorlander) → ui-review+
Comment on attachment 743040 [details] [diff] [review]
The Change. V2.1

> #back-button,
> #forward-button:-moz-locale-dir(rtl),
> toolbar[mode="icons"] #back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>   -moz-image-region: rect(0, 40px, 20px, 20px);
>+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(50,50,50,0.2) 50%, rgba(0,0,0,0.2) 50%, rgba(0,0,0,0.13));
> }

Do back and forward form the keyhole regardless of where they're placed? On Windows, we only do this when they're next to the location bar, which seems more consistent. If they're not connected with the location bar, it's seems dubious that they have a button shape in contrast to all other toolbar buttons.

>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button:active {
>+  -moz-appearance: none;
>+  -moz-margin-end: -7px;
>+  position: relative;
>+  z-index: 1;
>+  -moz-image-region: rect(20px, 20px, 40px, 0);
>+  width: 30px;
>+  height: 30px;
>+  padding: 4px 5px 4px 3px;
>+  border-radius: 10000px;
>+}

Only -moz-image-region should be needed here, the rest seems redundant.

>--- a/browser/themes/shared/panelUIOverlay.inc.css
>+++ b/browser/themes/shared/panelUIOverlay.inc.css
>@@ -1,12 +1,13 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+@import url("chrome://global/skin/");

Why is this needed?
Attachment #743040 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 743040 [details] [diff] [review]
> The Change. V2.1
> 
> > #back-button,
> > #forward-button:-moz-locale-dir(rtl),
> > toolbar[mode="icons"] #back-button:-moz-locale-dir(rtl):-moz-lwtheme {
> >   -moz-image-region: rect(0, 40px, 20px, 20px);
> >+  background-image: linear-gradient(rgba(255,255,255,0.3), rgba(50,50,50,0.2) 50%, rgba(0,0,0,0.2) 50%, rgba(0,0,0,0.13));
> > }
> 
> Do back and forward form the keyhole regardless of where they're placed? On
> Windows, we only do this when they're next to the location bar, which seems
> more consistent. If they're not connected with the location bar, it's seems
> dubious that they have a button shape in contrast to all other toolbar
> buttons.

Yes, on OS X, even when the buttons are not connected to the location bar the shape works the same. The only thing that changes at all is the fact that the back button will never hide.

> 
> >+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button:active {
> >+  -moz-appearance: none;
> >+  -moz-margin-end: -7px;
> >+  position: relative;
> >+  z-index: 1;
> >+  -moz-image-region: rect(20px, 20px, 40px, 0);
> >+  width: 30px;
> >+  height: 30px;
> >+  padding: 4px 5px 4px 3px;
> >+  border-radius: 10000px;
> >+}
> 
> Only -moz-image-region should be needed here, the rest seems redundant.
> 
> >--- a/browser/themes/shared/panelUIOverlay.inc.css
> >+++ b/browser/themes/shared/panelUIOverlay.inc.css
> >@@ -1,12 +1,13 @@
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this
> >  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > 
> >+@import url("chrome://global/skin/");
> 
> Why is this needed?

How embarrassing. Must have accidentally qrefreshed in part of another patch. :-/
Attached patch The Change (V 2.2) (obsolete) — Splinter Review
Updated patch addresses feedback. Carrying ui-review+ flag.
Attachment #743040 - Attachment is obsolete: true
Attachment #745572 - Flags: ui-review+
Attachment #745572 - Flags: review?(dao)
(In reply to Josiah Bruner [:JosiahOne] from comment #17)
> Yes, on OS X, even when the buttons are not connected to the location bar
> the shape works the same.

As I said, this doesn't make much sense given the removal of the button shape for all other buttons. Like on Windows, back/forward should only get a button shape when they're connected to the location bar.
Comment on attachment 745572 [details] [diff] [review]
The Change (V 2.2)

See my previous comment. Note that you can probably copy the pattern for targeting back/forward when they're next to the location bar from Windows; should be an easy fix.
Attachment #745572 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Josiah Bruner [:JosiahOne] from comment #17)
> > Yes, on OS X, even when the buttons are not connected to the location bar
> > the shape works the same.
> 
> As I said, this doesn't make much sense given the removal of the button
> shape for all other buttons. Like on Windows, back/forward should only get a
> button shape when they're connected to the location bar.

This should not have happened and is a regression, see bug 738548. The keyhole shape is unique to the Firefox brand and is one of the images that we should maintain.
Either way though, I realized that the forward button's color gradient is wrong, so I will still need to fix that...
Attached file The Change (V 2.4) (obsolete) —
Okay, updated patch that fixes the gradient and a few oversights related to the button being detached. Let's try this review... Again. :)
Attachment #745572 - Attachment is obsolete: true
Attachment #747643 - Flags: ui-review+
Attachment #747643 - Flags: review?(dao)
(In reply to Jared Wein [:jaws] from comment #21)
> > As I said, this doesn't make much sense given the removal of the button
> > shape for all other buttons. Like on Windows, back/forward should only get a
> > button shape when they're connected to the location bar.
> 
> This should not have happened and is a regression, see bug 738548.

It was an intentional measure when we made toolbar buttons borderless on Windows. We already didn't have the keyhole when back/forward were moved to a different toolbar, nor in small icons mode, so it's not like there was a law saying that the keyhole must always be there.

> The keyhole shape is unique to the Firefox brand and is one of the images that
> we should maintain.

We maintain it in the default configuration, but obviously we can't and shouldn't completely prevent the user from customizing it away, e.g. by installing a custom theme as the most extreme means. Toolbar customization affecting the keyhole isn't new either (see above). With floating icons between back/forward and the location bar or with back/forward moved to the end of the navigation toolbar, the keyhole would look increasingly unintended and fail to represent a coherent brand. Falling back on a generic style in states the keyhole wasn't designed for makes a lot of sense.
Dão, regardless I suppose there is really not much point in changing it now. The goal for Australis is that the back/forward buttons will become part of the url bar and therefore the button shape will look as it does now.

It would actually be cleaner to use this button shape so that it is still consistent when the ability to detach buttons is removed.

Since ui-review+ has already been given, I do not believe we need to stop landing simply because of this. :) Is that fair?
(In reply to Josiah Bruner [:JosiahOne] from comment #25)
> The goal for Australis is that the back/forward buttons will become part of
> the url bar and therefore the button shape will look as it does now.

If and when that happens, we can update the rules just like we'll want to update the Windows ones.
Stephen, do you agree that we should change the button style from the current look, to this "shapeless" one, and later back to the current look?

If so then that is what I will do. However it doesn't make much sense to me that we are trying to duplicate a look that we no longer want. Shouldn't we just fix the Window's buttons ASAP and leave OS X as is?
Flags: needinfo?(shorlander)
(In reply to Josiah Bruner [:JosiahOne] from comment #27)
> Shouldn't we just fix the Window's buttons ASAP and leave OS X as is?

Windows isn't broken as I explained in comment 24, so there's nothing to fix right now. AFAIK no final decision has been made that users shouldn't be allowed to separate back/forward from the location bar anymore, so ripping out support from the Windows theme would be premature.
(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to Josiah Bruner [:JosiahOne] from comment #27)
> > Shouldn't we just fix the Window's buttons ASAP and leave OS X as is?
> 
> Windows isn't broken as I explained in comment 24, so there's nothing to fix
> right now. AFAIK no final decision has been made that users shouldn't be
> allowed to separate back/forward from the location bar anymore, so ripping
> out support from the Windows theme would be premature.

I guess we (I), need more information on the plan here. (I'm not saying you are wrong here, but I would like to be sure of this) Responses to this question have varied quite a bit.
11:39 AM <JosiahOne> Now, jaws, shorlander, mconley, other Australis UX people: On OS X, I am about to land the Australis toolbar buttons, however it isn't clear if I should remove the back/forward button shape when they are detached from the location bar so that only the image shows (Like the other toolbar buttons will be). Or, keep the button shape no matter what.
11:39 AM <•shorlander> JosiahOne: Keep the shape even if they are detached
Stephen, could you please respond to the raised question about back/forward not being movable anymore and, if still applicable, comment 24? Thanks.
Attached patch The Change (V 2.5) (obsolete) — Splinter Review
Updated for current trunk. Also fixed a problem with the back button on Retina mode, and am not allowing the back button image to change when it is disabled.

Carrying review? flag.
Attachment #747643 - Attachment is obsolete: true
Attachment #747643 - Flags: review?(dao)
Attachment #749896 - Flags: review?(dao)
Whiteboard: [Australis:M?]
Comment on attachment 749896 [details] [diff] [review]
The Change (V 2.5)

You might be able to convince me that I was on the wrong track in comment 24, but I haven't seen these arguments yet.

To answer comment 26, we just WONTFIXed bug 755598.

Bug 755598 comment 7 points out that the keyhole rendering has issues, so maintenance cost would be another reason to drop the keyhole shape when it's detached from the location bar.
Attachment #749896 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #31)
> Stephen, could you please respond to the raised question about back/forward
> not being movable anymore and, if still applicable, comment 24? Thanks.

The plan is still for a unified back+forward+location+stop/go/reload widget.

Until then if there is a separated configuration we should still keep the keyhole shape. This retains the established visual identity and increased visual prominence of the back button.

The visual appearance inconsistency concern is valid. I would like to explore some additional designs to try and keep a larger back button. I will make a follow-up for that but I don't want it to block this.
Flags: needinfo?(shorlander)
Comment on attachment 749896 [details] [diff] [review]
The Change (V 2.5)

Because of Stephen's information, I am re-flagging for review.
Attachment #749896 - Flags: review- → review?(dao)
Comment on attachment 749896 [details] [diff] [review]
The Change (V 2.5)

> #forward-button,
> #back-button:-moz-locale-dir(rtl),
> #navigator-toolbox[iconsize="large"] > #nav-bar #forward-button:-moz-locale-dir(rtl),
> #forward-button:-moz-locale-dir(rtl):-moz-lwtheme {
>   -moz-image-region: rect(0, 60px, 20px, 40px);
>+  border: 1px solid rgba(0, 0, 0, 0.4);
>+  border-radius: @toolbarbuttonCornerRadius@;
>+  background: linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0.2) 50%, rgba(255,255,255,0.1) 50%, rgba(255,255,255,0.2)) repeat-x;
>+  background-origin: border-box;
>+  background-color: transparent;
>+  box-shadow: inset 0 1px rgba(255,255,255,0.3), 0 1px rgba(255,255,255,0.2);
>+}

Do all the above selectors still make sense?

>+#forward-button:-moz-window-inactive,
>+#back-button:-moz-locale-dir(rtl),
>+#navigator-toolbox[iconsize="large"] > #nav-bar #forward-button:-moz-locale-dir(rtl),
>+#forward-button:-moz-locale-dir(rtl):-moz-lwtheme {
>+  background-color: rgba(0, 0, 0, 0.04);
>+  border-color: rgba(0, 0, 0, 0.2);
>+}

I really don't understand these selectors. Looks like you want to modify the background and border colors for when window is inactive, but you specified :-moz-window-inactive only in the first line.

>+#forward-button:active:not([disabled="true"]),
>+#back-button:-moz-locale-dir(rtl),
>+#navigator-toolbox[iconsize="large"] > #nav-bar #forward-button:-moz-locale-dir(rtl),
>+#forward-button:-moz-locale-dir(rtl):-moz-lwtheme {
>+  -moz-image-region: rect(20px, 60px, 40px, 40px);
>+  text-shadow: @loweredShadow@;
>+  background-color: rgba(0,0,0,0.2);
>+  box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> }

Same here. Looks like you want to style the active state, but you added :active only in the first line.

> /* download manager button */
> 
>-#downloads-button {
>+#downloads-button:not([active]) {
>   -moz-image-region: rect(0, 140px, 20px, 120px);
> }

What's the point of this change? There's no 'active' attribute, is there?
Attachment #749896 - Flags: review?(dao) → review-
Attached patch The Change (V 2.6) (obsolete) — Splinter Review
Addresses review feedback. Resetting review flag. Also fixed an issue with the back button and lw-themes.
Attachment #749896 - Attachment is obsolete: true
Attachment #753290 - Flags: ui-review+
Attachment #753290 - Flags: review?(dao)
Whiteboard: [Australis:M?] → [Australis:M6]
Comment on attachment 753290 [details] [diff] [review]
The Change (V 2.6)

> #back-button,
> #forward-button:-moz-locale-dir(rtl),
> #back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>   -moz-image-region: rect(0, 40px, 20px, 20px);
>+  background-image: linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0.2) 50%, rgba(255,255,255,0.1) 50%, rgba(255,255,255,0.2));
>+}
>+
>+#back-button:-moz-lwtheme,
>+#forward-button:-moz-locale-dir(rtl),
>+#back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>+  background: linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0.2) 50%, rgba(255,255,255,0.1) 50%, rgba(255,255,255,0.2)) repeat-x;

Seems like you can unify some stuff. For instance, the background image is duplicated here and a third time somewhere below.

>+#back-button:-moz-lwtheme:active,
>+#forward-button:-moz-locale-dir(rtl),
>+#back-button:-moz-locale-dir(rtl):-moz-lwtheme:active {
>+  text-shadow: @loweredShadow@;
>+  background-color: rgba(0,0,0,0.2);
>+  box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> }

Did you mean to add :active in the second line?

> #forward-button,
>-#back-button:-moz-locale-dir(rtl),
>-#navigator-toolbox[iconsize="large"] > #nav-bar #forward-button:-moz-locale-dir(rtl),
>+#back-button:-moz-locale-dir(rtl) > #nav-bar #forward-button:-moz-locale-dir(rtl),
> #forward-button:-moz-locale-dir(rtl):-moz-lwtheme {

"#back-button:-moz-locale-dir(rtl) > #nav-bar #forward-button:-moz-locale-dir(rtl)" isn't going to match anything.
Attachment #753290 - Flags: review?(dao) → review-
Attached patch The Change (V 2.6.1) (obsolete) — Splinter Review
Addressed reviewed feedback. Again. (Maybe one day I'll get good at this stuff :) )

Resetting review flag.
Attachment #753290 - Attachment is obsolete: true
Attachment #753348 - Flags: ui-review+
Attachment #753348 - Flags: review?(dao)
Attached patch The Change (V 2.6.1.1) (obsolete) — Splinter Review
... And I totally forgot about the downloads button. Fixed that part of previous feedback as well in this patch. 

Sorry guys.
Attachment #753348 - Attachment is obsolete: true
Attachment #753348 - Flags: review?(dao)
Attachment #753350 - Flags: ui-review+
Attachment #753350 - Flags: review?(dao)
Attached patch The Change (V 2.6.2) (obsolete) — Splinter Review
Updated for the current trunk. Carrying flags.
Attachment #753350 - Attachment is obsolete: true
Attachment #753350 - Flags: review?(dao)
Attachment #754947 - Flags: ui-review+
Attachment #754947 - Flags: review?(dao)
Hey Dao,

Is it possible for you to finish your review on this today? (I leave for vacation tomorrow, and would prefer to have this finished by then if possible)

Thanks!
Comment on attachment 754947 [details] [diff] [review]
The Change (V 2.6.2)

>+#back-button:-moz-lwtheme:active,
>+#forward-button:-moz-locale-dir(rtl),
>+#back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>+  text-shadow: @loweredShadow@;
>+  background-color: rgba(0,0,0,0.2);
>+  box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> }

I'm really confused. Why is the back button's lightweight theme styling for the active state shared with the back button's RTL & lightweight theme styling (regardless of whether it's active) and the forward button's RTL styling (regardless lightweight themes and regardless of whether it's active)? I haven't tested what this does in practice, but just by looking at the code it seems completely random and unintended.

>+#back-button:-moz-locale-dir(rtl) > #nav-bar #forward-button:-moz-locale-dir(rtl):-moz-lwtheme {

Again, this selector is completely bogus. It's not going to match anything.

>+#back-button:-moz-locale-dir(rtl) > #nav-bar #forward-button:-moz-locale-dir(rtl):-moz-window-inactive,

ditto

>+#back-button:-moz-locale-dir(rtl) > #nav-bar #forward-button:-moz-locale-dir(rtl),

ditto
Attachment #754947 - Flags: review?(dao) → review-
Attached patch The Change (V 2.3) (obsolete) — Splinter Review
Thanks Dao!

Wow, yeah. The selectors were terribly confusing (it did not affect the visual outcome, which is why I missed it). Updated patch drastically simplifies the selectors.

Hopefully this is a little more comprehensive. (I also tested everything and things work correctly with lw-themes, rtl, etc)
Attachment #754947 - Attachment is obsolete: true
Attachment #756111 - Flags: ui-review+
Attachment #756111 - Flags: review?(dao)
Comment on attachment 756111 [details] [diff] [review]
The Change (V 2.3)

>+#back-button,
>+#back-button:-moz-locale-dir(rtl) {

What's the point of the second line?

>+#back-button:-moz-lwtheme,
>+#back-button:-moz-locale-dir(rtl):-moz-lwtheme {

and here?

>+#back-button:-moz-lwtheme:active {
>   text-shadow: @loweredShadow@;
>   background-color: rgba(0,0,0,0.2);
>   box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> }

Sometimes you're using :active, sometimes :active:hover. I suspect you want the same thing all the time. Also, you're not checking the 'disabled' attribute here -- you probably should.

>-#back-button,
>-#forward-button:-moz-locale-dir(rtl),
>-#back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>-  -moz-image-region: rect(0, 40px, 20px, 20px);
>-}
>-
> @media (min-resolution: 2dppx) {
>   #back-button,
>-  #forward-button:-moz-locale-dir(rtl),
>-  #back-button:-moz-locale-dir(rtl):-moz-lwtheme {
>+  #forward-button:-moz-locale-dir(rtl):-moz-lwtheme {
>     -moz-image-region: rect(0, 80px, 40px, 40px);
>   }
> }

Shouldn't you be setting the image region for #forward-button:-moz-locale-dir(rtl):-moz-lwtheme (by the way, I'm not sure why this is lightweight-theme-specific) before you override it for 2dppx? Note that the upper rule you removed exactly mirrored the rule in the @media block. This seems more random and fragile now.
Attached patch The Change (V 2.3.1) (obsolete) — Splinter Review
Changes made.

Dao, the reasoning behind #forward-button:-moz-locale-dir(rtl):-moz-lwtheme is quite odd (and semi-interesting). I'll explain.

For some reason, #forward-button:-moz-locale-dir(rtl) {} does not work. At all. That is never matched. However, adding the "-moz-lwtheme" solves this issue. To make things even more intriguing, the "-moz-lwtheme" works even *without* having a lw-theme. It's as if the combination of -moz-locale-dir(rtl) and lw-theme create their own property.

Anyway, that's why for that case I was forced to append the "-moz-lwtheme".
Attachment #756111 - Attachment is obsolete: true
Attachment #756111 - Flags: review?(dao)
Attachment #756169 - Flags: ui-review+
Attachment #756169 - Flags: review?(dao)
Attached patch The Change (V 2.3.2) (obsolete) — Splinter Review
Updated for the current trunk.
Attachment #756169 - Attachment is obsolete: true
Attachment #756169 - Flags: review?(dao)
Attachment #756174 - Flags: ui-review+
Attachment #756174 - Flags: review?(dao)
(In reply to Josiah Bruner [:JosiahOne] from comment #46)
> Created attachment 756169 [details] [diff] [review]
> The Change (V 2.3.1)
> 
> Changes made.
> 
> Dao, the reasoning behind #forward-button:-moz-locale-dir(rtl):-moz-lwtheme
> is quite odd (and semi-interesting). I'll explain.
> 
> For some reason, #forward-button:-moz-locale-dir(rtl) {} does not work. At
> all. That is never matched. However, adding the "-moz-lwtheme" solves this
> issue. To make things even more intriguing, the "-moz-lwtheme" works even
> *without* having a lw-theme. It's as if the combination of
> -moz-locale-dir(rtl) and lw-theme create their own property.
> 
> Anyway, that's why for that case I was forced to append the "-moz-lwtheme".

This doesn't make sense and suggests that something else is messed up in your patch.
(In reply to Dão Gottwald [:dao] from comment #48)
> (In reply to Josiah Bruner [:JosiahOne] from comment #46)
> > Created attachment 756169 [details] [diff] [review]
> > The Change (V 2.3.1)
> > 
> > Changes made.
> > 
> > Dao, the reasoning behind #forward-button:-moz-locale-dir(rtl):-moz-lwtheme
> > is quite odd (and semi-interesting). I'll explain.
> > 
> > For some reason, #forward-button:-moz-locale-dir(rtl) {} does not work. At
> > all. That is never matched. However, adding the "-moz-lwtheme" solves this
> > issue. To make things even more intriguing, the "-moz-lwtheme" works even
> > *without* having a lw-theme. It's as if the combination of
> > -moz-locale-dir(rtl) and lw-theme create their own property.
> > 
> > Anyway, that's why for that case I was forced to append the "-moz-lwtheme".
> 
> This doesn't make sense and suggests that something else is messed up in
> your patch.

That's what I figured before. But I looked over my code many, many times, and can find no logical reason behind this. In fact, looking at other code that I had no part in writing, "-moz-locale-dir(rtl):-mow-lwtheme" is used everywhere for no good reason. This includes the 

> @media (min-resolution: 2dppx) {
>   #back-button,
>-  #forward-button:-moz-locale-dir(rtl),
>-  #back-button:-moz-locale-dir(rtl):-moz-lwtheme {  <-- Why where we using -moz-lwtheme here?
>+  #forward-button:-moz-locale-dir(rtl):-moz-lwtheme {

Unless I have broken CSS entirely, I do not believe I caused this issue. Perhaps exposed it. But definitely did not cause it.
Attached patch The Change (V 2.3.2) (obsolete) — Splinter Review
Ah, so I figured out what the issue was. It was sort of my fault, even though I was right in saying I didn't cause it.

Apparently we have a rule for rtl that uses scaleX(-1) to swap the direction. So in this patch I removed all code that sets an image for either the forward or back button. This rule will solve both issues automatically.

I won't be here for two weeks, so if there are any more minor things that need to be addressed after Dao's review perhaps Stephen or Mike could take it over to make any final edits.
Attachment #756174 - Attachment is obsolete: true
Attachment #756174 - Flags: review?(dao)
Attachment #756298 - Flags: ui-review+
Attachment #756298 - Flags: review?(dao)
Shoot. Accidentally added that to the wrong patch. :/ Fix in a few minutes.
Attached patch The Change (V 2.3.2) (obsolete) — Splinter Review
Here we are.
Attachment #756298 - Attachment is obsolete: true
Attachment #756298 - Flags: review?(dao)
Attachment #756316 - Flags: ui-review+
Attachment #756316 - Flags: review?(dao)
Taking this bug to try and drive it over before Wednesday.

Dao, do you think you could complete a review before then?
Assignee: josiah → mconley
Comment on attachment 756316 [details] [diff] [review]
The Change (V 2.3.2)

>+.toolbarbutton-1:not([type="menu-button"]):not(#fullscreen-button)[checked="true"]:-moz-lwtheme {
>+  background-color: rgba(0,0,0,0.4);
>+  box-shadow: inset 0 2px 5px rgba(0,0,0,0.7), 0 1px rgba(255,255,255,0.2);
>+}
>+
>+.toolbarbutton-1:not([type="menu-button"]):not(#fullscreen-button)[checked="true"]:not([disabled="true"]):active:hover:-moz-lwtheme {
>+  background-color: rgba(0, 0, 0, 0.6);
>+  box-shadow: inset 0 2px 5px rgba(0, 0, 0, 0.8), 0 1px rgba(255, 255, 255, 0.2);
>+}

Is this still needed? Not sure why only the checked state in combination with lightweight themes needs special treatment.

nit: remove spaces from within rgba() values

>+#back-button {
>+  -moz-image-region: rect(0, 40px, 20px, 20px);
>+  background: linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0.2) 50%, rgba(255,255,255,0.1) 50%, rgba(255,255,255,0.2)) repeat-x;
>+  background-color: transparent;
>+}

'background-color: transparent' is redundant

It's unclear to me why only the background is being set here and no other styles such as border, box shadow and border radii, as well as the active state styling. I suppose this stuff is scattered across browser.css. Can it be consolidated?

>+#back-button:-moz-lwtheme {
>+  box-shadow: inset 0 1px rgba(255,255,255,0.3), 0 1px rgba(255,255,255,0.2);
>+  border: 1px solid rgba(0, 0, 0, 0.4);
>+}

nit: remove spaces from within rgba() values

>+#forward-button:-moz-window-inactive,
>+#forward-button:-moz-locale-dir(rtl):-moz-window-inactive {
>+  background-color: rgba(0, 0, 0, 0.04);
>+  border-color: rgba(0, 0, 0, 0.2);
>+}

nit: remove spaces from within rgba() values

I can see no special :-moz-window-inactive styling for the back button in this patch. Is this an intended difference?

>+#downloads-button:active {
>+  -moz-image-region: rect(20px, 140px, 40px, 120px);
>+}

Are you sure you want :active and not :hover:active?

> @media (min-resolution: 2dppx) {
>   #feed-button {
>     -moz-image-region: rect(0, 920px, 40px, 880px);
>   }
>+  
>+  #feed-button {
>+    -moz-image-region: rect(20px, 920px, 80px, 880px);
>+  }
> }

Apparently the pseudo class is missing here.

> #webrtc-status-button {
>   -moz-image-region: rect(0, 480px, 20px, 460px);
> }
> 
>+#webrtc-status-button {
>+  -moz-image-region: rect(20px, 480px, 40px, 460px);
>+}

and here

> @media (min-resolution: 2dppx) {
>   #webrtc-status-button {
>     -moz-image-region: rect(0, 960px, 40px, 920px);
>   }
>+  
>+  #webrtc-status-button {
>+    -moz-image-region: rect(40px, 960px, 80px, 920px);
>+  }
> }

and here
Attachment #756316 - Flags: review?(dao) → review-
Whiteboard: [Australis:M6] → [Australis:M7]
Attached patch WIP Patch 2.3.3 (obsolete) — Splinter Review
Checkpointing some progress here.
Attachment #756316 - Attachment is obsolete: true
Attachment #759570 - Attachment is patch: true
Attachment #759570 - Attachment mime type: text/x-patch → text/plain
Attached patch Patch v2.3.4 (obsolete) — Splinter Review
Attachment #759570 - Attachment is obsolete: true
Comment on attachment 760471 [details] [diff] [review]
Patch v2.3.4

Is this any closer, Dao?
Attachment #760471 - Flags: review?(dao)
Comment on attachment 760471 [details] [diff] [review]
Patch v2.3.4

>+:-moz-any(#back-button, #forward-button) {

Make this two lines, nixing :-moz-any as it reduces these selectors specificity.

>+:-moz-any(#back-button, #forward-button):-moz-lwtheme {

ditto

>+:-moz-any(#back-button, #forward-button):active:hover:-moz-lwtheme {
>   text-shadow: @loweredShadow@;
>   background-color: rgba(0,0,0,0.2);
>   box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> }

ditto

You can drop text-shadow here.

I suppose the styling for the :active:hover state without :-moz-lwtheme is somewhere else in browser.css. Can this be consolidated? Can the selectors be grouped better?

>+#sync-button:active {

:hover:active?

>+  #sync-button:active:not([status="active"]) {

ditto

>+#feed-button:active {

ditto

>+  #feed-button {

ditto
Attachment #760471 - Flags: review?(dao) → review-
Component: Toolbars and Customization → Theme
Also, the above issue put aside, I don't think :-moz-any simplifies things considerably in the cited cases.
Attached patch Patch v2.3.5 (obsolete) — Splinter Review
Attachment #760471 - Attachment is obsolete: true
Comment on attachment 762264 [details] [diff] [review]
Patch v2.3.5

(In reply to Dão Gottwald [:dao] from comment #58)
> Comment on attachment 760471 [details] [diff] [review]
> Patch v2.3.4
> 
> >+:-moz-any(#back-button, #forward-button) {
> 
> Make this two lines, nixing :-moz-any as it reduces these selectors
> specificity.

Ok - I've stopped using :-moz-any for these.

> 
> >+:-moz-any(#back-button, #forward-button):-moz-lwtheme {
> 
> ditto
> 

Done.

> >+:-moz-any(#back-button, #forward-button):active:hover:-moz-lwtheme {
> >   text-shadow: @loweredShadow@;
> >   background-color: rgba(0,0,0,0.2);
> >   box-shadow: inset 0 2px 5px rgba(0,0,0,0.6), 0 1px rgba(255,255,255,0.2);
> > }
> 
> ditto
> 

Done.

> You can drop text-shadow here.
> 

Done.

> I suppose the styling for the :active:hover state without :-moz-lwtheme is
> somewhere else in browser.css. Can this be consolidated? Can the selectors
> be grouped better?
> 

I've reorganized the back and forward button rules a bit, but I think they can be further simplified during or after bug 755598 lands.

> >+#sync-button:active {
> 
> :hover:active?
> 
> >+  #sync-button:active:not([status="active"]) {
> 
> ditto
> 
> >+#feed-button:active {
> 
> ditto
> 
> >+  #feed-button {
> 
> ditto

All fixed.
Attachment #762264 - Flags: review?(dao)
Comment on attachment 762264 [details] [diff] [review]
Patch v2.3.5

> #nav-bar #back-button {
>   -moz-appearance: none;
>   -moz-margin-end: -7px;
>   position: relative;
>   z-index: 1;
>   -moz-image-region: rect(0, 20px, 20px, 0);
>   width: 30px;
>   height: 30px;
>   padding: 4px 5px 4px 3px;
>   border-radius: 10000px;
> }

-moz-appearance: none; isn't needed anymore

>+#navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button:active:not([disabled="true"]) {

>+  #navigator-toolbox[iconsize="large"][mode="icons"] > #nav-bar #back-button:active:not([disabled="true"]) {

remove '#navigator-toolbox[iconsize="large"][mode="icons"] >'
(see bug 573329, bug 863299)

> @conditionalForwardWithUrlbar@ > #forward-button:not(:-moz-lwtheme) {
>   -moz-appearance: none;
>   -moz-padding-start: 2px;
>   background: linear-gradient(hsl(0,0%,99%), hsl(0,0%,67%)) padding-box;
>   border: 1px solid;
>   border-color: hsl(0,0%,31%) hsla(0,0%,29%,.6) hsl(0,0%,27%);
>   box-shadow: inset 0 1px 0 hsla(0,0%,100%,.35),
>               0 1px 0 hsla(0,0%,100%,.2);

-moz-appearance: none; isn't needed here either
Attachment #762264 - Flags: review?(dao) → review+
Awesome, done.

Thanks Josiah for getting this started, and thanks Dao for your excellent reviewing!
Attachment #762264 - Attachment is obsolete: true
Attachment #762285 - Flags: review+
https://hg.mozilla.org/projects/ux/rev/7e60aa0f0ccf
Whiteboard: [Australis:M7] → [Australis:M7][fixed-in-ux]
Just a quick question (this might be the wrong place to ask), but why in UX is the bookmark star now yellow instead of blue?  In the location bar drop down the bookmark star is still blue, so it's inconsistent.  Is this just a temporary thing?
(In reply to Jordan from comment #66)
> Just a quick question (this might be the wrong place to ask), but why in UX
> is the bookmark star now yellow instead of blue?  In the location bar drop
> down the bookmark star is still blue, so it's inconsistent.  Is this just a
> temporary thing?

For toolbar buttons that "open" things (like the menu panel button, or the bookmark button, downloads button, social button, etc), I believe the plan is to indicate their open state by coloring the icon blue. You can see this already by clicking on the menu button. It's not complete for the other buttons just yet.

So this is why the star turns blue when you click on it, and why gold/yellow is used to indicate that the page is bookmarked.
Depends on: 896008
https://hg.mozilla.org/mozilla-central/rev/7e60aa0f0ccf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
Target Milestone: --- → Firefox 28
Depends on: 940705
You need to log in before you can comment on or make changes to this bug.