Use Australis styling for the findbar buttons

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: ge3k0s, Assigned: ntim)

Tracking

(Depends on 2 bugs)

Trunk
Firefox 37
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 affected)

Details

(Whiteboard: [Australis:P4])

Attachments

(5 attachments, 13 obsolete attachments)

199.80 KB, image/png
Details
66.90 KB, image/png
Details
32.68 KB, image/png
Details
52.61 KB, image/png
Details
16.38 KB, patch
jaws
: review+
Details | Diff | Splinter Review
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.
No longer blocks: 776708
Is there a design/mockup for this?
Flags: needinfo?(ge3k0s)
Whiteboard: [Australis:P5]
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
Flags: needinfo?(ge3k0s)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Whiteboard: [Australis:P5] → [Australis:P4]
Guillaume, why did you change the priority of this bug? :-)
Flags: needinfo?(ge3k0s)
(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. ;-)
Flags: needinfo?(ge3k0s)
Taking this :)
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Posted patch Patch v1 (obsolete) — Splinter Review
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
Attachment #8399158 - Flags: review?(mdeboer)
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!
Attachment #8399158 - Flags: review?(mdeboer)
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) ?
(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.
(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.
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.
(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.
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8399158 - Attachment is obsolete: true
Attachment #8400767 - Flags: review?(mdeboer)
Only tested the patch on Windows 8. Can someone test this on Linux, and Windows XP, Vista and 7 ? Thanks :)
(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,
(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 :)
Posted patch Patch v3 (obsolete) — Splinter Review
Attachment #8400767 - Attachment is obsolete: true
Attachment #8400767 - Flags: review?(mdeboer)
Attachment #8400807 - Flags: review?(mdeboer)
(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. :)
(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
(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.
Asking shorlander for decision on Mac's findbar.
Flags: needinfo?(shorlander)
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...
Attachment #8400807 - Flags: review?(mdeboer) → feedback+
(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 ?
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
(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.
(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.
(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.
(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.
(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.
(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.
(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.
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.
(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.
(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. :)
Posted patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #8400807 - Attachment is obsolete: true
Attachment #8404192 - Flags: review?(mdeboer)
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.
(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.
(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.
...except on Linux (on the findbar, I mean).
(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.
(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 on attachment 8404192 [details] [diff] [review]
Patch v4

Cancelling review, because this patch doesn't make things pretty on OSX :(
Attachment #8404192 - Flags: review?(mdeboer)
(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.
Flags: firefox-backlog?
Any news of the patch with OSX ?
(In reply to Guillaume C. [:ge3k0s] from comment #46)
> Any news of the patch with OSX ?

Awaiting bug 949151 to land.
Flags: firefox-backlog? → firefox-backlog+
Posted patch Patch v5 (rebased) (obsolete) — Splinter Review
I just realized I actually had an OSX patch, so here it is, rebased.
Attachment #8471364 - Flags: review?(mdeboer)
Attachment #8404192 - Attachment is obsolete: true
Attachment #8471364 - Flags: review?(mdeboer) → review?(jaws)
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.
Attachment #8471364 - Flags: review?(jaws) → review-
Posted patch Patch v6 (obsolete) — Splinter Review
Attachment #8471364 - Attachment is obsolete: true
Attachment #8481932 - Flags: review?(jaws)
Comment on attachment 8481932 [details] [diff] [review]
Patch v6

Asking Blair review for the toolkit part
Attachment #8481932 - Flags: review?(bmcbride)
Flags: needinfo?(shorlander)
Attachment #8481932 - Flags: review?(jaws)
Attachment #8481932 - Flags: review?(bmcbride)
Attachment #8481932 - Flags: review+
Posted patch Patch v6.1 (r=Unfocused) (obsolete) — Splinter Review
Changed reviewer name in description
Attachment #8481932 - Attachment is obsolete: true
Attachment #8482577 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/99d6c547dccb
Keywords: checkin-needed
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/99d6c547dccb
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 35
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
QA Whiteboard: [qa+]
Resolution: --- → FIXED
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
Attachment #8482577 - Flags: approval-mozilla-beta?
Attachment #8482577 - Flags: approval-mozilla-aurora?
Iteration: --- → 35.1
Flags: qe-verify?
QA Whiteboard: [qa+]
Flags: qe-verify? → qe-verify+
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.
Attachment #8482577 - Flags: approval-mozilla-beta?
Attachment #8482577 - Flags: approval-mozilla-beta+
Attachment #8482577 - Flags: approval-mozilla-aurora?
Attachment #8482577 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Iteration: 35.1 → ---
(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 !
Posted patch Patch v7 (obsolete) — Splinter Review
Dave, can you test this on OSX see if the issue is fixed ? Thanks :)
Attachment #8482577 - Attachment is obsolete: true
Attachment #8483805 - Flags: feedback?(dtownsend+bugmail)
(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.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/0df611ce4a5b
Target Milestone: Firefox 35 → ---
Attachment #8482577 - Flags: approval-mozilla-beta+
Attachment #8482577 - Flags: approval-mozilla-aurora+
Still no good, this is a screenshot with the highlight all button hovered.
Attachment #8483805 - Flags: feedback?(dtownsend+bugmail) → feedback-
Posted patch Patch v8 (obsolete) — Splinter Review
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 :)
Attachment #8483805 - Attachment is obsolete: true
Attachment #8484288 - Flags: feedback?(dtownsend+bugmail)
(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.
(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 ?
(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.
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.
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?
Attachment #8484288 - Flags: feedback?(dtownsend+bugmail) → feedback-
Posted patch Patch v9 (obsolete) — Splinter Review
I couldn't test on OSX, but this should in theory work.
Attachment #8484288 - Attachment is obsolete: true
Attachment #8487417 - Flags: review?(mdeboer)
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)?
Flags: needinfo?(ntim007)
I won't be able to fully review this patch properly this week or the next.
Status: REOPENED → ASSIGNED
(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.
Flags: needinfo?(ntim007)
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 :)
Flags: needinfo?(mdeboer)
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?
Attachment #8487417 - Flags: review?(mdeboer) → review-
Flags: needinfo?(mdeboer)
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.
Posted patch Patch v10 (obsolete) — Splinter Review
This handles OSX view-source window properly (uses the same old style for that area). The patch was fully tested on OSX and Windows.
Attachment #8487417 - Attachment is obsolete: true
Attachment #8540116 - Flags: review?(jaws)
Comment on attachment 8540116 [details] [diff] [review]
Patch v10

Crap, just noticed I forgot to remove some changes in findbar.css
Attachment #8540116 - Flags: review?(jaws)
Posted patch Patch v11 (obsolete) — Splinter Review
Attachment #8540116 - Attachment is obsolete: true
Attachment #8540353 - Flags: review?(jaws)
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.
Attachment #8540353 - Flags: review?(jaws) → feedback+
Posted patch Patch v12 (obsolete) — Splinter Review
Good catch ! Included a fix for devedition.
Attachment #8540353 - Attachment is obsolete: true
Attachment #8540979 - Flags: review?(jaws)
Posted patch Patch v12.1Splinter Review
Err, one small change didn't make in the previous patch.
Attachment #8540979 - Attachment is obsolete: true
Attachment #8540979 - Flags: review?(jaws)
Attachment #8541213 - Flags: review?(jaws)
Comment on attachment 8541213 [details] [diff] [review]
Patch v12.1

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

Looks good!
Attachment #8541213 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/45e273192680
Keywords: checkin-needed
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/45e273192680
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 37
Iteration: --- → 37.3
Tim, thanks so much for working on this - it looks sooooo much better! Awesome :)
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Depends on: 1125677
You need to log in before you can comment on or make changes to this bug.