252126 - bookmarks separators need a minimum width in bookmark menu.

Status: NEW

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

Description

[Asa Dotzler [:asa]]

Bookmark separators need a minimum width of at least several ems and need to
show up in bookmark folders on the bookmarks menu even when they are the only
content in the folder. The current Windows behavior when adding a separator to a
previously "Empty" folder on the bookmarks menu is that the "Empty" is gone and
the new sub-menu shrinks down to nothing but its padding. This is undesirable.

Comment 1

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 2

[Marco Bonardo [:mak]]

still valid
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081215 Minefield/3.2a1pre

Comment 4

[Phil Ringnalda (:philor)]

Windows actually has it the best: both in the Bookmarks menu and on the toolbar, there's enough room in the tiny popup to right click, while Linux has just a 1px wide line, and on the toolbar Mac has nothing at all.

Comment 5

[Marco Bonardo [:mak]]

or alternatively, don't allow inserting a separator in an empty menupopup (what's the use-case?)

Comment 6

[Phil Ringnalda (:philor)]

Mine was a folder of bugs that I need to check in for timeless: things that I haven't pushed to the try server above the separator, things I have and that are ready to go below. Pretending that the non-empty folder is (Empty) because it only has a separator would be less broken than we are now, but if I drag into it, will my drop land above or below the separator?

Comment 7

[Marco Bonardo [:mak]]

right, your use-case is valid, i was only thinking to a folder where i initially want to add a separator, not to a folder that, after removing all entries, is only made up of a separator.

Comment 8

[Adam]

Attachment: Updates the places.css to give min-width to places menuseparators

This adds 7.2em min-width to the menus with only the separator.  It also adds a little padding (0.65em on top and bottom) if it's an :only-child.

The values used were to match the size of the separators-only menu to give the approximate size of the "(empty)" menu.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Adam, is there any more that needs to be done here? Can this patch be updated and sent for review?

Comment 10

[Adam]

Attachment: Fix only-child padding

This needed a small tweak: the |:only-child| pseudo-selector no longer works for adding some top/bottom padding, because there are a couple of hidden |menuseparator|s in that case now (added by bug 731563).

This uses |:nth-child(2):nth-last-child(2):nth-of-type(2):nth-last-of-type(2)|, which is ugly.  But AFAICT the only way to ensure that we only match the situation where a popup contains just one real menuseparator.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 725667 [details] [diff] [review]
Fix only-child padding

Review of attachment 725667 [details] [diff] [review]:
-----------------------------------------------------------------

Did you test drag and drop above/below the separator still works properly?

Btw, the padding rule is too complex to take it as is (also cause I fear D&D problems due to wrong calculation of the drop indicator position), could you please post a screenshot with just the min-width rule?
Would adding a min-height on all separators help? Or adding min-height to the popup.
Alternatively you could maybe use adjacent to check if it's a separator adjacent to start and end markers (adding a "places-popup-marker" class to the hidden separators (http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#785)

Comment 12

[Adam]

Attachment: Examples of some positions of separators

DND works properly with the previous patch, but I agree a simpler selector is needed.  

|min-height| directly on menuseparator doesn't have an effect.  They have no height, and their appearance is specified as a combination of a top and bottom border (at least on Linux, http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/menu.css#178 ).

The image is on Linux.  The image shows (left column first):

1. min-width only
2. min-height on the menupopup (1em), which doesn't get the drawn separator centered
3. original patch and the version adding classes to hidden separators (padding: 0.65em 0)
4. (top right) as [3] using classed hidden separators, but with a bookmark after the only real separator
5. Same as [4] but order swapped

We can't do previous-sibling with classes and the adjacent sibling selector.  The only pure-CSS hope I know of for that is CSS level 4 'subject selectors' (marking the subject of a selector by prepending '!') which isn't coming soon.

We could still do something like:

> .places-popup-marker + menuseparator:nth-last-of-type(2):nth-last-child(2)

A closer-to-ideal is probably to split the padding over two rules:

> .places-popup-marker + menuseparator { padding-top:...}
> menuseparator:nth-last-of-type(2):nth-last-child(2) { padding-bottom:... }

This would add extra space in the cases like [4] and [5] in the image, but without overpadding like in [4].  That second selector is still ugly, though.