Closed Bug 924388 Opened 7 years ago Closed 7 years ago

There should be small separators between the sub-edit controls and sub-zoom controls when each controls is placed in a toolbar

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [Australis:P5][Australis:M9])

Attachments

(1 file)

No description provided.
Attached patch PatchSplinter Review
After this patch is landed, we can update the OS X styles to use this class. (or just do that in this patch)
Attachment #814415 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [Australis:P5][Australis:M?]
Comment on attachment 814415 [details] [diff] [review]
Patch

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

::: browser/themes/windows/browser.css
@@ +440,5 @@
>  }
>  
>  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1:not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,
> +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1[disabled]:not(:active):not([open]) + .toolbarbutton-1[disabled]:not(:active):not([open])::before {

Why the separate [disabled] rule?
Comment on attachment 814415 [details] [diff] [review]
Patch

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

r=me, but we should probably make sure that we deal with OS X and Linux as well.
Attachment #814415 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 814415 [details] [diff] [review]
Patch

"toolbaritem-with-separator" suggests that there will be one separator in total, but there can be multiple separators, the exact number being dependent on the number of buttons. Please come up with a better class name.
Attachment #814415 - Flags: review-
Component: Toolbars and Customization → Theme
(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> Review of attachment 814415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +440,5 @@
> >  }
> >  
> >  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1:not(:active):not([open]) + .toolbarbutton-1:not(:hover):not(:active):not([open])::before,
> > +#nav-bar .toolbaritem-with-separator > .toolbarbutton-1[disabled]:not(:active):not([open]) + .toolbarbutton-1[disabled]:not(:active):not([open])::before {
> 
> Why the separate [disabled] rule?

This is because we still want to show the separator in the case that the buttons are disabled (since we aren't going to be showing the button outline in this case). I forgot to remove the :active from that rule too, will do it with the next patch.

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> "toolbaritem-with-separator" suggests that there will be one separator in
> total, but there can be multiple separators, the exact number being
> dependent on the number of buttons. Please come up with a better class name.

Talked over IRC:
<jaws> dao: what do you think of "toolbaritem-combined-buttons" instead of "toolbaritem-with-separator" ?
<dao> jaws: that sounds better
https://hg.mozilla.org/projects/ux/rev/859ff4a1af98

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 814415 [details] [diff] [review]
> Patch
> 
> Review of attachment 814415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but we should probably make sure that we deal with OS X and Linux as
> well.

Updated the OSX styling. Since Linux doesn't have the combined toolbar button styling yet there weren't any changes to make for Linux. That work is tracked in bug 875479.
Whiteboard: [Australis:P5][Australis:M?] → [Australis:P5][Australis:M9][fixed-in-ux]
OS: Windows XP → All
https://hg.mozilla.org/mozilla-central/rev/859ff4a1af98
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P5][Australis:M9][fixed-in-ux] → [Australis:P5][Australis:M9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.