891258 - Use Australis styling for the findbar buttons

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

The new findbar is very nice looking except for the "highlight all" and the "match case" buttons that both still use the old button styling. The Australis styling should be applied to these.

Comment 1

[:Gijs (he/him)]

Is there a design/mockup for this?

Comment 2

[u428464]

Buttons styling unification is wanted (bookmarks bar, additional toolbar, etc.). Specs are available here : http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html (nav bar button)

This old mockup shows the button with the Australis styling : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28FindBar%29.jpg

More info in bug 767319

Comment 3

[Mike de Boer [:mikedeboer]]

Guillaume, this only partly applies to the findbar. We don't use different toolbarbutton styling than the default on Windows and Linux, only on OSX. The 'next' and 'previous' buttons are using the default styling as much as possible, except some CSS is used to merge them together and touch the search input box.

So when button styling is applied in bug 767319, it'll fix this bug too. I'm ok to leave this bug open though, to keep track of the findbar separately.

Comment 4

[u428464]

As discussed with Gijs in bug 767319 comment 7 it seems the original plan was to update the styling of each items (bookmarks bar, library, additional toolbars) separately, but maybe it is not the case anymore.

Comment 5

[:Gijs (he/him)]

Guillaume, why did you change the priority of this bug? :-)

Comment 6

[u428464]

(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #5)
> Guillaume, why did you change the priority of this bug? :-)

Mainly because bug 767319 has been changed to be a P4. ;-)

Comment 7

[Tim Nguyen :ntim]

Taking this :)

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v1

Things you should take in mind while reviewing the patch :
- This patch styles only the Highlight All and Match Case buttons.
- I haven't used the .findbar-button class on the findbar textbox arrows since it would have complicated things. But if that's wanted, I can make that happen.
- I unprefixed the -moz- gradients.
- I copied most of the CSS from shorlander's mockups
- Only tested the patch on Windows 8
- I didn't use the IFDEF pattern since CSS pre-processors weren't parsed for some reason

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8399158 [details] [diff] [review]
Patch v1

Tim, the thing is that Australis is _very_ specific to Firefox, in other words: a browser theme.

The findbar is a component that can be used by any app that is built with the Mozilla codebase, or XULRunner. Examples are Thunderbird and Seamonkey.

When we apply the Australis toolbar styling to the findbar, we'd have to start in the browser theme and override the existing styles with Australis button styles. Incidentally, this will make your job easier, because the main part of the work entails adding selectors to CSS blocks that already exist!

I'm looking forward to see what you'll make of it! Thanks for picking this up!

Comment 10

[u428464]

I know it has already been discussed in bug 767319. But shouldn't it be possible to "simply" change the old styling (still present in the bookmarks toolbar, the findbar, the library and print preview) to the new one browser-wide (but as explained in comment 9 just for Firefox) ?

Comment 11

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #10)
> I know it has already been discussed in bug 767319. But shouldn't it be
> possible to "simply" change the old styling (still present in the bookmarks
> toolbar, the findbar, the library and print preview) to the new one
> browser-wide (but as explained in comment 9 just for Firefox) ?

Mac does this. I always wondered why other platforms dont.

Comment 12

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #10)
> I know it has already been discussed in bug 767319. But shouldn't it be
> possible to "simply" change the old styling (still present in the bookmarks
> toolbar, the findbar, the library and print preview) to the new one
> browser-wide (but as explained in comment 9 just for Firefox) ?
For the bookmarks toolbar, it's possible. But for the library or the print preview, it's a bit more tricky. The best way to do this would be hardcoding the whole UI like Chrome does.

Comment 13

[Mike de Boer [:mikedeboer]]

All elements are reachable with CSS selectors, including ones originating from toolkit. This means that any button can be styled à la Australis by adding these selectors to the `toolbarbutton-1` definitions in each browser theme.
Most elements from toolkit/ or layout/ carry the default `-moz-appearance: foo` style and/ or ones specified by the respective toolkit theme. In browser/ themes, we also override these styles when we need to; the `toolbarbutton-1` class is one such example.

IOW, no need for hardcoding the whole UI - what do you mean by that? - or other more complex things.

Comment 14

[Tim Nguyen :ntim]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> All elements are reachable with CSS selectors, including ones originating
> from toolkit. This means that any button can be styled à la Australis by
> adding these selectors to the `toolbarbutton-1` definitions in each browser
> theme.
> Most elements from toolkit/ or layout/ carry the default `-moz-appearance:
> foo` style and/ or ones specified by the respective toolkit theme. In
> browser/ themes, we also override these styles when we need to; the
> `toolbarbutton-1` class is one such example.
> 
> IOW, no need for hardcoding the whole UI - what do you mean by that? - or
> other more complex things.

Hardcoding the whole UI means scrapping out native rendering and build our own "native" rendering. Kinda like Chrome Aura UI which draws it's own buttons, inputs, controls, ... 

In other words, that means -moz-appearance: toolbarbutton will look like the Australis buttons.

Comment 15

[Tim Nguyen :ntim]

Attachment: Patch v2

Comment 16

[Tim Nguyen :ntim]

Only tested the patch on Windows 8. Can someone test this on Linux, and Windows XP, Vista and 7 ? Thanks :)

Comment 17

[Luís Miguel [:quicksaver] (not reading bugmail)]

(Haven't tested anything, not yet at least)

Mac stylesheet also needs these selector changes, it only styles .toolbarbutton-1's in <toolbar>'s: http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#462

Also:
> 637 	.findbar-button:not([disabled=true]):-moz-any([checked="true"],:hover:active),
shouldn't it be
> 637 	.findbar-button:not([disabled=true]):-moz-any([checked="true"],:hover:active) > .toolbarbutton-text,

Comment 18

[Tim Nguyen :ntim]

(In reply to Luís Miguel [:Quicksaver] from comment #17)
> (Haven't tested anything, not yet at least)
> 
> Mac stylesheet also needs these selector changes, it only styles
> .toolbarbutton-1's in <toolbar>'s:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css#462

Nope, Mac already had the correct styling before this bug.

> Also:
> > 637 	.findbar-button:not([disabled=true]):-moz-any([checked="true"],:hover:active),
> shouldn't it be
> > 637 	.findbar-button:not([disabled=true]):-moz-any([checked="true"],:hover:active) > .toolbarbutton-text,

Yep, good catch :)

Comment 19

[Tim Nguyen :ntim]

Attachment: Patch v3

Comment 20

[Luís Miguel [:quicksaver] (not reading bugmail)]

Attachment: Mac OS X Comparison of nav-bar buttons with findbar buttons

(In reply to Tim Nguyen [:ntim] from comment #18)
> (In reply to Luís Miguel [:Quicksaver] from comment #17)
> > Mac stylesheet also needs these selector changes, it only styles
> > .toolbarbutton-1's in <toolbar>'s:
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> > css#462
> 
> Nope, Mac already had the correct styling before this bug.

I'm definitely missing something then. I just went and checked, all the styling applied to the buttons in Mac's findbar is from the toolkit theme at http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/findBar.css

It's supposed to look like this then? (screenshot) And:

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> We don't use different
> toolbarbutton styling than the default on Windows and Linux, only on OSX.
And this really confused me now... Running a search for ".findbar" in moz-central (http://mxr.mozilla.org/mozilla-central/search?string=.findbar&find=%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) I only see toolkit stylesheets...

I'm sorry about all the confusion, I'm just trying to understand this, my brain won't let it go peacefully. :)

Comment 21

[Tim Nguyen :ntim]

(In reply to Luís Miguel [:Quicksaver] from comment #20)
> Created attachment 8400828 [details]
> Mac OS X Comparison of nav-bar buttons with findbar buttons
> 
> (In reply to Tim Nguyen [:ntim] from comment #18)
> > (In reply to Luís Miguel [:Quicksaver] from comment #17)
> > > Mac stylesheet also needs these selector changes, it only styles
> > > .toolbarbutton-1's in <toolbar>'s:
> > > http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> > > css#462
> > 
> > Nope, Mac already had the correct styling before this bug.
> 
> I'm definitely missing something then. I just went and checked, all the
> styling applied to the buttons in Mac's findbar is from the toolkit theme at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> findBar.css
> 
> It's supposed to look like this then? (screenshot) And:
> 
> (In reply to Mike de Boer [:mikedeboer] from comment #3)
> > We don't use different
> > toolbarbutton styling than the default on Windows and Linux, only on OSX.
> And this really confused me now... Running a search for ".findbar" in
> moz-central
> (http://mxr.mozilla.org/mozilla-central/search?string=.
> findbar&find=%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) I
> only see toolkit stylesheets...
> 
> I'm sorry about all the confusion, I'm just trying to understand this, my
> brain won't let it go peacefully. :)
The Mac styling looks good to me (even if it might be the default one).
Well, the worse case is probably Windows : http://images.devs-on.net/Image/e1GEiiRIvCNtp5VU-Region.png

Comment 22

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Tim Nguyen [:ntim] from comment #21)
> The Mac styling looks good to me (even if it might be the default one).
> Well, the worse case is probably Windows :
> http://images.devs-on.net/Image/e1GEiiRIvCNtp5VU-Region.png

Funny, I have the complete opposite opinion (the round borders and contrasting background in Mac vs its almost-square transparent nav-bar buttons stand out to me; I barely notice the blue in Windows). :)

But I was asking really because I'm not following why OSX's findbar shouldn't adopt the australis button styling, since the others will.

Comment 23

[Tim Nguyen :ntim]

Asking shorlander for decision on Mac's findbar.

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8400807 [details] [diff] [review]
Patch v3

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

::: toolkit/content/widgets/findbar.xml
@@ +181,5 @@
>        <!-- Do not use value, first child is used because it provides a11y with text change events -->
>        </xul:description>
>        <xul:spacer flex="1"/>
>        <xul:toolbarbutton anonid="highlight"
> +                         class="findbar-button findbar-highlight tabbable"

There's not really a need to introduce this class-name.

Can you change the selectors to

```css
.findbar-container > toolbarbutton:not(.close-icon),
```

?

Also, where's the OSX theme? It now also has proper styling for type=checkbox buttons, so they should look classy with them...

Comment 25

[Tim Nguyen :ntim]

(In reply to Mike de Boer [:mikedeboer] from comment #24)
> Comment on attachment 8400807 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8400807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/findbar.xml
> @@ +181,5 @@
> >        <!-- Do not use value, first child is used because it provides a11y with text change events -->
> >        </xul:description>
> >        <xul:spacer flex="1"/>
> >        <xul:toolbarbutton anonid="highlight"
> > +                         class="findbar-button findbar-highlight tabbable"
> 
> There's not really a need to introduce this class-name.
> 
> Can you change the selectors to
> 
> ```css
> .findbar-container > toolbarbutton:not(.close-icon),
> ```
> 
> ?

I don't know, seems more convenient with the class name, in case we introduce something new to the findbar that should remain unstyled.

> Also, where's the OSX theme? It now also has proper styling for
> type=checkbox buttons, so they should look classy with them...
OK :)

What about the up and down buttons ?

Comment 26

[Mike de Boer [:mikedeboer]]

(In reply to Tim Nguyen [:ntim] from comment #25)
> I don't know, seems more convenient with the class name, in case we
> introduce something new to the findbar that should remain unstyled.

We'll deal with that when we get there.

> What about the up and down buttons ?

The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same treatment! The only thing different about 'em is that they're glued together, which is a feature that needs to stay.

Comment 27

[u428464]

(In reply to Mike de Boer [:mikedeboer] from comment #26)
> (In reply to Tim Nguyen [:ntim] from comment #25)
> > What about the up and down buttons ?
> 
> The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same
> treatment! The only thing different about 'em is that they're glued
> together, which is a feature that needs to stay.

I wouldn't be so sure about that. I think UX input is needed here.

Comment 28

[Mike de Boer [:mikedeboer]]

(In reply to Guillaume C. [:ge3k0s] from comment #27)
> I wouldn't be so sure about that. I think UX input is needed here.

I agree.

Comment 29

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #27)
> (In reply to Mike de Boer [:mikedeboer] from comment #26)
> > (In reply to Tim Nguyen [:ntim] from comment #25)
> > > What about the up and down buttons ?
> > 
> > The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same
> > treatment! The only thing different about 'em is that they're glued
> > together, which is a feature that needs to stay.
> 
> I wouldn't be so sure about that. I think UX input is needed here.

Well, we could keep the current styling for Thunderbird,SeaMonkey,... And override that styling in browser.css, I see no reason why we can't do that.

Comment 30

[u428464]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #29)
> (In reply to Guillaume C. [:ge3k0s] from comment #27)
> > (In reply to Mike de Boer [:mikedeboer] from comment #26)
> > > (In reply to Tim Nguyen [:ntim] from comment #25)
> > > > What about the up and down buttons ?
> > > 
> > > The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same
> > > treatment! The only thing different about 'em is that they're glued
> > > together, which is a feature that needs to stay.
> > 
> > I wouldn't be so sure about that. I think UX input is needed here.
> 
> Well, we could keep the current styling for Thunderbird,SeaMonkey,... And
> override that styling in browser.css, I see no reason why we can't do that.

Because those buttons have been design to look like that I think. They don't need to have another styling applied.

Comment 31

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #30)
> (In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment
> #29)
> > (In reply to Guillaume C. [:ge3k0s] from comment #27)
> > > (In reply to Mike de Boer [:mikedeboer] from comment #26)
> > > > (In reply to Tim Nguyen [:ntim] from comment #25)
> > > > > What about the up and down buttons ?
> > > > 
> > > > The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same
> > > > treatment! The only thing different about 'em is that they're glued
> > > > together, which is a feature that needs to stay.
> > > 
> > > I wouldn't be so sure about that. I think UX input is needed here.
> > 
> > Well, we could keep the current styling for Thunderbird,SeaMonkey,... And
> > override that styling in browser.css, I see no reason why we can't do that.
> 
> Because those buttons have been design to look like that I think. They don't
> need to have another styling applied.
That design took in consideration the fact that thunderbird, seamonkey,... also used that widget. Which caused us to use system values, and some quick styling. But for Firefox, we'd like to have the Australis styling.

Comment 32

[u428464]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #31)
> (In reply to Guillaume C. [:ge3k0s] from comment #30)
> > (In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment
> > #29)
> > > (In reply to Guillaume C. [:ge3k0s] from comment #27)
> > > > (In reply to Mike de Boer [:mikedeboer] from comment #26)
> > > > > (In reply to Tim Nguyen [:ntim] from comment #25)
> > > > > > What about the up and down buttons ?
> > > > > 
> > > > > The 'up' and 'down' buttons (or 'next'/ 'previous') should get the same
> > > > > treatment! The only thing different about 'em is that they're glued
> > > > > together, which is a feature that needs to stay.
> > > > 
> > > > I wouldn't be so sure about that. I think UX input is needed here.
> > > 
> > > Well, we could keep the current styling for Thunderbird,SeaMonkey,... And
> > > override that styling in browser.css, I see no reason why we can't do that.
> > 
> > Because those buttons have been design to look like that I think. They don't
> > need to have another styling applied.
> That design took in consideration the fact that thunderbird, seamonkey,...
> also used that widget. Which caused us to use system values, and some quick
> styling. But for Firefox, we'd like to have the Australis styling.

A visual designer should give his opinion on this.

Comment 33

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #31)
> That design took in consideration the fact that thunderbird, seamonkey,...
> also used that widget. Which caused us to use system values, and some quick
> styling. But for Firefox, we'd like to have the Australis styling.

I don't know on what that design was based, but I disagree completely. Those buttons are set apart on every OS, they don't look or function the same way as the others do (they're not "checkboxes"), and should remain so.

(maybe I'm misunderstanding, but I'm reading this as if it was only related to OSX, as in change them only in OSX) Case in point, they weren't touched in the other OS's styling in the current patch, so they shouldn't be touched for the OSX theme either.

Comment 34

[Luís Miguel [:quicksaver] (not reading bugmail)]

I would suggest that this bug treat only the match case and highlight all buttons, as was its original intent. Whether or not the up and down buttons should be re-designed to fit Australis (only in OSX or in all platforms) should be left for another bug.

Comment 35

[Tim Nguyen :ntim]

(In reply to Luís Miguel [:Quicksaver] from comment #33)
> (In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment
> #31)
> > That design took in consideration the fact that thunderbird, seamonkey,...
> > also used that widget. Which caused us to use system values, and some quick
> > styling. But for Firefox, we'd like to have the Australis styling.
> 
> I don't know on what that design was based, but I disagree completely. Those
> buttons are set apart on every OS, they don't look or function the same way
> as the others do (they're not "checkboxes"), and should remain so.
Most of the toolbar buttons in the nav bar are not checkboxes.

> (maybe I'm misunderstanding, but I'm reading this as if it was only related
> to OSX, as in change them only in OSX) Case in point, they weren't touched
> in the other OS's styling in the current patch, so they shouldn't be touched
> for the OSX theme either.
Nope, we're talking for all platforms now. 

(In reply to Luís Miguel [:Quicksaver] from comment #34)
> I would suggest that this bug treat only the match case and highlight all
> buttons, as was its original intent. Whether or not the up and down buttons
> should be re-designed to fit Australis (only in OSX or in all platforms)
> should be left for another bug.
Yes, let's wait until this bug is done.

Comment 36

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #35)
> (In reply to Luís Miguel [:Quicksaver] from comment #33)
> > I don't know on what that design was based, but I disagree completely. Those
> > buttons are set apart on every OS, they don't look or function the same way
> > as the others do (they're not "checkboxes"), and should remain so.
> Most of the toolbar buttons in the nav bar are not checkboxes.

I'm sorry, I should have explained myself better. By "others" I meant the highlight all and match case buttons in the find bar. As in those "change an action", while the up and down "continue an action" of sorts. So they should look as equally "distinct" as their function. which I guess was at least one of the initial points in their current design. But that's a discussion for another place I guess. :)

Comment 37

[Tim Nguyen :ntim]

Attachment: Patch v4

I didn't have time to test the patch yet since I have to do a full rebuild, but here it is anyway.

This patch addresses mac, leaves the 2 up and down buttons untouched. 
OSX was weird to fix, since it didn't use the .toolbarbutton-* classes in the CSS. I just followed the other CSS and did the same thing.

Comment 38

[u428464]

I would consider the up/down buttons as findbar's counterparts to the toolbar's back/forward buttons, so I think the different styling is fine here.

Comment 39

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #38)
> I would consider the up/down buttons as findbar's counterparts to the
> toolbar's back/forward buttons, so I think the different styling is fine
> here.

The toolbar's back/forward buttons styling are not that different from the other nav bar buttons. So, the difference between the find up/down buttons and the other buttons shouldn't be that different.

Comment 40

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #39)
> The toolbar's back/forward buttons styling are not that different from the
> other nav bar buttons. So, the difference between the find up/down buttons
> and the other buttons shouldn't be that different.

The back/forward buttons always have their own background which contrasts with the toolbar, their borders are always visible, their hover styles are (similar but slightly) different from other nav-bar buttons (not checking the styles, so I may be wrong, but looking at them it seems that way to me), and at least the back button is completely round. I'm sorry to sound so direct, but how is this "not that different"?

To me, with this patch's changes to the highlight all and case match buttons, they will have a very similar difference to the up/down buttons as do these nav-bar buttons.

Comment 41

[Mike de Boer [:mikedeboer]]

...except on Linux (on the findbar, I mean).

Comment 42

[Tim Nguyen :ntim]

(In reply to Luís Miguel [:Quicksaver] from comment #40)
> (In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment
> #39)
> > The toolbar's back/forward buttons styling are not that different from the
> > other nav bar buttons. So, the difference between the find up/down buttons
> > and the other buttons shouldn't be that different.
> 
> The back/forward buttons always have their own background which contrasts
> with the toolbar, their borders are always visible, their hover styles are
> (similar but slightly) different from other nav-bar buttons (not checking
> the styles, so I may be wrong, but looking at them it seems that way to me),
> and at least the back button is completely round. I'm sorry to sound so
> direct, but how is this "not that different"?

The :active state box-shadow is the same, the hover state is the same as the normal state. Which isn't the case for the findbar.

Comment 43

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #42)
> The :active state box-shadow is the same, the hover state is the same as the
> normal state. Which isn't the case for the findbar.

It must be the background or the borders that make it seem so different to me then, I'm sorry about that.

Comment 44

[Mike de Boer [:mikedeboer]]

Comment on attachment 8404192 [details] [diff] [review]
Patch v4

Cancelling review, because this patch doesn't make things pretty on OSX :(

Comment 45

[Tim Nguyen :ntim]

(In reply to Mike de Boer [:mikedeboer] from comment #44)
> Comment on attachment 8404192 [details] [diff] [review]
> Patch v4
> 
> Cancelling review, because this patch doesn't make things pretty on OSX :(

I thought I actually made changes to the osx files, they must have been scraped out somehow.

Comment 46

[u428464]

Any news of the patch with OSX ?

Comment 47

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #46)
> Any news of the patch with OSX ?

Awaiting bug 949151 to land.

Comment 48

[Tim Nguyen :ntim]

Attachment: Patch v5 (rebased)

I just realized I actually had an OSX patch, so here it is, rebased.

Comment 49

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8471364 [details] [diff] [review]
Patch v5 (rebased)

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

Please add a .findbar-button class to the buttons that you want to style and then reference that from the CSS. Using a type selector (toolbarbutton) is bad for performance, and it also means that we need to use :not(.close-icon) in a bunch of places.

See https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS#Try_to_put_rules_into_the_most_specific_category_you_can.21 for more information.

Comment 50

[Tim Nguyen :ntim]

Attachment: Patch v6

Comment 51

[Tim Nguyen :ntim]

Comment on attachment 8481932 [details] [diff] [review]
Patch v6

Asking Blair review for the toolkit part

Comment 52

[Tim Nguyen :ntim]

Attachment: Patch v6.1 (r=Unfocused)

Changed reviewer name in description

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/99d6c547dccb

Comment 54

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/99d6c547dccb

Comment 55

[Tim Nguyen :ntim]

Comment on attachment 8482577 [details] [diff] [review]
Patch v6.1 (r=Unfocused)

Approval Request Comment
[Feature/regressing bug #]: Australis
[User impact if declined]: Inconsistent findbar button styling with other toolbar buttons
[Describe test coverage new/current, TBPL]: Landed on m-c
[Risks and why]: Very low, CSS only change
[String/UUID change made/needed]: None

Comment 56

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8482577 [details] [diff] [review]
Patch v6.1 (r=Unfocused)

Taking it because it is very early in the beta cycle.
Should be part of beta 2.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/31dad821234e
https://hg.mozilla.org/releases/mozilla-beta/rev/4815ff146c57

Comment 58

[Dave Townsend [:mossop]]

Attachment: Screenshot 2014-09-03 16.02.48.png

I've backed this out as it has broken the styling of the findbar buttons on mine and other people's machines.

https://hg.mozilla.org/integration/fx-team/rev/0df611ce4a5b
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e804aa81604
https://hg.mozilla.org/releases/mozilla-beta/rev/e7d6edff44d3

Since this patch has no tests (as a pure UI change that's difficult to do) we should be waiting more than 4 hours after reaching mozilla-central to approve an uplift to aurora and beta. At least a few days to allow nightly testers to spot issues.

Comment 59

[Tim Nguyen :ntim]

(In reply to Dave Townsend [:mossop] from comment #58)
> Created attachment 8483741 [details]
> Screenshot 2014-09-03 16.02.48.png
> 
> I've backed this out as it has broken the styling of the findbar buttons on
> mine and other people's machines.
> 
> https://hg.mozilla.org/integration/fx-team/rev/0df611ce4a5b
> https://hg.mozilla.org/releases/mozilla-aurora/rev/0e804aa81604
> https://hg.mozilla.org/releases/mozilla-beta/rev/e7d6edff44d3
> 
> Since this patch has no tests (as a pure UI change that's difficult to do)
> we should be waiting more than 4 hours after reaching mozilla-central to
> approve an uplift to aurora and beta. At least a few days to allow nightly
> testers to spot issues.

Hmm, looks fine for me on Windows, I'll submit another patch tomorrow. Thanks for spotting this !

Comment 60

[Tim Nguyen :ntim]

Attachment: Patch v7

Dave, can you test this on OSX see if the issue is fixed ? Thanks :)

Comment 61

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #58)
> Since this patch has no tests (as a pure UI change that's difficult to do)
> we should be waiting more than 4 hours after reaching mozilla-central to
> approve an uplift to aurora and beta. At least a few days to allow nightly
> testers to spot issues.

This doesn't need to be uplifted at all. It can ride trains as usual.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/0df611ce4a5b

Comment 63

[Dave Townsend [:mossop]]

Attachment: Screenshot 2014-09-04 10.45.42.png

Still no good, this is a screenshot with the highlight all button hovered.

Comment 64

[Tim Nguyen :ntim]

Attachment: Patch v8

Exactly the same patch as v6.1, except it removes some styles in findBar.css.

Dave, can you check if the issue is fixed for you ? Thanks again :)

Comment 65

[Justin Dolske [:Dolske]]

(In reply to Dão Gottwald [:dao] from comment #61)
> (In reply to Dave Townsend [:mossop] from comment #58)
> > Since this patch has no tests (as a pure UI change that's difficult to do)
> > we should be waiting more than 4 hours after reaching mozilla-central to
> > approve an uplift to aurora and beta. At least a few days to allow nightly
> > testers to spot issues.
> 
> This doesn't need to be uplifted at all. It can ride trains as usual.

Concur. Unless there's a special reason for doing so, the expectation is that patches will ride the usual trains. That gives things times to bake and be tested before hitting non-Nightly audiences.

Comment 66

[Tim Nguyen :ntim]

(In reply to Justin Dolske [:Dolske] from comment #65)
> (In reply to Dão Gottwald [:dao] from comment #61)
> > (In reply to Dave Townsend [:mossop] from comment #58)
> > > Since this patch has no tests (as a pure UI change that's difficult to do)
> > > we should be waiting more than 4 hours after reaching mozilla-central to
> > > approve an uplift to aurora and beta. At least a few days to allow nightly
> > > testers to spot issues.
> > 
> > This doesn't need to be uplifted at all. It can ride trains as usual.
> 
> Concur. Unless there's a special reason for doing so, the expectation is
> that patches will ride the usual trains. That gives things times to bake and
> be tested before hitting non-Nightly audiences.

I would consider this as Australis visual polish, and we used to uplift those. Now I guess it's over now that Australis is shipped ?

Comment 67

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Nguyen [:ntim] from comment #66)
> I would consider this as Australis visual polish, and we used to uplift
> those. Now I guess it's over now that Australis is shipped ?

Yeah, Australis shipped in 29, so maybe uplifting something to 30 may have made sense if it was early in the 30-beta cycle, but now that we are a few releases out it would be better to get more testing time on things and make sure that we are shipping more durable code.

Comment 68

[Justin Dolske [:Dolske]]

Correct. Further, Australis was exceptional in that development continued on 29 as that version rode the trains through beta. The normal expectation is that projects are largely finished if they uplift to Aurora.

Comment 69

[Dave Townsend [:mossop]]

Attachment: Screenshot 2014-09-05 13.27.38.png

I feel like this blind patching and asking for someone with an OSX machine to test isn't terribly efficient. Can we find someone with an OSX box to test on to finish up this patch?

Comment 70

[Tim Nguyen :ntim]

Attachment: Patch v9

I couldn't test on OSX, but this should in theory work.

Comment 71

[Dão Gottwald [:dao]]

Comment on attachment 8487417 [details] [diff] [review]
Patch v9

How does this make these findbar buttons look on OS X without browser.css (e.g. in the view-source window)?

Comment 72

[Mike de Boer [:mikedeboer]]

I won't be able to fully review this patch properly this week or the next.

Comment 73

[Tim Nguyen :ntim]

(In reply to Mike de Boer [:mikedeboer] from comment #72)
> I won't be able to fully review this patch properly this week or the next.

The Windows and Linux part has been reviewed already. What needs review is mainly the OSX part.

(In reply to Dão Gottwald [:dao] from comment #71)
> Comment on attachment 8487417 [details] [diff] [review]
> Patch v9
> 
> How does this make these findbar buttons look on OS X without browser.css
> (e.g. in the view-source window)?

I'm really not sure about this, as I don't have an OS X machine to test.

Comment 74

[Tim Nguyen :ntim]

Do you think you can review this any time soon ? Or should I land Windows and Linux first and do OSX later ? My original target was Firefox 34, and it'd be nice if it didn't get pushed to Firefox 36 :)

Comment 75

[Mike de Boer [:mikedeboer]]

Comment on attachment 8487417 [details] [diff] [review]
Patch v9

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

I have to say, this looks rather nice! Kudos for working on this, Tim.

I checked this out first on OSX and that is looking a-ok to me. Then I checked the view-source window and there it didn't look OK. Can you fix that?

Comment 76

[Mike de Boer [:mikedeboer]]

Comment on attachment 8487417 [details] [diff] [review]
Patch v9

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

::: toolkit/themes/osx/global/findBar.css
@@ +63,5 @@
>    }
>  }
>  
>  .findbar-find-next,
> +.findbar-find-previous {

I'm pretty sure you don't want to touch these toolkit styles, but instead override them in browser.css with more specific selectors.

Comment 77

[Tim Nguyen :ntim]

Attachment: Patch v10

This handles OSX view-source window properly (uses the same old style for that area). The patch was fully tested on OSX and Windows.

Comment 78

[Tim Nguyen :ntim]

Comment on attachment 8540116 [details] [diff] [review]
Patch v10

Crap, just noticed I forgot to remove some changes in findbar.css

Comment 79

[Tim Nguyen :ntim]

Attachment: Patch v11

Comment 80

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8540353 [details] [diff] [review]
Patch v11

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

This is missing style changes that are necessary for dark-themed Developer Edition (see http://screencast.com/t/M2T18fIUA for what it looks like on Windows when the Highlight All button is hovered). With dev-edition light-theme, it's not so bad, but the hover state has the wrong color (pressed state looks fine).

Otherwise, these changes look fine for the default theme.

Comment 81

[Tim Nguyen :ntim]

Attachment: Patch v12

Good catch ! Included a fix for devedition.

Comment 82

[Tim Nguyen :ntim]

Attachment: Patch v12.1

Err, one small change didn't make in the previous patch.

Comment 83

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8541213 [details] [diff] [review]
Patch v12.1

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

Looks good!

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/45e273192680

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45e273192680

Comment 86

[Mike de Boer [:mikedeboer]]

Tim, thanks so much for working on this - it looks sooooo much better! Awesome :)

Comment 87

[Cornel Ionce [:noni] [Hubs QA]]

Verified this on: Windows 7 64-bit, Vista 64-bit, XP 32-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit using Nightly 37, build ID: 20150106030201.