Closed Bug 430902 Opened 16 years ago Closed 16 years ago

Use new expand and collapse progressive disclosure controls

Categories

(Firefox :: Theme, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1a1

People

(Reporter: faaborg, Assigned: ehsan.akhgari)

References

()

Details

(Whiteboard: [has patch][has review][RC2-])

Attachments

(5 files, 10 obsolete files)

23.83 KB, image/png
Details
240 bytes, image/png
mtschrep
: approval1.9-
Details
238 bytes, image/png
mtschrep
: approval1.9-
Details
48.30 KB, image/png
faaborg
: ui-review+
Details
8.79 KB, patch
dao
: review+
mtschrep
: approval1.9-
samuel.sidler+old
: approval1.9.0.4-
Details | Diff | Splinter Review
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.)
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Attachment #317866 - Flags: review?
>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.
Attachment #317866 - Flags: review? → review?(gavin.sharp)
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.
Attachment #317866 - Attachment is obsolete: true
Attachment #317866 - Flags: review?(gavin.sharp)
Attached patch Patch (v1.1) (obsolete) — Splinter Review
This time with the correct icon locations...
Attachment #317971 - Flags: review?(gavin.sharp)
Whiteboard: [has patch] [needs review gavin]
Attachment #317971 - Flags: review?(gavin.sharp) → review?(dao)
Whiteboard: [has patch] [needs review gavin] → [has patch] [needs review dao]
Comment on attachment 317971 [details] [diff] [review]
Patch (v1.1)

I think this needs UI review. The buttons look too big to me.
Attachment #317971 - Flags: review?(dao) → review+
Comment on attachment 317971 [details] [diff] [review]
Patch (v1.1)

Asking for ui-review as per comment 9.
Attachment #317971 - Flags: ui-review?(faaborg)
Whiteboard: [has patch] [needs review dao] → [has patch] [needs ui-review faaborg]
Can you attach screen shots for the ui-r
Attached patch Patch (v1.2) (obsolete) — Splinter Review
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.
Attachment #317971 - Attachment is obsolete: true
Attachment #317971 - Flags: ui-review?(faaborg)
Attached image Screenshot of v1.2 (obsolete) —
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.
Attachment #320120 - Flags: ui-review?(faaborg)
Attached patch Patch (v2) (obsolete) — Splinter Review
This version makes the button smaller.  A screenshot will follow.
Attached image Screenshot of v2 (obsolete) —
Attachment #320122 - Flags: ui-review?(faaborg)
Attached patch Patch (v3) (obsolete) — Splinter Review
This patch uses round buttons for the progressive disclosure controls.
Attached image Screenshot of v3 (obsolete) —
Personally, I like this style the best.
Attachment #320124 - Flags: ui-review?(faaborg)
Attachment #320123 - Attachment is patch: true
Attachment #320123 - Attachment mime type: application/octet-stream → text/plain
(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).
Attached image new expand.png
Attached image New collapse.png
Comment on attachment 320120 [details]
Screenshot of v1.2

notes in comment #28
Attachment #320120 - Flags: ui-review?(faaborg) → ui-review-
Comment on attachment 320122 [details]
Screenshot of v2

notes in comment #28
Attachment #320122 - Flags: ui-review?(faaborg) → ui-review-
Comment on attachment 320124 [details]
Screenshot of v3

notes in comment #28
Attachment #320124 - Flags: ui-review?(faaborg) → ui-review-
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.
Whiteboard: [has patch] [needs ui-review faaborg] → [needs new patch]
Attached patch Patch (v1.3) (obsolete) — Splinter Review
New patch, implementing Alex suggestions in comment 28 and 34.  I'll attach a screenshot for ui-r.
Attachment #320119 - Attachment is obsolete: true
Attachment #320120 - Attachment is obsolete: true
Attachment #320121 - Attachment is obsolete: true
Attachment #320122 - Attachment is obsolete: true
Attachment #320123 - Attachment is obsolete: true
Attachment #320124 - Attachment is obsolete: true
Attachment #320155 - Flags: review?(dao)
Attached image Screenshot of v1.3
Attachment #320156 - Flags: ui-review?(faaborg)
Attachment #320155 - Attachment is patch: true
Attachment #320155 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [needs new patch] → [has patch][needs review dao]
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.
Attachment #320156 - Flags: ui-review?(faaborg) → ui-review+
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.
Attachment #320155 - Flags: review?(dao) → review-
(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.
Attached patch Patch (v1.4) (obsolete) — Splinter Review
Addressed the issues in comment 38.
Attachment #320155 - Attachment is obsolete: true
Attachment #320244 - Flags: review?(dao)
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.
Attachment #320244 - Flags: review?(dao) → review-
Attached patch Patch (v1.5)Splinter Review
OK, added back the focus ring.
Attachment #320244 - Attachment is obsolete: true
Attachment #320251 - Flags: review?(dao)
Comment on attachment 320251 [details] [diff] [review]
Patch (v1.5)

>+      infoBoxExpanderLabel.setAttribute("accesskey", infoBoxExpanderLabel.getAttribute("lessaccesskey"));

could also use .accessKey here
Attachment #320251 - Flags: review?(dao) → review+
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.
Attachment #320251 - Flags: approval1.9?
Comment on attachment 320148 [details]
new expand.png

This needs to land with this bug as well.
Attachment #320148 - Flags: approval1.9?
Comment on attachment 320149 [details]
New collapse.png

This one too.
Attachment #320149 - Flags: approval1.9?
(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...
Whiteboard: [has patch][needs review dao] → [has patch][has review][needs approval]
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.
Attachment #320251 - Flags: approval1.9? → approval1.9-
Attachment #320148 - Flags: approval1.9? → approval1.9-
Attachment #320149 - Flags: approval1.9? → approval1.9-
Flags: wanted1.9.0.x?
Whiteboard: [has patch][has review][needs approval] → [has patch][has review][needs approval][RC2?]
Whiteboard: [has patch][has review][needs approval][RC2?] → [has patch][has review][needs approval][RC2-]
This can be pushed on mozilla-central now.
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs approval][RC2-] → [has patch][has review][RC2-]
Comment on attachment 320251 [details] [diff] [review]
Patch (v1.5)

This theme only change could be considered for 3.0.1.
Attachment #320251 - Flags: approval1.9.0.1?
Attachment #320251 - Flags: approval1.9.0.1? → approval1.9.0.2?
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1a1
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.
Status: RESOLVED → VERIFIED
Comment on attachment 320251 [details] [diff] [review]
Patch (v1.5)

Moving approval request out to 1.9.0.3.
Attachment #320251 - Flags: approval1.9.0.2? → approval1.9.0.3?
Blocks: 403151
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.
Flags: wanted1.9.0.x?
Attachment #320251 - Flags: approval1.9.0.4? → approval1.9.0.4-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: