Both XP and Vista should use these two files (will land with bug 430759): toolkit/themes/winstripe/global/icons/expand.png toolkit/themes/winstripe/global/icons/collapse.png on the following buttons: -bookmark contextual dialog folder progressive disclosure control -bookmark contextual dialog tag progressive disclosure control -library window properties pane tag progressive disclosure control -library window properties pane progressive disclosure control ("More", Less") I think that is all of the instances of progressive disclosure controls in Firefox, but any others should use these new icons as well. Since this is a simple glyph we don't have different versions for Luna and Aero. The image has been designed to work with high contrast themes.

Are we going to have depressed states for the progressive disclosure controls? The code currently uses chrome://global/skin/arrow/arrow-dn-hov.gif and chrome://global/skin/arrow/arrow-up-hov.gif (which are the same as their non-depressed counterparts, as far as my eyes can tell.)

This patch uses the new icons, and removes the depressed images. I'm not requesting review on this until someone from the UX team answers comment 1.

>Are we going to have depressed states for the progressive disclosure controls? On vista an XP text and images that are drawn on buttons do not shift on down press, so I don't think we should have depressed states. I should also file a follow up bug on getting our textual buttons to behave correctly.

Filed follow up bug 430998 for the behavior of xul buttons.

Comment on attachment 317866 [details] [diff] [review] Patch (v1) > On vista an XP text and images that are drawn on buttons do not shift on down > press, so I don't think we should have depressed states. OK. Then this is ready for review.

I think the paths are wrong in the above patch, chrome://global/skin/collapse.png vs chrome://global/skin/icons/collapse.png

Comment on attachment 317866 [details] [diff] [review] Patch (v1) (In reply to comment #6) > I think the paths are wrong in the above patch, > chrome://global/skin/collapse.png vs chrome://global/skin/icons/collapse.png You're right; I created this before the icons landed. :( Thanks for catching this, new patch forthcoming.

This time with the correct icon locations...

Comment on attachment 317971 [details] [diff] [review] Patch (v1.1) I think this needs UI review. The buttons look too big to me.

Comment on attachment 317971 [details] [diff] [review] Patch (v1.1) Asking for ui-review as per comment 9.

Can you attach screen shots for the ui-r

Oops, I forgot to handle the Mode/Less button in v1.1. Here's a new patch with the same style, handling that button as well. I'll attach a screenshot right away and ask ui-review on it.

I'll attach two other styles as well, and ask ui-r on all of them. Alex: please ui-r+ the styling you think we should use, and then I'll ask review on the corresponding patch.

This version makes the button smaller. A screenshot will follow.

This patch uses round buttons for the progressive disclosure controls.

Personally, I like this style the best.

(In reply to comment #12) > Created an attachment (id=320119) [details] > Patch (v1.2) > > Oops, I forgot to handle the Mode/Less button in v1.1. Here's a new patch with > the same style, handling that button as well. Why did you remove the label? Does it look wrong with both the label and the icon?

(In reply to comment #18) > Why did you remove the label? Does it look wrong with both the label and the > icon? Windows buttons don't usually have both an icon and text.

If we have to opt between the text and the icon, I'd pick the text. The icon looks lost without the expandable UI element next to it.

(In reply to comment #19) > Windows buttons don't usually have both an icon and text. i'm unsure about that, see this screenshot, i think we should follow the same style. this page could be interesting http://msdn.microsoft.com/en-us/library/aa511487.aspx

I don't think that there should be 3D beveling on the round button; that makes it look very much like something out of the old 16-color world. The border should be a single color; the button look should come from a background gradient.

(In reply to comment #20) > If we have to opt between the text and the icon, I'd pick the text. The icon > looks lost without the expandable UI element next to it. I agree. I was just following the description in comment 0. Let's hear from Alex on the ui-r about this as well.

(In reply to comment #21) > Created an attachment (id=320128) [details] > screenshot of windows dialog > > i'm unsure about that, see this screenshot, i think we should follow the same > style. Hmmm, if text-only display is not preferred in the ui-r (see comment 20), then I'd agree with this style. But please note that this is actually *not* a button with an icon. It's a button with a label next to it. To implement this, we would need a change in the XUL structure, which may not be appealing to other platforms. (We can have Windows-specific XUL content of course.) (In reply to comment #22) > I don't think that there should be 3D beveling on the round button; that makes > it look very much like something out of the old 16-color world. The border > should be a single color; the button look should come from a background > gradient. I couldn't get that look via CSS. I had to set -moz-appearance to none to enable round borders, which caused the gradient to drop off, so I decided that having a 3D beveled border is better than nothing. But yeah, it's not perfect. Can you play with it a bit and see if you can obtain the desired "button" effect here, Kai?

(In reply to comment #24) > Hmmm, if text-only display is not preferred in the ui-r (see comment 20), then > I'd agree with this style. But please note that this is actually *not* a > button with an icon. It's a button with a label next to it. To implement yes it is, and it's probably Vista specific, if you see the microsoft page you can find a lot of different implementations for a progressive disclosure, on XP probably the correct button would be something like [More >>] [Less <<], so again a different one...

(In reply to comment #24) > I couldn't get that look via CSS. I had to set -moz-appearance to none to > enable round borders, which caused the gradient to drop off, so I decided that > having a 3D beveled border is better than nothing. But yeah, it's not perfect. > Can you play with it a bit and see if you can obtain the desired "button" > effect here, Kai? > I was thinking something along the lines of supplying our own gradient, via a PNG file used as a background.

OK, let's hear from Alex before trying any other styles here.

If we start to create our own circular button either with CSS or images, we have to start worrying about a wide range of alternate default themes and normal/hit/hover states. This wouldn't be impossible to pull off, but it would take really a lot of work to get it perfect. I recommend that we go with a normal square button for now that is 22x22, using slight smaller icons (I'll attach them).

Comment on attachment 320120 [details] Screenshot of v1.2 notes in comment #28

Comment on attachment 320122 [details] Screenshot of v2 notes in comment #28

Comment on attachment 320124 [details] Screenshot of v3 notes in comment #28

For the properties pane in the library window let's have the progressive disclosure button, and then a label that says "more" or "less" next to it, similar to microsoft's ux guidelines. Personally I think placing the label inside of the button would look better, but I guess we should go for native.

New patch, implementing Alex suggestions in comment 28 and 34. I'll attach a screenshot for ui-r.

Comment on attachment 320156 [details] Screenshot of v1.3 looks good. A lot of these text fields are different heights, we should file a follow up bug on that.

Comment on attachment 320155 [details] [diff] [review] Patch (v1.3) >Index: browser/components/places/content/places.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v >retrieving revision 1.167 >diff -u -8 -p -r1.167 places.js >--- browser/components/places/content/places.js 8 May 2008 04:20:00 -0000 1.167 >+++ browser/components/places/content/places.js 9 May 2008 10:14:09 -0000 >@@ -587,30 +587,39 @@ var PlacesOrganizer = { > * of livemark-children are not likely to fill the infoBox anyway, > * thus we remove the "More/Less" button and show all details. > * > * the wasminimal attribute here is used to persist the "more/less" > * state in a bookmark->folder->bookmark scenario. > */ > var infoBox = document.getElementById("infoBox"); > var infoBoxExpander = document.getElementById("infoBoxExpander"); >+#ifdef XP_WIN >+ var infoBoxExpanderLabel = document.getElementById("infoBoxExpanderLabel"); >+#endif > if (aNode.itemId != -1 && > ((PlacesUtils.nodeIsFolder(aNode) && > !PlacesUtils.nodeIsLivemarkContainer(aNode)) || > PlacesUtils.nodeIsLivemarkItem(aNode))) { > if (infoBox.getAttribute("minimal") == "true") > infoBox.setAttribute("wasminimal", "true"); > infoBox.removeAttribute("minimal"); > infoBoxExpander.hidden = true; >+#ifdef XP_WIN >+ infoBoxExpanderLabel.hidden = true; >+#endif > } > else { > if (infoBox.getAttribute("wasminimal") == "true") > infoBox.setAttribute("minimal", "true"); > infoBox.removeAttribute("wasminimal"); > infoBoxExpander.hidden = false; >+#ifdef XP_WIN >+ infoBoxExpanderLabel.hidden = false; >+#endif > } > }, > > // NOT YET USED > updateThumbnailProportions: function PO_updateThumbnailProportions() { > var previewBox = document.getElementById("previewBox"); > var canvas = document.getElementById("itemThumbnail"); > var height = previewBox.boxObject.height; >@@ -705,25 +714,40 @@ var PlacesOrganizer = { > ctx.translate(-len/2,0); > ctx.mozDrawText(notAvailableText); > ctx.restore(); > }, > > toggleAdditionalInfoFields: function PO_toggleAdditionalInfoFields() { > var infoBox = document.getElementById("infoBox"); > var infoBoxExpander = document.getElementById("infoBoxExpander"); >+#ifdef XP_WIN >+ var infoBoxExpanderLabel = document.getElementById("infoBoxExpanderLabel"); >+#endif > if (infoBox.getAttribute("minimal") == "true") { > infoBox.removeAttribute("minimal"); >+#ifdef XP_WIN >+ infoBoxExpanderLabel.value = infoBoxExpanderLabel.getAttribute("lesslabel"); >+ infoBoxExpanderLabel.setAttribute("accesskey", infoBoxExpanderLabel.getAttribute("lessaccesskey")); >+ infoBoxExpander.setAttribute("class", "up"); nit: use foo.className = ... >Index: browser/themes/winstripe/browser/places/organizer.css >=================================================================== >RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/places/organizer.css,v >retrieving revision 1.16 >diff -u -8 -p -r1.16 organizer.css >--- browser/themes/winstripe/browser/places/organizer.css 5 May 2008 21:05:04 -0000 1.16 >+++ browser/themes/winstripe/browser/places/organizer.css 9 May 2008 10:14:10 -0000 >@@ -261,32 +261,52 @@ > > /**** expanders ****/ > > .expander-up, > .expander-down { > min-width: 0; > } > >+.expander-up > hbox, >+.expander-down > hbox { >+ padding: 1px; >+ border-width: 0; >+} >+ > .expander-up { >- list-style-image: url("chrome://global/skin/arrow/arrow-up.gif"); >+ list-style-image: url("chrome://global/skin/icons/collapse.png"); > } > > .expander-down { >- list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif"); >+ list-style-image: url("chrome://global/skin/icons/expand.png"); >+} >+%endif >+ >+#infoBoxExpander { >+ min-width: 0; > } > >-.expander-down:hover:active { >- list-style-image: url("chrome://global/skin/arrow/arrow-dn-hov.gif"); >+#infoBoxExpander > hbox { >+ padding: 1px; >+ border-width: 0; > } > >-.expander-up:hover:active { >- list-style-image: url("chrome://global/skin/arrow/arrow-up-hov.gif"); >+#infoBoxExpander.up { >+ list-style-image: url("chrome://global/skin/icons/collapse.png"); >+} >+ >+#infoBoxExpander.down { >+ list-style-image: url("chrome://global/skin/icons/expand.png"); >+} >+ >+#infoBoxExpanderLabel { >+ margin: 0; >+ padding: 5px 0; > } >-%endif Seems like it would make sense to move .expander-up/-down out of the ifdef and just use them instead of .up and .down.

(In reply to comment #37) > (From update of attachment 320156 [details]) > A lot of these text fields are different heights, we should file a > follow up bug on that. -> bug 433055.

Addressed the issues in comment 38.

Comment on attachment 320244 [details] [diff] [review] Patch (v1.4) >+.expander-up > hbox, >+.expander-down > hbox { >+ padding: 1px; >+ border-width: 0; > } This looks like it won't work well together with this rule from button.css: button:focus > .button-box { border: 1px dotted ThreeDDarkShadow; }

(In reply to comment #41) > (From update of attachment 320244 [details] [diff] [review]) > >+.expander-up > hbox, > >+.expander-down > hbox { > >+ padding: 1px; > >+ border-width: 0; > > } > > This looks like it won't work well together with this rule from button.css: > > button:focus > .button-box { > border: 1px dotted ThreeDDarkShadow; > } How come? Setting border-width to 0 should cancel the rendering of the border, and I don't see any border on focus for this button. Or maybe you mean that the border is supposed to be there?

Comment on attachment 320244 [details] [diff] [review] Patch (v1.4) (In reply to comment #42) > and I don't see any border on focus for this button. Or maybe you mean that > the border is supposed to be there? Yes, hiding the focus ring would be an accessibility regression.

OK, added back the focus ring.

Comment on attachment 320251 [details] [diff] [review] Patch (v1.5) >+ infoBoxExpanderLabel.setAttribute("accesskey", infoBoxExpanderLabel.getAttribute("lessaccesskey")); could also use .accessKey here

Comment on attachment 320251 [details] [diff] [review] Patch (v1.5) This patch changes winstripe to use the newly added progressive closure images so that we can get closer to the native look and feel of such controls on Windows. Requesting approval.

Comment on attachment 320148 [details] new expand.png This needs to land with this bug as well.

Comment on attachment 320149 [details] New collapse.png This one too.

(In reply to comment #45) > (From update of attachment 320251 [details] [diff] [review]) > >+ infoBoxExpanderLabel.setAttribute("accesskey", infoBoxExpanderLabel.getAttribute("lessaccesskey")); > > could also use .accessKey here Yeah, but for some reason that didn't work. I don't know why, bug I guess the label widget doesn't correctly sync the accessKey property with the accesskey attribute...

Comment on attachment 320251 [details] [diff] [review] Patch (v1.5) we are closed down for showstoppers. We can get this in 3.0.1 if needed.

This can be pushed on mozilla-central now.

Comment on attachment 320251 [details] [diff] [review] Patch (v1.5) This theme only change could be considered for 3.0.1.

http://hg.mozilla.org/index.cgi/mozilla-central/rev/37f0a75e3b2a (In reply to comment #52) > (From update of attachment 320251 [details] [diff] [review]) > This theme only change could be considered for 3.0.1. It's actually not a theme only change.

I can verify the changes based on comment 0 for Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008071303 Minefield/3.1a1pre ID:2008071303 and the appropriate build on XP.

Comment on attachment 320251 [details] [diff] [review] Patch (v1.5) Moving approval request out to 1.9.0.3.

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release). And likewise, doesn't meet the criteria for 1.9.0.4.