Closed Bug 403140 Opened 12 years ago Closed 12 years ago

Splitters in the bookmarks organizer should not be collapsible / auto size height of details pane

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: faaborg, Assigned: mak)

References

Details

(Keywords: polish)

Attachments

(4 files, 6 obsolete files)

The splitters in the bookmarks organizer should not be collapsable (clicking on a splitter causes the window pane to minimize to the side of the window).  The splitters should not contain any type of visual affordance for gripping, for visual integration with the operating system (the purple seems curiously similar to Java Swing).

See bug# 393514 for a mockup.
Additional rationale:

-this isn't native to any platform (I'm pretty sure)
-it is easy to accidently collapse them when trying to drag them
-the purple grippy appearance is java-swing-style ugly
Flags: blocking-firefox3?
These 1990s era UI elements has gots to go.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Attached image screenshot
something like this?
i've done some experimental change:
- fixed the back-forward buttons (hover, pressed state, padding)
- removed the grippy
- added min/max width values to panes
- changed the splitter to a single line
- set an initial height of 140px for details pane, enough for not having scrollbars with the more pane closed (making it enough for the details open is too much, takes about half of the window)
Attached patch experimental changes (obsolete) — Splinter Review
like above, plus light blue splitters like in Alex mockup (is this for vista only or for xp too?)... i could also add the translucent background to tree column headers if you want to try that
Assignee: nobody → mak77
>light blue splitters like in Alex mockup (is this for vista
>only or for xp too?)

We should only hard code the color on Vista (and only if we are going to also hard code the window background color).
Attached patch patch (obsolete) — Splinter Review
cleaned up a bit
- fixed back-forward buttons
- toolbar has min-height 30px, Vista toolbars are 30px
- removed the grippy
- added min/max width values to panes
- changed the splitter to a single line
- set an initial details panel height of 150px
Attachment #310975 - Attachment is obsolete: true
Attachment #312047 - Flags: review?(dietrich)
Attached image screenshot on Vista
appearance on Vista after removal of grippy and fixed back-forward
Attachment #312048 - Flags: ui-review?(beltzner)
Status: NEW → ASSIGNED
Comment on attachment 312047 [details] [diff] [review]
patch

i can probably fix the datails pane to adapt height based on contents
Attachment #312047 - Flags: review?(dietrich)
Whiteboard: [has patch][needs ui-r beltzner]
Beltzner, if i change the details pane to dynamically change its height based on content then persisting the user height makes no sense, on each select the panel height could change, on the other way if i persist the user height i cannot change the panel height and we have scrollbars. What is the correct behaviour?
We should probably spin off other changes to separate bugs, or adjust the summary of this bug (although I'm reluctant to do that since it is set as blocking).

>if i change the details pane to dynamically change its height based
>on content then persisting the user height makes no sense, on each select the
>panel height could change

On irc I instructed marco that we want to try to avoid ever having scroll bars, and as a secondary goal try not to have too much extra wasted space (for instance the area should get smaller when you hit "less".  Making the details area not user resizable (so it doesn't keep changing on them) is fine.
Attachment #312048 - Flags: ui-review?(beltzner)
Attached patch patch (obsolete) — Splinter Review
Alex, this is for your testing. I've done the changes we talked about yesterday, details pane is not resizable, but its height is calculated when the user select an item, deselect everything, or click on More/Less expander.
Plus previous fixes
Attachment #312047 - Attachment is obsolete: true
Attachment #312236 - Flags: ui-review?(faaborg)
Whiteboard: [has patch][needs ui-r beltzner] → [has patch][needs ui-r faaborg]
Comment on attachment 312236 [details] [diff] [review]
patch

Personally I'm in favor of the change, but switching the ui-r to beltzner for his feedback.
Attachment #312236 - Flags: ui-review?(faaborg) → ui-review?(beltzner)
Blocks: 425582
Whiteboard: [has patch][needs ui-r faaborg] → [has patch][needs ui-r beltzner]
Blocks: 405605
Comment on attachment 312236 [details] [diff] [review]
patch

I think we should be styling the splitter consistently with the browser sidebar splitter in the default css; to style that splitter appropriately for Vista we should use bug 403138

uir+ with that nit addressed
Attachment #312236 - Flags: ui-review?(beltzner) → ui-review+
Whiteboard: [has patch][needs ui-r beltzner] → [has patch][needs review ?]
Attached patch patch (obsolete) — Splinter Review
Reverted splitter style as requested.

fixed here:
- removed grippy from splitters
- winstripe left splitter styled like sidebar splitter
- winstripe back-forward buttons have correct styling now
- panels are not collapsable (added max/min sizes)
- details pane not resizable (auto-size based on contents and less/more)
Attachment #312236 - Attachment is obsolete: true
Attachment #314212 - Flags: review?(mano)
Whiteboard: [has patch][needs review ?] → [has patch][needs review mano]
Attached image screenshot on XP
Attached patch patch (obsolete) — Splinter Review
:\ the correct one
Attachment #314212 - Attachment is obsolete: true
Attachment #314286 - Flags: review?(mano)
Attachment #314212 - Flags: review?(mano)
Attachment #314286 - Attachment is obsolete: true
Attachment #314286 - Flags: review?(mano)
Attachment #314286 - Attachment is obsolete: false
Attachment #314286 - Flags: review?(mano)
I think we could just remove the scrollbox and don't set any heights.
Attached patch patchSplinter Review
scrollbox gone
Attachment #314286 - Attachment is obsolete: true
Attachment #314448 - Flags: review?(mano)
Attachment #314286 - Flags: review?(mano)
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch][has review]
Checking in browser/components/places/content/organizer.css;
/cvsroot/mozilla/browser/components/places/content/organizer.css,v  <--  organizer.css
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.154; previous revision: 1.153
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.129; previous revision: 1.128
done
Checking in browser/themes/gnomestripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.14; previous revision: 1.13
done
Checking in browser/themes/pinstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.11; previous revision: 1.10
done
Checking in browser/themes/winstripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.9; previous revision: 1.8
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
about the infoPane, the height could still be set up in the localstore.rdf for it, it should shrink when nothing is selected to leave some more space, but if the height is set up there it does not shrink.

Should we change the infoPane id or annotate this for beta users?
Depends on: 428020
filled bug 428020 on that
(In reply to comment #22)
> about the infoPane, the height could still be set up in the localstore.rdf for
> it, it should shrink when nothing is selected to leave some more space, but if
> the height is set up there it does not shrink.

Thanks for the tip, it improves the experience quite a bit!
(In reply to comment #22)
> about the infoPane, the height could still be set up in the localstore.rdf for
> it, it should shrink when nothing is selected to leave some more space, but if
> the height is set up there it does not shrink.

So the user doesn't have control about the height in any way? I'm not able to move the horizontal splitter. Is this expected?
Hardware: PC → All
Whiteboard: [has patch][has review]
(In reply to comment #25)
> So the user doesn't have control about the height in any way? I'm not able to
> move the horizontal splitter. Is this expected?

Yes, the pane takes height of its contents (apart bug 428020 problem)

Attached image Too tall and not sizable details pane (obsolete) —
I end-up in such a sectioning of the bookmarks list and the details pane. There is no way to shrink the size of the lower pane.
Comment on attachment 314718 [details]
Too tall and not sizable details pane

Sorry, I hit bug 428020. Removing localstore.rdf solves it for the current profile.
Attachment #314718 - Attachment is obsolete: true
(In reply to comment #28)
> (From update of attachment 314718 [details])
> Sorry, I hit bug 428020. Removing localstore.rdf solves it for the current
> profile.

yes please annotate on that bug and post the same screenshot

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040907 Minefield/3.0pre ID:2008040907
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Summary: Splitters in the bookmarks organizer should not be collapsable → Splitters in the bookmarks organizer should not be collapsible / auto size height of details pane
Depends on: 431639
(In reply to comment #14)
> Created an attachment (id=314212) [details]
> patch
> 
> Reverted splitter style as requested.
> 
> fixed here:
[..]
> - winstripe left splitter styled like sidebar splitter

This didn't really work. See bug 431639.
not really an item that's needed to be checked in litmus
Flags: in-litmus? → in-litmus-
Depends on: 433114
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.