Closed Bug 414183 Opened 17 years ago Closed 17 years ago

Style site button and search button to mirror the keyhole form on Windows

Categories

(Firefox :: Theme, defect, P3)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta5

People

(Reporter: faaborg, Assigned: dao)

References

Details

(Keywords: polish)

Attachments

(7 files, 5 obsolete files)

The new theme for windows was designed to mirror curves found in the keyhole form of the navigation controls throughout the navigation toolbar. This helps the keyhole form feel like it fits into the design as a whole, and creates a sleeker and more aerodynamic appearance throughout the browser. Proto is already updated with this change, and since the keyhole shaped navigation controls are about to land for Windows, we need to update the site button and search button as part of the visual refresh. Since this bug touches the site button, we are also wrapping in visual changes for the EV state. Dao: beltzner said on IRC that you might potentially have cycles to work on this.
Flags: blocking-firefox3?
Attachment #299523 - Flags: ui-review?(beltzner)
Notes from the mockup: -The intersecting circle on XP should be 42x42 given a text field that is 22 pixels tall. -The intersecting circle on Vista should be 28x28 given a text field that is 22 pixels tall.
Summary: Style site button and search button to mirror the keyhole form on Windows → Style site button and search button to mirror the keyhole form on Windows, add support for EV
This mockup covers visual changes to the site button for different identity states. Notes: -These styles emphasize that the site button is chrome and not content. -I just did a quick gradient map on the Vista forward button, this is only an example of how we might style the EV site button with a matching texture. On XP the navigation controls are already green, so we will definitely want to match the existing controls.
Attachment #299530 - Flags: ui-review?(beltzner)
Could I please get a mockup of how this is expected to work with a non-default font size?
Summary: Style site button and search button to mirror the keyhole form on Windows, add support for EV → Style site button and search button to mirror the keyhole form on Windows
>Could I please get a mockup of how this is expected to work with a non-default >font size? Sure thing, I've been giving it some thought and there are a few different ways we could handle it. Overall it will probably come down to what is the easiest to implement though, since this is a boundary case.
(In reply to comment #4) > >Could I please get a mockup of how this is expected to work with a non-default > >font size? > > Sure thing, I've been giving it some thought and there are a few different ways > we could handle it. Overall it will probably come down to what is the easiest > to implement though, since this is a boundary case. Problem is, even though I could imagine a few possible styles, I don't see a way to implement any with the tools that we have: CSS, images and theme-specific XBL. The latter should be avoided for performance reasons. Btw, I think we've been using the dropmarker for menus only. And strictly speaking the identity panel isn't a menu.
Yet, the drop-down arrow makes it clear that it can be clicked. If I hadn't read about the identity panel, it would never click it I guess.
We'd be fixing the current asymmetry (A) by creating a new one (B). Alex says that's not a showstopper, Safari is already doing something similar. I agree, asymmetry between the Location bar and the Search bar isn't a showstopper. However, compared to A it's more visible. So if B is considered minor, that challenges the reasoning behind this proposal.
Dão, I agree that the asymmetry created with B isn't ideal. What do you think of potentially only curving the site button?
(A) is less of a problem (at least less in-your-face than B), because the size and appearance of the back/forward item and the site button are different, and because there are two icons in between. So I don't think that's worth it. But sure, one curve would be better than two :)
At the very least, the site button needs to not look as ... awkward and puffy? ... as it does now. That's probably achievable by not using toolbox styling.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
OS: Windows XP → Windows Vista
After thinking about this more, I'm concerned about how using images for the site button would mesh with the user switching OS themes. Out of curiosity, how did we get the tab strip to correctly match to the OS theme in Firefox 2?
It seems to be a color blending with the theme background color. At the top, it's Luna, bellow is Knorr.
We've been using semi-transparent images and -moz-dialog as the background color. For Firefox 3, we also use ThreeDShadow for some of the tabs' borders.
-I think we should exactly match the texture of the tabs, but have the glass reflection centered vertically, like the forward button -I agree with Dao that the curve looks bad up against the right side of the location bar, the search button needs to be square -For consistency I think the site button should get a drop marker (it isn't a menu, but it does contain widgets that you can select to take you to other places, and hitting it produces a panel) -We should use the same ratio as the forward button for the curve, Vista uses an intersecting circle that is 28 pixels tall, XP has one that is 42 pixels tall.
Since we are proposing that we implement the buttons with images, they should just stretch vertically when the font changes size in the location bar.
What's the base background color, what's the border color (opaque or transparent, necessarily not a system color)? Are there hover and active states? What about RTL?
Comment on attachment 299530 [details] Site button identity states (Normal, SSL, EV) Crap, I should have plussed this a while back. It's an important thing for us to get right in terms of being able to echo the etch and keyhole design. I think in the SSL case, we're going to want the background to be blue. Dao: can we assign this to you? Will it take long to work on?
Attachment #299530 - Flags: ui-review?(beltzner) → ui-review+
I've already been assigned to this bug, but IMHO the specification isn't workable yet.
(In reply to comment #16) > What's the base background color, what's the border color (opaque or > transparent, necessarily not a system color)? We're getting Vlad to see if he can help us get a system colour, but for now let's just use ButtonFace (see http://iansgraham.com/books/xhtml1/appd/update-23feb2000.html to see what that looks like) > Are there hover and active states? On XP I think there probably should be, yes. On Vista I believe just active. Alex might have more thoughts here. > What about RTL? Yup, we'll need an RTL version, too. (In reply to comment #18) > I've already been assigned to this bug, but IMHO the specification isn't > workable yet. So, to be clear, what you need to continue are: - background colour - border colour - hover/active state descriptions
Our RTL interface isn't just mirrored, because the location bar is always LTR (except for the dropmarker).
(In reply to comment #19) > for now let's just use ButtonFace I see a fundamental problem here: Along the curve, we want transparency on the left side and some native color on the right side. That doesn't seem doable with CSS and PNG images. So for the curve we need to use SVG (implies a Ts/Txul hit) or a non-native background color (and border color too, as already mentioned).
>I've already been assigned to this bug, but IMHO the specification isn't >workable yet. Sorry, I thought matching the appearance of the tab background image would be good enough but as you pointed out that doesn't account for the border, and we can't draw the border with css because of the curve. >but for now let's just use ButtonFace The problem with buttonface this is that it gives us a grey shade on a light purple toolbar on Vista. Ideally we would use a transparent image and the base color would be the toolbar itself, similar to how the tab strip lets the background color shine through. I honestly don't know what we should do about the borders. Could we pick a shade of grey for the edges that when placed over the toolbar looks ok? Note how the proto end cap buttons don't fade exactly into the line color of the text field, but it still works reasonably well.
I found a way to use CSS colors for the background and the border.
Attached patch patch (obsolete) — Splinter Review
- same radius for XP and Vista - no special image for the shading, just reusing the image of the active tab - no hover state, no active state All that could be tweaked with little risk after beta 4. But basically, this patch should work. I couldn't resist cleaning up more of larry's css code.
Attachment #305262 - Flags: review?(gavin.sharp)
Attached image screenshots
It looks nice, but the drop-marker is missing.
Attached patch patch v2 (obsolete) — Splinter Review
using a 21px radius for XP, the Vista code is commented out.
Attachment #305262 - Attachment is obsolete: true
Attachment #305341 - Flags: review?(gavin.sharp)
Attachment #305262 - Flags: review?(gavin.sharp)
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #305341 - Attachment is obsolete: true
Attachment #305827 - Flags: review?(gavin.sharp)
Attachment #305341 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Component: OS Integration → Theme
QA Contact: os.integration → theme
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #305827 - Attachment is obsolete: true
Attachment #306350 - Flags: review?(gavin.sharp)
Attachment #305827 - Flags: review?(gavin.sharp)
Comment on attachment 306350 [details] [diff] [review] updated to trunk >Index: browser/themes/gnomestripe/browser/browser.css >+#identity-popup-content-host , > #identity-popup-content-owner { >- font-weight: bold; >- max-width: 300px; >- margin-bottom: 0 !important; >+ font-size: 140%; >+ font-weight: bold; >+ max-width: 300px; > } This makes the larger font size apply to |.verifiedDomain #identity-popup-content-owner| when it didn't used to. Not sure that's desired. Same comment applies to the other files as well. >-.verifiedDomain > #identity-popup-container > #identity-popup-icon { >- -moz-image-region: rect(64px, 64px, 128px, 0px); >+#identity-popup.verifiedDomain > #identity-popup-container > #identity-popup-icon { >+ -moz-image-region: rect(64px, 64px, 128px, 0px); I'm not sure this extra specificity actually helps. I know very little about CSS selector matching beyond what's in http://developer.mozilla.org/en/docs/Writing_Efficient_CSS, but http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Don.27t_qualify_ID-categorized_rules_with_tag_names_or_classes makes me think this might not actually be better than what was there before. Unless there's some reason the extra specificity is needed? I just skimmed the new styling rules and didn't review them in depth, because you're much better at determining the best way to style something than I am, and it looks like you've covered all the bases with testing this.
Attachment #306350 - Flags: review?(gavin.sharp) → review+
I noticed that the patch adds lines with -moz-pre-wrap, would it make sense to use pre-wrap instead? I say this because I saw that in bug 418543, roc says that -moz-pre-wrap is deprecated and its use should be phased out. (There is a patch waiting for approval that converts all instances -moz-pre-wrap to pre-wrap.)
(In reply to comment #30) > This makes the larger font size apply to |.verifiedDomain > #identity-popup-content-owner| when it didn't used to. Not sure that's desired. It's not desired, since the owner label says "(no information provided)" in that case. I'll fix that. > >-.verifiedDomain > #identity-popup-container > #identity-popup-icon { > >- -moz-image-region: rect(64px, 64px, 128px, 0px); > >+#identity-popup.verifiedDomain > #identity-popup-container > #identity-popup-icon { > >+ -moz-image-region: rect(64px, 64px, 128px, 0px); > > I'm not sure this extra specificity actually helps. I know very little about > CSS selector matching beyond what's in > http://developer.mozilla.org/en/docs/Writing_Efficient_CSS, but > http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Don.27t_qualify_ID-categorized_rules_with_tag_names_or_classes > makes me think this might not actually be better than what was there before. > Unless there's some reason the extra specificity is needed? .genericClass > #concreteElement isn't in the spirit of "Writing Efficient CSS" (which states: "all of our classes will be unique"). I don't think my change makes it significantly more or less efficient, but I do think it makes these rules more meaningful. (In reply to comment #31) > I noticed that the patch adds lines with -moz-pre-wrap, would it make sense to > use pre-wrap instead? Yes, thanks.
Comment on attachment 305264 [details] screenshots Is RTL broken here?
(In reply to comment #33) > (From update of attachment 305264 [details]) > Is RTL broken here? No -- see comment 20.
Attached patch comments addressed (obsolete) — Splinter Review
Attachment #306350 - Attachment is obsolete: true
Attachment #306543 - Flags: approval1.9?
Attached patch updated to trunkSplinter Review
Attachment #306543 - Attachment is obsolete: true
Attachment #306824 - Flags: approval1.9?
Attachment #306543 - Flags: approval1.9?
Keywords: checkin-needed
Attachment #306824 - Flags: approval1.9?
Checking in browser/themes/gnomestripe/browser/browser.css; /cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v <-- browser.css new revision: 1.198; previous revision: 1.197 done Checking in browser/themes/pinstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v <-- browser.css new revision: 1.131; previous revision: 1.130 done Checking in browser/themes/winstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v <-- browser.css new revision: 1.184; previous revision: 1.183 done Checking in browser/themes/winstripe/browser/searchbar.css; /cvsroot/mozilla/browser/themes/winstripe/browser/searchbar.css,v <-- searchbar.css new revision: 1.24; previous revision: 1.23 done Checking in browser/base/content/browser.css; /cvsroot/mozilla/browser/base/content/browser.css,v <-- browser.css new revision: 1.59; previous revision: 1.58 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
According to https://bugzilla.mozilla.org/attachment.cgi?id=299530 The dropdown icon is missing and the domain of the ssl connection is also missing.
(In reply to comment #38) > According to https://bugzilla.mozilla.org/attachment.cgi?id=299530 > The dropdown icon is missing and the domain of the ssl connection is also > missing. Bug 414627 deals with domain display on SSL.
Blocks: 406021
Blocks: 402018
Blocks: 395248
Should I file a new bug for missing dropdown ?
On Windows Classic, the site button is not connected smoothly to the location bar. Screenshot: http://img407.imageshack.us/img407/3456/identityglitchrd0.png
The current one looks better IMHO.
(In reply to comment #40) > Should I file a new bug for missing dropdown ? Yes.
Depends on: 420958
Target Milestone: Firefox 3 → Firefox 3 beta5
>using a 21px radius for XP, the Vista code is commented out. /*margin: -4px 0; + padding: 3px 2px 3px 4px; + background-position: 0 3px; + -moz-border-radius-topleft: 14px; + -moz-border-radius-bottomleft: 14px;*/ Can we get these lines added to browser-aero.css or however we plan on using chrome overrides for vista specific themeing.
Blocks: 425582
(In reply to comment #45) > Can we get these lines added to browser-aero.css or however we plan on using > chrome overrides for vista specific themeing. Someone needs to fork browser.css. I can't do that.
Depends on: 425931
Attachment #299523 - Flags: ui-review?(beltzner)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: