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

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
P4
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: faaborg, Assigned: ddahl)

Tracking

({polish, verified1.9.1})

Trunk
Firefox 3.6a1
polish, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
blocking-firefox3.5 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy][polish-visual][polish-p3])

Attachments

(2 attachments, 10 obsolete attachments)

40.23 KB, image/png
faaborg
: ui-review+
Details
8.46 KB, patch
mak
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
Flags: blocking-firefox3?

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
Not going to continue to block on this, please renominate with reasons why we can't ship with this in final
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Target Milestone: Firefox 3 beta3 → ---
(Reporter)

Updated

10 years ago
Blocks: 425582
Flags: blocking-firefox3.1?
Again, I'd like to see this fixed in 3.1, but it's not a release blocker.
Flags: blocking-firefox3.1? → blocking-firefox3.1-

Comment 3

10 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Depends on: 430902
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 5

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

Comment 7

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

Updated

10 years ago
Whiteboard: [polish-easy][polish-visual]
(Reporter)

Comment 12

10 years ago
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
Keywords: uiwanted
(Reporter)

Comment 13

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

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

Comment 14

10 years ago
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.
OS: All → Linux
(Reporter)

Updated

10 years ago
Keywords: uiwanted

Comment 15

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

10 years ago
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.
(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?
Status: REOPENED → NEW
(Assignee)

Updated

10 years ago
Assignee: nobody → ddahl
(Assignee)

Comment 18

10 years ago
So should this be blocked on using the actual GTK widget?
(Assignee)

Comment 19

10 years ago
Created attachment 356624 [details]
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?
Attachment #356624 - Flags: review?(faaborg)
(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.
(Reporter)

Comment 21

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

Updated

10 years ago
Attachment #356624 - Flags: review?(faaborg)
OS: Linux → All
(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.
(Assignee)

Comment 23

10 years ago
Created attachment 357056 [details]
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.
Attachment #356624 - Attachment is obsolete: true
Attachment #357056 - Flags: review?(faaborg)
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.
(Assignee)

Comment 25

10 years ago
No worries. My bad on not hg updating. I think I still have some stuff to contribute for this on linux at least.
(Reporter)

Updated

10 years ago
Attachment #357056 - Flags: review?(faaborg) → ui-review+
(Assignee)

Comment 26

10 years ago
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;
+}
+
(Assignee)

Comment 27

10 years ago
Created attachment 357194 [details] [diff] [review]
patch submittal

Here is the patch, I did a hg pull --update and i left out the alignment margin:)
Attachment #357194 - Flags: review?(mak77)
(Assignee)

Comment 28

10 years ago
Created attachment 357196 [details]
new screenshot showing progressive disclsure for linux

this screenshot is the result of patch 357194
Attachment #357056 - Attachment is obsolete: true
Attachment #357196 - Flags: ui-review?(faaborg)
Attachment #357196 - Flags: review?(mak77)
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");
 }
(Assignee)

Comment 30

10 years ago
I know the code/css/ifdefs have some duplication, as I am iterating on this:)
(Reporter)

Updated

10 years ago
Attachment #357196 - Flags: ui-review?(faaborg) → ui-review+

Updated

10 years ago
Attachment #357196 - Flags: review?(mak77)

Updated

10 years ago
Keywords: uiwanted
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.

Updated

10 years ago
Keywords: uiwanted
What about hiding the label with css and not having ifdefs at all?
(Assignee)

Comment 35

10 years ago
Created attachment 357858 [details] [diff] [review]
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?
Attachment #357194 - Attachment is obsolete: true
Attachment #357858 - Flags: review?(mak77)
Attachment #357194 - Flags: review?(mak77)

Updated

10 years ago
Attachment #357858 - Flags: review?(mak77) → review-
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.
(Assignee)

Comment 37

10 years ago
Created attachment 357886 [details] [diff] [review]
testing with sdwilsh

just wanted to post to discuss with sdwilsh
notice you can also use http://pastebin.mozilla.org/ to share code with other devs.

Comment 39

10 years ago
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.
(Assignee)

Comment 40

10 years ago
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.
(Assignee)

Comment 41

10 years ago
Created attachment 358056 [details] [diff] [review]
whoops forgot to attach new patch
Attachment #357886 - Attachment is obsolete: true
(Assignee)

Comment 42

10 years ago
Created attachment 358063 [details] [diff] [review]
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
Attachment #358056 - Attachment is obsolete: true
Attachment #358063 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #358063 - Flags: review? → review?(sdwilsh)
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
(Assignee)

Comment 44

10 years ago
Created attachment 358080 [details] [diff] [review]
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...
Attachment #358063 - Attachment is obsolete: true
Attachment #358080 - Flags: review?
Attachment #358063 - Flags: review?(sdwilsh)
(Assignee)

Comment 45

10 years ago
Created attachment 358087 [details] [diff] [review]
hopefully fixes mac expander class set error
Attachment #358087 - Flags: review?(sdwilsh)
(Assignee)

Updated

10 years ago
Attachment #358080 - Attachment is obsolete: true
Attachment #358080 - Flags: review?
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.
Attachment #358087 - Flags: review?(sdwilsh) → review?(mak77)
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 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
Attachment #358087 - Flags: review?(mak77) → review-
and remove margin: 0; if that's no more useful after setting align="center"
(Assignee)

Comment 50

10 years ago
Created attachment 358207 [details] [diff] [review]
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.
Attachment #357858 - Attachment is obsolete: true
Attachment #358087 - Attachment is obsolete: true
Attachment #358207 - Flags: review?(mak77)
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 ":"
(Assignee)

Comment 52

10 years ago
Created attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

fixed css, added align="center" to xul. does "-moz-box-align: center" do the same thing?
Attachment #358207 - Attachment is obsolete: true
Attachment #358213 - Flags: review?(mak77)
Attachment #358207 - Flags: review?(mak77)
(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).

Updated

10 years ago
Attachment #358213 - Flags: review?(mak77) → review+
Comment on attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

Looks good, thanks!

Comment 55

10 years ago
yay! do we need a super-review, and who should I tap?
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.
http://hg.mozilla.org/mozilla-central/rev/b880c2cb569d
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
(Assignee)

Updated

10 years ago
Attachment #358213 - Flags: approval1.9.1?
(Assignee)

Comment 58

10 years ago
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.
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!
Status: RESOLVED → VERIFIED
Attachment #358213 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 358213 [details] [diff] [review]
new patch to supercede old one. alright.

a191=beltzner

Updated

10 years ago
Keywords: checkin-needed
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
Keywords: fixed1.9.1 → verified1.9.1

Comment 63

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

This caused bug 475798.
Depends on: 475798

Updated

9 years ago
Duplicate of this bug: 411981
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.
Whiteboard: [polish-easy][polish-visual] → [polish-easy][polish-visual][polish-p3]
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.