Use a square site button for the windows classic os theme

VERIFIED FIXED in Firefox 3

Status

()

defect
VERIFIED FIXED
11 years ago
2 years ago

People

(Reporter: faaborg, Assigned: kliu)

Tracking

Trunk
Firefox 3
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

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?
(Reporter)

Updated

11 years ago
Summary: Use a square site button for window classic → Use a square site button for the windows classic os theme
Assignee: nobody → dao
(Assignee)

Comment 1

11 years ago
Posted patch patch (obsolete) — Splinter Review
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 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.
Assignee: dao → kliu.bugzilla.3c9f
Status: NEW → ASSIGNED
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.
(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 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.
(Assignee)

Comment 6

11 years ago
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.  ;)
(Assignee)

Comment 7

11 years ago
(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?
(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.
(Assignee)

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
Posted patch patch v2a (obsolete) — Splinter Review
New patch, addressing Dao's comments.
(Assignee)

Comment 11

11 years ago
Posted patch patch v2b (obsolete) — Splinter Review
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?
(Assignee)

Updated

11 years ago
Attachment #315471 - Attachment is obsolete: true
(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 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?
(Assignee)

Comment 14

11 years ago
(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).
(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?
(Assignee)

Comment 16

11 years ago
(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

11 years ago
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 <mozilla@aymanh.com> 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.
(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.
(Assignee)

Comment 19

11 years ago
Posted patch patch v3 (obsolete) — Splinter Review
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.
Attachment #315555 - Attachment is obsolete: true
Attachment #315556 - Attachment is obsolete: true
(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.
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.
(Assignee)

Comment 22

11 years ago
Posted patch patch v3b (obsolete) — Splinter Review
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.
(Assignee)

Comment 23

11 years ago
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...
Attachment #315598 - Flags: ui-review?(faaborg)
Attachment #315598 - Flags: review?(mconnor)
(Assignee)

Comment 24

11 years ago
Theme stuff has been changing a lot these days; this is just a bitrot update.
Attachment #315592 - Attachment is obsolete: true
Attachment #315598 - Attachment is obsolete: true
Attachment #316331 - Flags: ui-review?(faaborg)
Attachment #316331 - Flags: review?(mconnor)
Attachment #315598 - Flags: ui-review?(faaborg)
Attachment #315598 - Flags: review?(mconnor)
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.
Attachment #316331 - Flags: ui-review?(faaborg) → ui-review?(beltzner)
(Assignee)

Updated

11 years ago
Blocks: 430366
(Assignee)

Updated

11 years ago
Blocks: 430414
Comment on attachment 316331 [details] [diff] [review]
patch v3.1; previous patch had bitrotted

r+uir+a=beltzner
Attachment #316331 - Flags: ui-review?(beltzner)
Attachment #316331 - Flags: ui-review+
Attachment #316331 - Flags: review?(mconnor)
Attachment #316331 - Flags: review+
Attachment #316331 - Flags: approval1.9+
(Assignee)

Comment 27

11 years ago
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.
Keywords: checkin-needed
(Assignee)

Updated

11 years ago
Depends on: 417844, 422559
(Assignee)

Comment 28

11 years ago
Posted patch patch v4 (unbitrot) (obsolete) — Splinter Review
Attachment #316331 - Attachment is obsolete: true
Attachment #317645 - Flags: review?(dao)
(Assignee)

Comment 29

11 years ago
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?
Attachment #317645 - Attachment is obsolete: true
Attachment #317647 - Flags: review?(dao)
Attachment #317645 - Flags: review?(dao)
(Assignee)

Comment 30

11 years ago
Posted image after
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.
Attachment #317647 - Flags: review?(dao) → review+
(Assignee)

Comment 32

11 years ago
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.
Attachment #317647 - Flags: approval1.9?
Comment on attachment 317647 [details] [diff] [review]
patch v4.1 (unbitrot, fix minor error in prev. patch)

a1.9+=damons
Attachment #317647 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Whiteboard: [Wait for 422559 to land first]
(Assignee)

Updated

11 years ago
Duplicate of this bug: 427207
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 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [Wait for 422559 to land first]
Target Milestone: --- → Firefox 3
I'm using Bruna theme (non-classic theme) by lassekongo83 and the site button is square.
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."
(Assignee)

Comment 38

11 years ago
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

11 years ago
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.
(Assignee)

Comment 40

11 years ago
(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.
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.
Status: RESOLVED → VERIFIED
Is there an easy to have the rounded site button with ux-themes ?
(Assignee)

Comment 43

11 years ago
(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)
You need to log in before you can comment on or make changes to this bug.