403151 - In the bookmarks organizer preview pane change "more" from a button to a progressive disclosure control

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

In the bookmarks organizer preview pane change "more" from a button to a progressive disclosure control.  An image for the control is currently in the icon inventory.

See bug# 393514 for a mockup.

Comment 1

[Mike Connor [:mconnor]]

Not going to continue to block on this, please renominate with reasons why we can't ship with this in final

Comment 2

[Dietrich Ayala (:dietrich)]

Again, I'd like to see this fixed in 3.1, but it's not a release blocker.

Comment 3

[Marco Bonardo [:mak]]

i think this has been fixed with Bug 430902, with changeset http://hg.mozilla.org/mozilla-central/rev/37f0a75e3b2a, i see the progressive disclosure control in both XP and Vista, the only difference is that it's a square, but all this kind of items are in the application.
correct me if i'm wrong.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, what about OS X? Do we have it on its own bug? It's still there with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081002 Minefield/3.1b1pre ID:20081002020319

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> Marco, what about OS X? Do we have it on its own bug? It's still there with
> Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre)
> Gecko/20081002 Minefield/3.1b1pre ID:20081002020319

i thought it was by design since this bug was windows only and the global change was the same a windows only, you should ask Alex if that's a wanted change on os X too.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex, can you please give a comment? Probably I'm not able to find the OS X related bug but as what I see the change should also happen on OS X. We still have the disclosure control behind the tag field so why not for the more/less button. If there is another bug we could close this.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'm not able to find the OS X
>related bug but as what I see the change should also happen on OS X. We still
>have the disclosure control behind the tag field so why not for the more/less
>button.

I'm not 100% sure about the proper use of progressive disclosure controls on OS X (so anyone totally feel free to correct me), but let's use the same image that we currently have for the expanding the tags area, which is meant to appear similar to the progressive disclosure control in the native save as dialog.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex, I hope it's ok to morph the bug to the remaining issue. Probably it should be simple to get this fixed on OS X.

Currently we are using following twisties:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/twisty-open.gif
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/twisty-closed.gif

They could be replaced by:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/expander-open.png
http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/expander-closed.png

Alex, something you would prefer?

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>They could be replaced by:
>http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/expander-open.png
>http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/places/expander-closed.png

Yes, on OS X. For Windows, we have these, which should be placed on an actual button:

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/collapse.png
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/expand.png

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> Yes, on OS X. For Windows, we have these, which should be placed on an actual
> button:
> 
> http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/collapse.png
> http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/expand.png

This part has been fixed on Windows which was the work in bug 430902. So how does it look like for Linux? Has there also work to be done or is it only left for OS X now?

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> This part has been fixed on Windows which was the work in bug 430902. So how
> does it look like for Linux? Has there also work to be done or is it only left
> for OS X now?

bug 430902 has implemented the progressive disclosure with ifdef XP_WIN su linux and OS X most likely are using the old way

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

On windows we should have a button progressive disclosure control, using the same icons as the star panel, next to a label that says "details"

uiwanted: check the correct term to use against windows HIGs

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

>uiwanted: check the correct term to use against windows HIGs

There isn't really a standard terminology, what we have is fine.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

This just needs to be fixed on linux now, although I'm honestly not sure if this is the correct way to represent progressive disclosure controls there, I couldn't find any reference to this type of control in the Gnome HIG.  Cc'ing some linux folks.

Comment 15

[Michael Ventnor]

We have support for the native GTK expander which is definitely the most suitable thing here. We just need to hook it up to nsITheme and use it.

Comment 16

[Michael Ventnor]

Oh, in the bookmarks dialog the control to expand things is _under_ the widgets it hides instead of on top. That makes things difficult, the GTK expander assumes the latter.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #14)
> This just needs to be fixed on linux now, although I'm honestly not sure if
> this is the correct way to represent progressive disclosure controls there, I
> couldn't find any reference to this type of control in the Gnome HIG.  Cc'ing
> some linux folks.

Alex, what about comment 8 and comment 9? We also need a fix on OS X. Even its probably not a disclosure control. Shall I file a new bug on that?

Comment 18

[David Dahl :ddahl]

So should this be blocked on using the actual GTK widget?

Comment 19

[David Dahl :ddahl]

Attachment: screen grab shows that there appears to be an existing progressive disclosure...

screen grab shows that there appears to be an existing progressive disclosure for the tagging interface. Should both of these be the same?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #14)
> This just needs to be fixed on linux now, although I'm honestly not sure if

But what happens on OS X? There is still no such control in use. The more/less button differs from the tag expand/collapse button.

Comment 21

[Alex Faaborg [:faaborg] (Firefox UX)]

Sorry about all the confusion.

For all three platforms we are talking about the button at the bottom of the preference pane, which contains the text "more" or "less" and when expanded displays the keyword, description, and the option to load the bookmark in a sidebar.

Here is what we should do for each platform:

OS X: button with an arrow on it, similar to what we current have next to the tags field.  We might want to update this image for leopard, but we can do that in a follow up bug.

Windows: Normal button, that says "more" or "less", but with a progressive disclosure icon:

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/expand.png
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/collapse.png

Linux: Same as windows, but with an icon uplifted from the OS theme.  We can switch to a native progressive disclosure control in a follow up bug.

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #21)
> Windows: Normal button, that says "more" or "less", but with a progressive
> disclosure icon:

as said Windows has already been fixed in bug 430902 with an ifdef in the code, so removing the ifdef and fixing the code for remaining OSes could work.

Comment 23

[David Dahl :ddahl]

Attachment: before and after of bookmarks tagging interface.

I made the linux version (pretty much) match the UI of the Windows version, (my screens are KDE 4.1 - perhaps I should also look at Gnome).

I also added a tweak to the width (via margin) of the tag selector/deselector listbox. It seems too wide (on all platforms). what is the proper unit of measurement to use in the css? I used px just for the time being.

Comment 24

[Marco Bonardo [:mak]]

david, some change has landed on mozilla-central with bug 413053 yesterday, sorry for double work, but check current m-c version since the dialog now has a different alignment.

Comment 25

[David Dahl :ddahl]

No worries. My bad on not hg updating. I think I still have some stuff to contribute for this on linux at least.

Comment 26

[David Dahl :ddahl]

I think the only place your patch touches that I have also worked on is here:

/browser/themes/gnomestripe/browser/browser.css

And you only added 4 lines there: 
+/* Implements editBookmarkPanel resizing on folderTree un-collapse. */
+#editBMPanel_folderTree {
+  min-width: 300px;
+}
+

Comment 27

[David Dahl :ddahl]

Attachment: patch submittal

Here is the patch, I did a hg pull --update and i left out the alignment margin:)

Comment 28

[David Dahl :ddahl]

Attachment: new screenshot showing progressive disclsure for linux

this screenshot is the result of patch 357194

Comment 29

[Marco Bonardo [:mak]]

Faaborg, i don't completely get comment 21 meanings.

> OS X: button with an arrow on it, similar to what we current have next to the
> tags field. 

so i think: [^] without any label

> Windows: Normal button, that says "more" or "less", but with a progressive
> disclosure icon.

what do you mean for "Normal" button?
actually we have: [^] Less
from your comment i think: [^ Less]

but the button was changed not much time ago with request to be the former...

> Linux: Same as windows, but with an icon uplifted from the OS theme. 

so i get the same as above: [^ Less]

This sounds opposite to what this bug requested initially and to the screen you gave ui-r+, so i probably misunderstand you. Can you clarify the design please? Since if we want the progressive disclosure control like "[^] Less" on all platform we can probably get rid of some ifdef here.

David, this change appear unrelated:

 /* Star button */
 #star-button {
-  padding: 1px;
+  padding: 1px 4px 1px 1px;
   list-style-image: url("chrome://browser/skin/places/starPage.png");
 }

Comment 30

[David Dahl :ddahl]

I know the code/css/ifdefs have some duplication, as I am iterating on this:)

Comment 31

[Marco Bonardo [:mak]]

David, so i think we can simplify this a lot, your fix on Linux appear good, now on Mac we want practically the same thing, a button like [^] the same near tags selector, but on Mac we don't want any label near it. So i think we can probably ifdef out only the label and try to reduce code duplication between the three platforms. could you try that? eventually i can generate trybuilds for you.

Comment 32

[Marco Bonardo [:mak]]

faaborg pointed out this is the button we want on Mac
http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/places/expander-closed.png

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

You mostly all of those expander-...png buttons:
http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/places/

Comment 34

[Shawn Wilsher :sdwilsh]

What about hiding the label with css and not having ifdefs at all?

Comment 35

[David Dahl :ddahl]

Attachment: simplified patch unifying everything except the label on Win32

this tightens it up a bit. Perhaps sdwilsh's css fix concept is good as well?

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 357858 [details] [diff] [review]
simplified patch unifying everything except the label on Win32

Sdwilsh suggestion is good if we can avoid an ifdef, since makes the code common, delegating the change to the theme. If you want to try that it's a welcome change. Clearly we should already have all the needed strings (and i suppose we have them) on Mac too.

Apart that, do you have access to a Mac machine to try the changes you make?


>-#ifdef XP_WIN
>+
>                     <button type="image" id="infoBoxExpander"
>                             class="expander-down"
>                             oncommand="PlacesOrganizer.toggleAdditionalInfoFields();"
>                             observes="paneElementsBroadcaster"/>
>+#ifndef XP_MACOSX
>                     <label id="infoBoxExpanderLabel"
>                            lesslabel="&detailsPane.less.label;"
>                            lessaccesskey="&detailsPane.less.accesskey;"
>                            morelabel="&detailsPane.more.label;"
>                            moreaccesskey="&detailsPane.more.accesskey;"
>                            value="&detailsPane.more.label;"
>                            accesskey="&detailsPane.more.accesskey;"
>                            control="infoBoxExpander"/>
>-#else
>-                    <button type="image" id="infoBoxExpander"
>+#endif
>+
>+#ifdef XP_WINXP
>+		    <button type="image" id="infoBoxExpander"

XP_WINXP does not exists, and this button is a repetition.

Comment 37

[David Dahl :ddahl]

Attachment: testing with sdwilsh

just wanted to post to discuss with sdwilsh

Comment 38

[Marco Bonardo [:mak]]

notice you can also use http://pastebin.mozilla.org/ to share code with other devs.

Comment 39

[David Dahl]

Yeah - pastebin would have been better. I will have to go back through the javascript and all 3 platform css to see what is really going on. They are all markedly different and could use some unifying.

Comment 40

[David Dahl :ddahl]

Removed all ifdefs in XUL, unified the Win and Linux css (mostly). How does this approach look?

I am posting a link to pastebin to test build on Mac and Win.

Comment 41

[David Dahl :ddahl]

Attachment: whoops forgot to attach new patch

Comment 42

[David Dahl :ddahl]

Attachment: unifying xul for all platforms - needs a build on macos

I unified the css between the lin and win and have minimal changes for the mac os version. all xul ifdefs are gone now. needs build on mac os

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 358063 [details] [diff] [review]
unifying xul for all platforms - needs a build on macos

some fly-by comment, but wait for Shawn before working on a new version

>-#ifdef XP_WIN
>+#ifndef XP_MACOSX
>     var infoBoxExpanderLabel = document.getElementById("infoBoxExpanderLabel");

infoBoxExpanderLabel exists on mac too with this patch, there's probably no need to ifdef all the code here, only pay attention to changes to hidden if needed

>diff --git a/browser/themes/gnomestripe/browser/places/organizer.css b/browser/themes/gnomestripe/browser/places/organizer.css
>--- a/browser/themes/gnomestripe/browser/places/organizer.css
>+++ b/browser/themes/gnomestripe/browser/places/organizer.css
>@@ -177,22 +177,46 @@
>   display: none;
> }
> 
> .advancedSearchMinus .button-icon,
> .advancedSearchPlus .button-icon {
>   margin: 0 !important;
> }
> 
>+%endif
>+
> /**** expanders ****/
> .expander-up,
> .expander-down {
>   min-width: 0;
> }
>-%endif
>+
>+/* copy from winstripe */
>+
>+.expander-up > hbox,
>+.expander-down > hbox {
>+  padding: 0;
>+}
>+
>+.expander-up {
>+  list-style-image: url("chrome://global/skin/icons/collapse.png");
>+}
>+
>+.expander-down {
>+  list-style-image: url("chrome://global/skin/icons/expand.png");
>+}

these are also defined in editBookmarkOverlay.css, i guess if we can remove them really

>+#infoBoxExpanderLabel {
>+  margin: 0;
>+  padding: 5px 0;
>+  -moz-padding-start: 2px;
>+}
>+
>+/* end copy */
> 

no need for "copy" comments i think

Comment 44

[David Dahl :ddahl]

Attachment: one more time...

added display:none for the "more label" for the pinstripe theme, checking into the expander-up/expander-down class selectors - and what else they may be used by, etc...

Comment 45

[David Dahl :ddahl]

Attachment: hopefully fixes mac expander class set error

Comment 46

[Shawn Wilsher :sdwilsh]

Comment on attachment 358087 [details] [diff] [review]
hopefully fixes mac expander class set error

This works as spec'd by alex on the mac now.  You'll need a different peer to review this though.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 358087 [details] [diff] [review]
hopefully fixes mac expander class set error

>+#infoBoxExpanderLabel {
>+  margin: 0;
>+  padding: 5px 0;

You're doing this to center the label, right? We should probably do this by adding align="center" to the parent hbox.

Comment 48

[Marco Bonardo [:mak]]

Comment on attachment 358087 [details] [diff] [review]
hopefully fixes mac expander class set error

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -592,16 +592,17 @@ var PlacesOrganizer = {
>      * 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

You should get rid of all ifdefs here, practically what you should do is setting an id in the hbox that is containing the button and the label, then here you get that element, and below you set hidden on it instead of setting it on both the button and the label.
So we get rid of all remaining ifdefs.

>diff --git a/browser/themes/gnomestripe/browser/places/organizer.css b/browser/themes/gnomestripe/browser/places/organizer.css
>--- a/browser/themes/gnomestripe/browser/places/organizer.css
>+++ b/browser/themes/gnomestripe/browser/places/organizer.css
> /**** expanders ****/
> .expander-up,
> .expander-down {
>   min-width: 0;
> }
>-%endif
>+
>+.expander-up > hbox,
>+.expander-down > hbox {
>+  padding: 0;
>+}
>+
>+.expander-up {
>+  list-style-image: url("chrome://global/skin/icons/collapse.png");
>+}
>+
>+.expander-down {
>+  list-style-image: url("chrome://global/skin/icons/expand.png");
>+}

remove all expander-up and expander-down rules from organizer.css in both winstripe and gnomestripe, since they are already included in editBookmarkOverlay.css and should not be useful

>+
>+#infoBoxExpanderLabel {
>+  margin: 0;
>+  padding: 5px 0;
>+  -moz-padding-start: 2px;
>+}

use "center" in the surrounding hbox as suggested by dao, and remove the top/bottom padding from both winstripe and gnomestripe

Comment 49

[Marco Bonardo [:mak]]

and remove margin: 0; if that's no more useful after setting align="center"

Comment 50

[David Dahl :ddahl]

Attachment: added infoBoxExpanderWrapper id to parent hbox, removed extraeous css

added infoBoxExpanderWrapper id to parent hbox, removed extraneous css from gnome and winstripe, removed all #ifdefs, yay! thanks Dao and Mak77.

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 358207 [details] [diff] [review]
added infoBoxExpanderWrapper id to parent hbox, removed extraeous css

>+#infoBoxWrapper {
>+  align: center;
> }

There's no CSS property called "align" (but "-moz-box-align"). The idea was to add align="center" in the XUL file.

"infoBoxWrapper" also seems to be the wrong id.

>+#infoBoxExpanderLabel {
>+  display:none;
>+}

nit: space after ":"

Comment 52

[David Dahl :ddahl]

Attachment: new patch to supercede old one. alright.

fixed css, added align="center" to xul. does "-moz-box-align: center" do the same thing?

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #52)
> does "-moz-box-align: center" do the same thing?

It does, but it's better to use align="center" in this case, as it fixes it for all themes (including third-party themes).

Comment 54

[Marco Bonardo [:mak]]

Comment on attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

Looks good, thanks!

Comment 55

[David Dahl]

yay! do we need a super-review, and who should I tap?

Comment 56

[Marco Bonardo [:mak]]

no you don't need any super-review, we are inside Places Code, so a Places peer/subpeer review is enough (owner/peers mostly for api changes). You would need a browser peer review if you would be touching code outside of Places, and a super review if your code is changing platform or doing big changes to some common behavior in the mozilla code.

Comment 57

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b880c2cb569d

Comment 58

[David Dahl :ddahl]

Comment on attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

tested on linux, mac and windows. mainly an attempt to unify css and xul across platforms. killed off a number of #ifdefs and cleaned up mac and linux ui.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified too on trunk with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090124 Minefield/3.2a1pre ID:20090124020327

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre (.NET CLR 3.5.30729) ID:20090123033224

Looks great. Thanks David!

Comment 60

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

a191=beltzner

Comment 61

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/59d99ab5b571

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on 1.9.1 with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090125
Shiretoko/3.1b3pre (.NET CLR 3.5.30729) ID:20090125052901

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre)
Gecko/20090126 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090126020313

Comment 63

[Simon Bünzli]

(In reply to comment #58)
> killed off a number of #ifdefs

This caused bug 475798.

Comment 65

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

The bookmarks organizer is a secondary interface, and probably most users won't notice the change.

Comment 66

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv