972405 - Bookmarks panel items should be aligned on the left

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

View bookmarks sidebar and Subscribe this page should be aligned with other items like in the subview and like every other widgets since bug 938578 has landed.

Comment 1

[u428464]

Also note that the Sidebar opened state is wrong in the panel.

Comment 2

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #0)
> View bookmarks sidebar and Subscribe this page should be aligned with other
> items like in the subview and like every other widgets since bug 938578 has
> landed.

I'm confused. Are you saying this broke because of bug 938578, or just that it wasn't fixed?

Comment 3

[u428464]

Sorry I wasn't clear. First it hasn't been fixed by bug 938578. The bookmarks panel is the only one that hasn't its items aligned on the left. Secondly when the bookmarks sidebar is enabled the "View Bookmarks sidebar" is aligned to the left and has no checkbox which its strange.

Comment 4

[:Gijs (he/him)]

OK, but then this shouldn't block the other bug, which was about separators... let's instead make it block the polish bug.

Comment 5

[u428464]

(In reply to :Gijs Kruitbosch from comment #4)
> OK, but then this shouldn't block the other bug, which was about
> separators... let's instead make it block the polish bug.

Bug 938578 was indeed about separators but it also contains the left alignment...

Comment 6

[u428464]

Attachment: Bookmarks panel bugs.png

Here's a (very very quick made and ugly) visual feedback.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug972405.patch

Okay.  So.

Bug XXX changed:
#BMB_bookmarksPopup > menuitem[checked="true"]::before
to:
.PanelUI-subView menuitem[checked="true"]::before
but, the BMB_bookmarksPopup isn't a PanelUI-subView, so it doesn't pick up any of the styling, and notably is now missing the checkmark.  This patch fixes that.

Screenshots:
https://dl.dropboxusercontent.com/u/2301433/Screenshots/BookmarksPanelMac1.png
https://dl.dropboxusercontent.com/u/2301433/Screenshots/BookmarksPanelWindows1.png

Comment 8

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #7)
> Created attachment 8376327 [details] [diff] [review]
> bug972405.patch
> 
> Okay.  So.
> 
> Bug XXX changed:
> #BMB_bookmarksPopup > menuitem[checked="true"]::before
> to:
> .PanelUI-subView menuitem[checked="true"]::before
> but, the BMB_bookmarksPopup isn't a PanelUI-subView, so it doesn't pick up
> any of the styling, and notably is now missing the checkmark.  This patch
> fixes that.
> 
> Screenshots:
> https://dl.dropboxusercontent.com/u/2301433/Screenshots/BookmarksPanelMac1.
> png
> https://dl.dropboxusercontent.com/u/2301433/Screenshots/
> BookmarksPanelWindows1.png

(note that the windows checked outline thing is bug 938603)

Comment 9

[:Gijs (he/him)]

Comment on attachment 8376327 [details] [diff] [review]
bug972405.patch

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

I think this works, but Mike might have a better idea. Is there any reason we shouldn't do this? :-)

Comment 10

[u428464]

On the Windows screenshot the first two items ("View bookmarks sidebar" and "Subscribe to this page") are still not aligned properly.

Comment 11

[Stephen Horlander [:shorlander]]

Comment on attachment 8376327 [details] [diff] [review]
bug972405.patch

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

The "View Bookmarks Sidebar" label shifts out of alignment when it is checked: 

- http://people.mozilla.org/~shorlander/bugs/Bug-972405-osx-alignment.png 
- http://people.mozilla.org/~shorlander/bugs/Bug-972405-windows-alignment.png

Could you also please use the menu checkmark?

- OS X — http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/menu/menu-check.png
- I think we pull Linux and Windows from the system

Thanks!

Comment 12

[u428464]

In fact the Subscribe item should always have an icon but greyed out when disabled.

Comment 14

[Mike de Boer [:mikedeboer]]

As far as the _summary_ of this bug is concerned, I think Gijs' patch is a step in the right direction. I don't really get how checkmarks got dragged into this.

If we still want to address that here, plz update the summary. Otherwise I'd rather see that work moved to a more appropriate bug. Also note that the check marks we currently use in the panel are Unicode characters, not images.

Gijs, it's up to you if you also want to address the check mark _alignment_ here. I think we'll be doing a more 'final pass' over all our panels and subviews soon, however.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: Borrow the PanelUI-subView styling, and make the checkbox line up.

Fixed the alignment (on OS X, will check on Windows next).
Switching to the menu checkmark will be a bit more involved, and it would be nice to get this in first, so I'm going to try to tackle that in a separate patch.  (Possibly attached to a separate bug.)

Comment 16

[Stephen Horlander [:shorlander]]

Comment on attachment 8377737 [details] [diff] [review]
Borrow the PanelUI-subView styling, and make the checkbox line up.

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

Looks good on OS X, still looks off on Windows.

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: Supplementary patch for linux, to be qfolded

This patch qfolds onto attachment 8377737 [details] [diff] [review] and makes this work well on linux. I'll upload an animation in a moment.

Comment 18

[Manish Goregaokar [:manishearth]]

Attachment: Animation on linux

Here's the animation. Ignore the blue thing, that's the nightly logo jumping back and forth due to the appearance of the sidebar.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch…

Okay, this one has Manishearth's Linux patch folded in, and fixes the Mac alignment.

Phteven, what do you think?  Getting close?  Better than what we currently have?

Comment 20

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #19)
> Created attachment 8382285 [details] [diff] [review]
> The next version of the patch…
> 
> Okay, this one has Manishearth's Linux patch folded in, and fixes the Mac
> alignment.
> 
> Phteven, what do you think?  Getting close?  Better than what we currently
> have?

This looks like it breaks checkmarks for the toolbarbuttons in the main menu panel.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: A stripped down version of the patch, which doesn't change the checkmarks.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Attachment: Like the previous one, but with 5px spacing on OS X.

As per Stephen's showing me the misalignment in real life.

Comment 23

[Stephen Horlander [:shorlander]]

Comment on attachment 8384656 [details] [diff] [review]
Like the previous one, but with 5px spacing on OS X.

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

Looks good, thanks!

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 8384656 [details] [diff] [review]
Like the previous one, but with 5px spacing on OS X.

> .subviewbutton.panel-subview-footer > .toolbarbutton-text,
> .subviewbutton.panel-subview-footer > .menu-text {
>   -moz-padding-start: 0;
>+  -moz-padding-end: 12px;
>   text-align: center;
>+  -moz-box-flex: 0 !important;
> }

text-align:center doesn't seem useful if there's no flex.

>   <binding id="menuitem" extends="chrome://global/content/bindings/menu.xml#menuitem-base">
>     <content>
>-      <xul:label class="menu-text" flex="1" xbl:inherits="value=label,accesskey,crop" crop="right"/>
>+      <xul:label class="menu-text" style="-moz-box-flex: 1" xbl:inherits="value=label,accesskey,crop" crop="right"/>

This should probably move to xul.css rather than being an inline style.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Attachment: Like the previous one, but with Dao's suggestions.

(Carrying forward ui-r=shorlander)

(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 8384656 [details] [diff] [review]
> > .subviewbutton.panel-subview-footer > .toolbarbutton-text,
> > .subviewbutton.panel-subview-footer > .menu-text {
> text-align:center doesn't seem useful if there's no flex.

Removed!

> >+      <xul:label class="menu-text" style="-moz-box-flex: 1" xbl:inherits="value=label,accesskey,crop" crop="right"/>
> This should probably move to xul.css rather than being an inline style.

Moved!

Thanks,
Blake.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8384692 [details] [diff] [review]
Like the previous one, but with Dao's suggestions.

>           <menupopup id="BMB_bookmarksPopup"
>-                     class="cui-widget-panel cui-widget-panelview cui-widget-panelWithFooter"
>+                     class="cui-widget-panel cui-widget-panelview cui-widget-panelWithFooter PanelUI-subView"

Doesn't this add PanelUI-subView even if this opens from the toolbar rather than as a sub view in the panel? Doesn't seem to make much sense in that case.

> .subviewbutton.panel-subview-footer > .toolbarbutton-text,
> .subviewbutton.panel-subview-footer > .menu-text {
>   -moz-padding-start: 0;
>-  text-align: center;
>+  -moz-padding-end: 12px;
>+  -moz-box-flex: 0 !important;
> }

Now you shouldn't need the !important anymore.

>   <binding id="menuitem" extends="chrome://global/content/bindings/menu.xml#menuitem-base">
>     <content>
>-      <xul:label class="menu-text" flex="1" xbl:inherits="value=label,accesskey,crop" crop="right"/>
>+      <xul:label class="menu-text" xbl:inherits="value=label,accesskey,crop" crop="right"/>
>       <xul:hbox class="menu-accel-container" anonid="accel">
>         <xul:label class="menu-accel" xbl:inherits="value=acceltext"/>
>       </xul:hbox>
>     </content>
>   </binding>

>+menuitem > .menu-text {
>+  -moz-box-flex: 1;
>+}

Can you do this for both menuitem and menu? Inconsistency doesn't help here.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch…

(Continuing to carry forward ui-r=shorlander.)

(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 8384692 [details] [diff] [review]
> >           <menupopup id="BMB_bookmarksPopup"
> >-                     class="cui-widget-panel cui-widget-panelview cui-widget-panelWithFooter"
> >+                     class="cui-widget-panel cui-widget-panelview cui-widget-panelWithFooter PanelUI-subView"
> Doesn't this add PanelUI-subView even if this opens from the toolbar rather
> than as a sub view in the panel? Doesn't seem to make much sense in that
> case.

Yes, but it makes it like #PanelUI-history and #PanelUI-developer, both of which have the PanelUI-subView class when opened from the toolbar.  (My whole purpose in adding it was to make it like those, so that we can style everything once and have it all be the same.  :)

> >+  -moz-box-flex: 0 !important;
> Now you shouldn't need the !important anymore.

Fixed!

> >   <binding id="menuitem" extends="chrome://global/content/bindings/menu.xml#menuitem-base">
> >     <content>
> >-      <xul:label class="menu-text" flex="1" xbl:inherits="value=label,accesskey,crop" crop="right"/>
> >+      <xul:label class="menu-text" xbl:inherits="value=label,accesskey,crop" crop="right"/>
[...]
> >+menuitem > .menu-text {
> >+  -moz-box-flex: 1;
> >+}
> Can you do this for both menuitem and menu? Inconsistency doesn't help here.

Fixed!

Thanks,
Blake.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Blake Winton (:bwinton) from comment #27)
> Yes, but it makes it like #PanelUI-history and #PanelUI-developer, both of
> which have the PanelUI-subView class when opened from the toolbar.  (My
> whole purpose in adding it was to make it like those, so that we can style
> everything once and have it all be the same.  :)

The consistency is of little value if the whole approach is wrong in the first place. I don't know what the right solution is here, maybe the class just needs to be renamed or a separate class should be added. In any case, I'd like to at least see a bug filed before I r+ something that essentially looks wrong.

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Filed bug 978881, to make the bookmarks panel an actual panel, instead of a menu that tries to look like a panel.

Comment 30

[Dão Gottwald [:dao]]

(In reply to Blake Winton (:bwinton) from comment #29)
> Filed bug 978881, to make the bookmarks panel an actual panel, instead of a
> menu that tries to look like a panel.

How does this bug relate to the issue of PanelUI-subView being used on what isn't a sub view?

Comment 31

[Blake Winton (:bwinton) (:☕️)]

We'll rename the class when we make it an actual panel.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

In the interests of moving this bug forward, I've also filed bug 978922.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 8384742 [details] [diff] [review]
The next version of the patch…

>+menu > .menu-text,
>+menuitem > .menu-text {
>+  -moz-box-flex: 1;
>+}

.menu-text {
  -moz-box-flex: 1;
}

r=me with that change

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Attachment: An alternate version of the patch.

(In reply to Dão Gottwald [:dao] from comment #33)
> >+menu > .menu-text,
> >+menuitem > .menu-text {
> >+  -moz-box-flex: 1;
> >+}
> 
> .menu-text {
>   -moz-box-flex: 1;
> }
> 
> r=me with that change

So, I'm happy to make that change (and indeed, here's that version), but aren't you concerned that that might catch things with a class of menu-text in add-ons?

Since I'm not that concerned, I'll let you pick which version to mark obsolete and which to check in.  :)

Thanks,
Blake.

Comment 35

[Dão Gottwald [:dao]]

(In reply to Blake Winton (:bwinton) from comment #34)
> So, I'm happy to make that change (and indeed, here's that version), but
> aren't you concerned that that might catch things with a class of menu-text
> in add-ons?

Not at all.

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8385016 [details] [diff] [review]
An alternate version of the patch.

Carrying forward r=dao and ui-r=shorlander.

Comment 37

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8384742 [details] [diff] [review]
The next version of the patch…

(Clearing review flags so that this patch doesn't get checked in.
Mostly because I can't mark it obsolete, as far as I can tell.)

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3d267302cbd6

Comment 39

[u428464]

Attachment: Still not aligned to the left.png

This is still not resolved and sadly the bookmark toolbar submenu has a double checkmark when it is enabled.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d267302cbd6

Comment 41

[u428464]

Is the plan to have the two first items not aligned to the far left as seen on the mockups on Windows ?

https://bugzilla.mozilla.org/attachment.cgi?id=8385517 shows what it looks like now.

Comment 42

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #41)
> Is the plan to have the two first items not aligned to the far left as seen
> on the mockups on Windows ?
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8385517 shows what it looks
> like now.

Comment 43

[Blake Winton (:bwinton) (:☕️)]

From shorlander on IRC:
> If we follow system convention then on Window *every* menu has space on the left for icons
> Whether they have icons or not

I'll likely be futzing with the alignment as part of bug 979378, so let's leave this one resolved, and do the extra work there.

Comment 44

[Blake Winton (:bwinton) (:☕️)]

(There's also bug 938603 which seems related, but even more focused…)

Comment 45

[:Gijs (he/him)]

Comment on attachment 8385016 [details] [diff] [review]
An alternate version of the patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bookmarks menu panel menu items don't look right
Testing completed (on m-c, etc.): on m-c for quite a while
Risk to taking this patch (and alternatives if risky): medium. There are some known issues, but this and the other patches to the bookmarks panel make the overall situation better, IMO, and so it's either moving forward with all of them or keeping Aurora frozen (in a pretty non-ideal state). At this point I think moving forward is less risky than not taking these patches
String or IDL/UUID changes made by this patch: none

Comment 46

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/791fa5b46a18