938578 - Update separators style in the Developer and History subviews

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike de Boer [:mikedeboer]]

STR:

1) Open the MenuPanel
2) Select the History subview and check the separators' style there.
3) Select the Bookmarks subview (some customization is required to get it into the MenuPanel) and notice the difference.

ER:

The separators should all have the same visual appearance.

Comment 1

[u428464]

A difference between "large" and normal separators should also be introduced as seen on the subview mockup : http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html The "large" ones take the whole width.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: update separator styling in subviews and align related items accordingly

Things done in this patch:

1) Introduced a class `content-separator` for separators that are use in subview contents and are less wide.
2) Added one additional separator to the History subview
3) Separators using a custom style (like all of 'em in the Menu Panel!) wouldn't get the style applied because of https://hg.mozilla.org/integration/fx-team/rev/e88e1d0056a5, so the Linux theme needs an extra rule. Note that the Menu Panel footer buttons also didn't look good because of this.
4) Subview buttons that contain only text (no icon) were not aligned with the other items, so they appeared to be indented. I made sure that items that do NOT have an item or checkbox prefixed will be neatly aligned from now on.
5) Generalized the selectors for subview buttons that contain a checkmark, because add-ons.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8370789 [details] [diff] [review]
Patch v1: update separator styling in subviews and align related items accordingly

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

The vertical margins around the separators are not correct in this patch in any of the views, on my windows 7 aero build.

Also, the 'view history sidebar' and 'clear recent history' items don't line up with anything else in the history view. Nor do the view bookmarks sidebar, view bookmarks toolbar and edit this bookmark / bookmark this page item line up with the items without icons. So I'm not sure what you meant about the alignment... 

There should be a wide separator between the recently closed tabs and the history items, not a narrow one.

Is '.content-separator' meant for small separators? If so, can you please pick a more descriptive class name?

::: browser/components/places/content/browserPlacesViews.js
@@ +292,5 @@
>      let element;
>      let type = aPlacesNode.type;
>      if (type == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) {
>        element = document.createElement("menuseparator");
> +      element.setAttribute("class", "content-separator");

I'm confused. You added the other class conditionally. Won't this also affect separators in the main menu bar's bookmark menu?

::: browser/components/sessionstore/src/RecentlyClosedTabsAndWindowsMenuUtils.jsm
@@ +72,5 @@
>        restoreAllTabs.classList.add("restoreallitem");
>        restoreAllTabs.setAttribute("label", navigatorBundle.GetStringFromName(aRestoreAllLabel));
>        restoreAllTabs.setAttribute("oncommand",
>                "for (var i = 0; i < " + closedTabs.length + "; i++) undoCloseTab();");
> +      let separator = doc.createElementNS(kNSXUL, "menuseparator");

This hunk is bitrotted on current fx-team.

@@ +80,3 @@
>          fragment.appendChild(restoreAllTabs);
>        } else {
> +        fragment.insertBefore(separator, fragment.firstChild);

This is wrong. There shouldn't be an additional separator according to the design in attachment 8366251 [details] on bug 928843. That also makes the code in the history portion of CustomizableWidgets.jsm unnecessary. Not having the separator makes it much clearer what the items are.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +413,5 @@
>    transition-property: background-color, border-color;
>    transition-duration: 150ms;
>  }
>  
> +.subviewbutton:not(:-moz-any([image],[targetURI],#panelMenu_bookmarksToolbar,#panelMenu_unsortedBookmarks)) > .toolbarbutton-text {

hardcoding the bookmarks items here seems ugly. Instead, let's make the sidebar one type="checkbox" and use that.

Comment 5

[Mike de Boer [:mikedeboer]]

Stephen, Zhenshuo: Can I have a final answer on the following question:

"Should there be a narrow separator below the menu item 'Restore Closed Tabs' in the History subview: yes/ no?"

(It is present in this mockup: http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html, others say nay)

Thanks!

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

With the current architecture of the menu, I think it is better to have no separator.
If we manage to move menu items without icons all the way to the left (as it happens already in the bookmarks menu and also in the mockups), this should provide a pretty clear hierarchy.

By the way, I noticed that the separators in the history subview on Windows 8 are all narrow separators. They are full width on XP (same build 2014-02-09). We should use full-width separators on all platforms here.

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #4)
> > +.subviewbutton:not(:-moz-any([image],[targetURI],#panelMenu_bookmarksToolbar,#panelMenu_unsortedBookmarks)) > .toolbarbutton-text {
> 
> hardcoding the bookmarks items here seems ugly. Instead, let's make the
> sidebar one type="checkbox" and use that.

I think you mean 'make the subview one type="checkbox"? We shouldn't do that, because they don't function as checkboxes inside a subview; they just open the Library window. If we'd show a checkmark in front of it, it would

1) Look awkward with a checkmark + icon
2) The checkmark doesn't go away when the Library window is closed. We'd need to add additional code to track the closing of the window and navigating away from the opened view inside the Library window.

In all, I'm for keeping the IDs inside this selector OR add a class to these two buttons that can replace the ids with one class-name.

Comment 9

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to :Gijs Kruitbosch from comment #4)
> > > +.subviewbutton:not(:-moz-any([image],[targetURI],#panelMenu_bookmarksToolbar,#panelMenu_unsortedBookmarks)) > .toolbarbutton-text {
> > 
> > hardcoding the bookmarks items here seems ugly. Instead, let's make the
> > sidebar one type="checkbox" and use that.
> 
> I think you mean 'make the subview one type="checkbox"?

No, I just misread. Both of these items have an image in front, though, why doesn't the [image] selector catch them and/or what are you actually trying to do? :-)

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #9)
> No, I just misread. Both of these items have an image in front, though, why
> doesn't the [image] selector catch them and/or what are you actually trying
> to do? :-)

I want subview items without an icon to NOT have a start margin, so they will be aligned with the separators and other items as well.

Comment 11

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > No, I just misread. Both of these items have an image in front, though, why
> > doesn't the [image] selector catch them and/or what are you actually trying
> > to do? :-)
> 
> I want subview items without an icon to NOT have a start margin, so they
> will be aligned with the separators and other items as well.

Then maybe we should make the [image] selector catch these items by specifying their list-style-image using the attribute.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #11)
> Then maybe we should make the [image] selector catch these items by
> specifying their list-style-image using the attribute.

And what will happen with the HiDPI version of the icon?

Comment 13

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > Then maybe we should make the [image] selector catch these items by
> > specifying their list-style-image using the attribute.
> 
> And what will happen with the HiDPI version of the icon?

You could either override it in CSS or set it to the right thing from JS.
(the max-height/width should already be set generically in CSS, I should hope)

I guess you're implying this doesn't feel quite right either... I would tend to agree somewhat.

Why not set the margin to what it needs to be for items without an icon, on all items, and then set a margin on the icon so that the result matches what we want? That seems more straightforward, and more like what toolbarbuttons do already.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: update separator styling in subviews and align related items accordingly

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: update separator styling in subviews and align related items accordingly

forgot to remove something!

Comment 16

[:Gijs (he/him)]

Comment on attachment 8374908 [details] [diff] [review]
Patch v1.2: update separator styling in subviews and align related items accordingly

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

The vertical margins still don't seem to match the spec, at least not on OS X...

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +108,5 @@
>                         type="checkbox"
>                         toolbarId="PersonalToolbar"
>                         class="subviewbutton"
>                         oncommand="onViewToolbarCommand(event); PanelUI.hide();"/>
> +        <toolbarseparator class="narrow-separator"/>

You mean small-separator?

Comment 17

[:Gijs (he/him)]

(I'm also still worried about applying this class throughout the bookmarks stuff instead of making it conditional based on the container)

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: update separator styling in subviews and align related items accordingly

Comment 19

[:Gijs (he/him)]

Comment on attachment 8375422 [details] [diff] [review]
Patch v1.2: update separator styling in subviews and align related items accordingly

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

r=me, although it'd be good to nix the right inner border on subviews inside the main menu panel, because they make the result here still look slightly imbalanced horizontally. If you don't want to do that here, please file a followup bug for it.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +120,2 @@
>                         oncommand="PlacesCommandHook.showPlacesOrganizer('UnfiledBookmarks'); PanelUI.hide();"/>
>          <toolbarseparator/>

In the design, this is also a small-separator.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +579,5 @@
>    margin-bottom: 0;
>  }
>  
> +.PanelUI-subView toolbarseparator,
> +.PanelUI-subView menuseparator,

Nit: while I hope you don't find me too OCD, can you order these the same way in all these selectors? :-)

@@ +585,4 @@
>    -moz-appearance: none;
>    min-height: 0;
> +  border-top: 1px solid hsla(210,4%,10%,.15);
> +  margin: 2px 0;

But you override (the horizontal part) for everything but .cui-widget-panelview menuseparator, is that right?

Maybe just stick that bit in a separate rule and just specify margin-top/bottom here?

@@ +799,5 @@
>    background-size: 1px 18px;
>    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
>  }
>  
> +.PanelUI-subView toolbarbutton[checked="true"],

Yay! Do we not need to do anything for the bookmarks standalone panel-that-is-really-a-menu here?

@@ +816,1 @@
>  .PanelUI-characterEncodingView-list > toolbarbutton[current]::before {

Bonus points if you update the character encoding view to just use checked="true" instead... :-)

If you don't want to do that here, please file a followup bug.

Comment 20

[Mike de Boer [:mikedeboer]]

Pushed with comments addressed:

remote: https://hg.mozilla.org/integration/fx-team/rev/6879e47ff29b

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6879e47ff29b

Comment 22

[Mike de Boer [:mikedeboer]]

Comment on attachment 8375422 [details] [diff] [review]
Patch v1.2: update separator styling in subviews and align related items accordingly

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis merge
User impact if declined: incoherent widths of separators inside panel menus
Testing completed (on m-c, etc.): baked on m-c for 3 days
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

Comment 23

[Mike de Boer [:mikedeboer]]

Unbitrotted and landed as:

https://hg.mozilla.org/releases/mozilla-aurora/rev/45c263f8653c

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0

- verified as fixed on latest Aurora (build ID: 20140310004003) and latest Nightly (build ID: 20140309030204) on Windows 7 64 bit, Ubuntu 12.04 and Mac OS X 10.9. 
- verified on a Microsoft Surface Pro 2 device running Windows 8.1 64 bit.