Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use new expand and collapse progressive disclosure controls

VERIFIED FIXED in Firefox 3.1a1

Status

()

Firefox
Theme
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: faaborg, Assigned: Ehsan)

Tracking

Trunk
Firefox 3.1a1
All
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 10 obsolete attachments)

23.83 KB, image/png
Details
240 bytes, image/png
Mike Schroepfer
: approval1.9-
Details
238 bytes, image/png
Mike Schroepfer
: approval1.9-
Details
48.30 KB, image/png
faaborg
: ui-review+
Details
8.79 KB, patch
dao
: review+
Mike Schroepfer
: approval1.9-
Samuel Sidler (old account; do not CC)
: 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
Created attachment 317866 [details] [diff] [review]
Patch (v1)

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

Comment 3

9 years ago
>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.
(Reporter)

Comment 4

9 years ago
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)

Comment 6

9 years ago
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)
Created attachment 317971 [details] [diff] [review]
Patch (v1.1)

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
Created attachment 320119 [details] [diff] [review]
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.  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)
Created attachment 320120 [details]
Screenshot of v1.2

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)
Created attachment 320121 [details] [diff] [review]
Patch (v2)

This version makes the button smaller.  A screenshot will follow.
Created attachment 320122 [details]
Screenshot of v2
Attachment #320122 - Flags: ui-review?(faaborg)
Created attachment 320123 [details] [diff] [review]
Patch (v3)

This patch uses round buttons for the progressive disclosure controls.
Created attachment 320124 [details]
Screenshot of v3

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.
Created attachment 320128 [details]
screenshot of windows dialog

(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

Comment 22

9 years ago
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...

Comment 26

9 years ago
(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).
Created attachment 320148 [details]
new expand.png
Created attachment 320149 [details]
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-

Updated

9 years ago
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]
Created attachment 320155 [details] [diff] [review]
Patch (v1.3)

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)
Created attachment 320156 [details]
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.
Created attachment 320244 [details] [diff] [review]
Patch (v1.4)

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-
Created attachment 320251 [details] [diff] [review]
Patch (v1.5)

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 50

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

Updated

9 years ago
Attachment #320148 - Flags: approval1.9? → approval1.9-

Updated

9 years ago
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?]

Updated

9 years ago
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
Last Resolved: 9 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?

Updated

9 years ago
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.