428878 - Use a square site button for the windows classic os theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Now that we have the ability to detect what OS theme the user is running, I believe we should use a completely square site button for windows classic mode.  The curvature of the site button has been set to match the curve of the start button on windows, and for windows classic, the curve is clearly illogical since the start button is square.

Dao, do you have any cycles to work on this?

Comment 1

[Kai Liu]

Attachment: patch

I, for one, am absolutely delighted at the possibility of restoring the square and the beveling on Classic.  :)  Since I've whined about this on the forum, I felt obligated to at least contribute a patch (feel free to disregard it, of course, since this is unsolicited)

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 315471 [details] [diff] [review]
patch

>Index: browser/themes/winstripe/browser/browser.css
> #urlbar[chromedir="ltr"] > .autocomplete-textbox-container > .textbox-input-box {
>   border-left: 1px solid ButtonShadow;

While you're at it, can you make that a right border for #identity-box (#urlbar[chromedir="ltr"] > #identity-box)? Putting it there seems more logical.

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Dao: what do you think about cutting the texture from the button in classic
mode?  The texture is supposed to make it look like the button is coming
towards you, but this obviously doesn't make as much sense visually when placed
inside of a beveled text field:

http://img137.imageshack.us/img137/1438/sitebuttoncurvezv3.png  

Also, no other buttons in windows classic have this type of texture, which is
probably reason enough to get rid of it for windows classic.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> Dao: what do you think about cutting the texture from the button in classic
> mode?

Since !windows-default-theme doesn't mean classic, I don't really think that's an option.

Here's how the gradient without the curve looks like on classic:
http://img145.imageshack.us/img145/3944/classicfoxbarsqw9.png

I think that's fine.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 315471 [details] [diff] [review]
patch

>Index: browser/themes/winstripe/browser/browser-aero.css

>-#urlbar[chromedir="ltr"] > #identity-box > hbox {
>+#urlbar[chromedir="ltr"]:-moz-system-metric(windows-default-theme) > #identity-box > hbox {
>   border: 1px solid ButtonShadow;
>   border-right-style: none;
>   margin: -11px -10px -11px 0;
>   padding: 10px 12px 10px 4px;
>   background-position: 0 10px;
>   -moz-border-radius-topleft: 21px;
>   -moz-border-radius-bottomleft: 21px;
> }
> 
>+#urlbar[chromedir="ltr"]:not(:-moz-system-metric(windows-default-theme)) > #identity-box > hbox {
>+  padding: 0 2px;
>+}

Please move the last rule up and remove ":not(:-moz-system-metric(windows-default-theme))" from the selector. Note that we're using "padding: 0 3px" for RTL. There doesn't seem to be a good reason to differentiate at this point.

Comment 6

[Kai Liu]

Attachment: Flat vs. gradient site icons

I agree with Dao; the gradient looks fine with the beveling (if anything, it's the flat buttons that look more out-of-place, but that may be the result of me being so used to the gradient).  Besides, while it's true that you don't see gradients like this in Classic, you also don't see buttons embedded in a text box like this in Classic, so I think we have some license here.  ;)

Comment 7

[Kai Liu]

(In reply to comment #5)
> Note that
> we're using "padding: 0 3px" for RTL.

Should 3px or 2px be used?  I picked the 2px since that was the padding value used for the right side of the curved site button.

> There doesn't seem to be a good reason to
> differentiate at this point.

If 2px is used, shall I take that to mean that RTL should be dropped to 2px to match?

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Should 3px or 2px be used?

Attachment 315479 [details] looks fine, so just use whatever you've been using there.

> > There doesn't seem to be a good reason to differentiate at this point.
> 
> If 2px is used, shall I take that to mean that RTL should be dropped to 2px to
> match?

Yes.

Comment 9

[Kai Liu]

Attachment: RTL issues: Arabic vs. Hebrew

While trying out the patch on a RTL system, I noticed that we are forcing RTL address bars to be LTR in browser.css with the following:

#urlbar .autocomplete-textbox-container {
  direction: ltr;
}

But in intl.css (Arabic locale), there is this:

#urlbar {
  direction: ltr !important;
}

So browser.css makes the entire address bar except for the dropdown button LTR while intl.css (Arabic) goes one step further and makes the entire address bar LTR.  But not all RTL locales do this, which results in inconsistencies between the different RTL locales.  Specifically, in the Arabic locale, the dropdown button is on the right (so it looks like a regular LTR urlbar) while in the Hebrew locale, it's on the left (thus giving it a weird hybrid look).  See screenshot.

* Since the address bar is going to be LTR regardless of locale, the only time that we ever need #urlbar[chromedir= is for selectively applying the address bar curve on LTR only, and other uses could be removed/consolidated.  The patch for the identity button squaring already touches on some of this...

* Should we change the override in browser.css so that it covers #urlbar instead of just #urlbar .autocomplete-textbox-container?  This will let all RTL locales to look the same, and it avoids the weird-looking hybrid look that we get in the Hebrew locale.

Comment 10

[Kai Liu]

Attachment: patch v2a

New patch, addressing Dao's comments.

Comment 11

[Kai Liu]

Attachment: patch v2b

Alternative patch.  This addresses the #urlbar RTL/LTR issues in comment 9.  We may want to do this in a new bug, but since some of the #urlbar RTL/LTR issues are already being addressed by this squarification patch, would it make sense to take care of these other things while we're at it?

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #9)
> But in intl.css (Arabic locale), there is this:
> 
> #urlbar {
>   direction: ltr !important;
> }

Please ping Mano and/or Ehsan about that.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 315555 [details] [diff] [review]
patch v2a

>Index: browser/themes/winstripe/browser/browser.css

>-#urlbar[chromedir="rtl"] > #identity-box > hbox {
>-  border-right: 1px solid ButtonShadow;
>-  border-left: 1px solid ButtonShadow;

Where is the left border going?

Comment 14

[Kai Liu]

(In reply to comment #13)
> (From update of attachment 315555 [details] [diff] [review])
> >Index: browser/themes/winstripe/browser/browser.css
> 
> >-#urlbar[chromedir="rtl"] > #identity-box > hbox {
> >-  border-right: 1px solid ButtonShadow;
> >-  border-left: 1px solid ButtonShadow;
> 
> Where is the left border going?
> 

We only need a left border if there is another element to the left; otherwise, we'd get a double-border as the site icon border bumps up against the urlbar border.  This is one of the motivations for addressing the RTL/LTR problem, because in RTL/Arabic, there is no element to the left of the site icon and we would get a double border (the screenshot in comment 9 sports such a double border), but in RTL/Hebrew and RTL/Farsi, there is an element (the dropdown button).

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 315555 [details] [diff] [review] [details])
> > >Index: browser/themes/winstripe/browser/browser.css
> > 
> > >-#urlbar[chromedir="rtl"] > #identity-box > hbox {
> > >-  border-right: 1px solid ButtonShadow;
> > >-  border-left: 1px solid ButtonShadow;
> > 
> > Where is the left border going?
> > 
> 
> We only need a left border if there is another element to the left; otherwise,
> we'd get a double-border as the site icon border bumps up against the urlbar
> border.  This is one of the motivations for addressing the RTL/LTR problem

But that's not part of that patch, right?

Comment 16

[Kai Liu]

(In reply to comment #15)
> But that's not part of that patch, right?
> 

No, it's not (unless we take the alternate patch).

The problem right now is that preserving the left border will look wrong in the Arabic RTL locale (there is a double-border in the fresh install of 3b5), and removing it as I have done will look wrong in the other two RTL locales.  So I don't think that there is a correct way to handle this border at this time, because either way, it's going to look wrong somewhere.  This is why I didn't request any reviews for this, because until the RTL consistency thing is resolved (I've sent an e-mail to both Mano and Ehsan), either in this bug or in a new spinoff bug, the border situation is going to be murky.

Comment 17

[(no longer active)]

Firstly, I think someone from the Arabic l10n team should be CCed on this bug (if they're not already).  I don't know anyone, but I took my chance and looked at the ar l10n component in bugzilla and CCed <[e-mail]> here.

Here is what I think about this.  I think generally we would want the drop-down to appear at the left side (like all combo-boxes in RTL mode), so the default behavior should follow this assumption.  Locales which choose different layouts for their location bar should be expected to tweak other parts of the layout to match their specific needs.

So I propose a comment in the code (possibly in l10n/ar's intl.css) to make them aware of the required changes to their locales, and go with the assumption of a drop-down at the left side of the location bar.  A patch on ar's intl.css would be super-nice, but I guess that would be pushing this too far.  :-)

BTW, with the changes made to the location bar for Fx3, I really prefer what ar locale is doing (because the location bar has diverged much from the concept on a "combo-box with an icon"), but anyway I think it's better not to break current RTL layout in the main code base.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #16)
> The problem right now is that preserving the left border will look wrong in the
> Arabic RTL locale (there is a double-border in the fresh install of 3b5)

Right, I think they need to stop hacking the theme in intl.css. Whatever the right solution is should be put in browser.css.

Comment 19

[Kai Liu]

Attachment: patch v3

Same as attachment 315555 [details] [diff] [review], except that a right-border has been added to the dropmarker button in RTL.  I opted for adding a right border to the dropmarker button instead of a left border to the site icon button because the dropmarker button already has LTR-specific styles, so by adding the border to the dropmarker instead of the site icon, we can consolidate all of #urlbar's LTR/RTL-specific stuff unrelated to the curve to a single element, which should also make things slightly easier for the l10n teams that wish to adjust things on their own.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> I opted for adding a right border to the dropmarker
> button instead of a left border to the site icon button because the dropmarker
> button already has LTR-specific styles, so by adding the border to the
> dropmarker instead of the site icon, we can consolidate all of #urlbar's
> LTR/RTL-specific stuff unrelated to the curve to a single element

Seems like this makes the styling less logical and harder to follow again (a problem that I wanted to solve in comment 2). The border really belongs to the identity box.

> also make things slightly easier for the l10n teams that wish to adjust things
> on their own.

I still think they shouldn't do that and we shouldn't try to support it.

Comment 21

[Tomer Cohen :tomer]

You might wish to merge the changes I suggested in bug 427729 for the Go button. Together, I think both addresses all the RTL-UI issues left in the new location bar. 

We should change this bug to every platform, but this will require us to provide patch the other themes for OSX and GTK. Please apply the patch to these platforms  too.

Comment 22

[Kai Liu]

Attachment: patch v3b

Same as attachment 315592 [details] [diff] [review], except move the border from the dropmarker button to the site icon button... I think that either the border belonging to the drop button or the border belonging to the site icon makes logical sense, so it doesn't matter that much to me; I shall do as you request.

Comment 23

[Kai Liu]

Comment on attachment 315598 [details] [diff] [review]
patch v3b

Since there hasn't been any other comments, I'll request a review now.

I'm not really sure who I should be requesting reviews from (and I've requested reviews from the wrong person more than once), so feel free to change that if I got it wrong...

Comment 24

[Kai Liu]

Attachment: patch v3.1; previous patch had bitrotted

Theme stuff has been changing a lot these days; this is just a bitrot update.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 316331 [details] [diff] [review]
patch v3.1; previous patch had bitrotted

Setting the ui review to beltzner.  I'm obviously personally in favor of the change, but I want to get his feedback.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316331 [details] [diff] [review]
patch v3.1; previous patch had bitrotted

r+uir+a=beltzner

Comment 27

[Kai Liu]

Thanks!  Unfortunately, with all the checkins that have been going on, this patch has bitrotted slightly, so I'll need to update it.  I think it's probably best if I wait for bug 422559 and bug 417844 to land first before unbitrotting.

Comment 28

[Kai Liu]

Attachment: patch v4 (unbitrot)

Comment 29

[Kai Liu]

Attachment: patch v4.1 (unbitrot, fix minor error in prev. patch)

Oops, made a minor error in my previous patch...

Dao, may I ask why you are using a darker right border without a white line for the curved button but not for the RTL button?

Comment 30

[Kai Liu]

Attachment: after

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 317647 [details] [diff] [review]
patch v4.1 (unbitrot, fix minor error in prev. patch)

(In reply to comment #29)
> Dao, may I ask why you are using a darker right border without a white line for
> the curved button but not for the RTL button?

I couldn't use the inner rgba border for LTR because |#identity-box > hbox| has a negative right margin in that case.

Comment 32

[Kai Liu]

Comment on attachment 317647 [details] [diff] [review]
patch v4.1 (unbitrot, fix minor error in prev. patch)

Requesting approval (not sure if I should carry over the old one).

r=beltzner,dao
ui-r=beltnzer

Note: This patch was made against trunk+422559 (which is currently awaiting checkin), so it'll probably be a good idea to wait for bug 422559 to land first.

Comment 33

[Damon Sicore (:damons)]

Comment on attachment 317647 [details] [diff] [review]
patch v4.1 (unbitrot, fix minor error in prev. patch)

a1.9+=damons

Comment 35

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/themes/winstripe/browser/browser-aero.css 	1.10
mozilla/browser/themes/winstripe/browser/browser.css 	1.211
mozilla/browser/themes/winstripe/browser/searchbar.css 	1.29 

Comment 36

[Darren Kalck [:D-Kalck]]

Attachment: Site button is square on non-classic windows theme

I'm using Bruna theme (non-classic theme) by lassekongo83 and the site button is square.

Comment 37

[Alex Faaborg [:faaborg] (Firefox UX)]

Unfortunately we can't differentiate between third party themes and the classic themes.  So the summary is correct in that w are now using the square site button on the windows classic theme, but I guess I should have said "on all non-default themes."

Comment 38

[Kai Liu]

The theme detection currently makes the distinction between official Microsoft signed themes (i.e., Luna, Royale, Zune, and Aero) and everything else (Classic, third-party).  It may be worthwhile in the future to add another qualifier to distinguish between uxthemed and non-uxthemed if we want more fine-grained control.  But for now, this is what we have.

Comment 39

[mmortal03]

Since we decided to fix this bug, shouldn't we apply the same logic to the back/forward buttons and also have their edges squared in the windows classic os theme? I'll file a bug report for it if we think we should.  Btw, I do think we should.

Comment 40

[Kai Liu]

(In reply to comment #39)
> Since we decided to fix this bug, shouldn't we apply the same logic to the
> back/forward buttons and also have their edges squared in the windows classic
> os theme? I'll file a bug report for it if we think we should.  Btw, I do think
> we should.
> 

That could be a pretty tough sell.  And it would also require fixing bug 216266 (which is a problem that has been around since before the days of Firefox 1).  I plan on wrapping everything in bug 430366 that doesn't get addressed by final into an addon, and maybe an addon like that would be a good place for a back/forward restoration.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre ID:2008042806

I switched to classic mode on XP and had to restart the browser to take the theme change in effect. The location bar looks like the given screenshots.

I will file a RFE which will cover a possible theme change without the need to restart the browser.

Comment 42

[Darren Kalck [:D-Kalck]]

Is there an easy to have the rounded site button with ux-themes ?

Comment 43

[Kai Liu]

(In reply to comment #42)
> Is there an easy to have the rounded site button with ux-themes ?
> 

In the future, it'd be nice to have another -moz-system-metric that distinguishes between uxtheme and non-uxtheme (should be very easy to do).  In the meantime, you can use the following addon: <https://addons.mozilla.org/firefox/7151> (or an equivalent userChrome.css)