Closed Bug 972405 Opened 6 years ago Closed 6 years ago

Bookmarks panel items should be aligned on the left

Categories

(Firefox :: Theme, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: ge3k0s, Assigned: bwinton)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(4 files, 8 obsolete files)

14.09 KB, image/png
Details
54.37 KB, image/gif
Details
6.42 KB, patch
dao
: review+
bwinton
: ui-review+
Details | Diff | Splinter Review
12.24 KB, image/png
Details
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.
Whiteboard: [Australis:P3]
Blocks: 938578
Also note that the Sidebar opened state is wrong in the panel.
(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?
Flags: needinfo?(ge3k0s)
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.
Flags: needinfo?(ge3k0s)
OK, but then this shouldn't block the other bug, which was about separators... let's instead make it block the polish bug.
Blocks: 963098
No longer blocks: 938578
(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...
Here's a (very very quick made and ugly) visual feedback.
Attached patch bug972405.patch (obsolete) — Splinter Review
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
Assignee: nobody → bwinton
Attachment #8376327 - Flags: ui-review?(shorlander)
(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 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? :-)
Attachment #8376327 - Flags: review?(mdeboer)
On the Windows screenshot the first two items ("View bookmarks sidebar" and "Subscribe to this page") are still not aligned properly.
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!
Attachment #8376327 - Flags: ui-review?(shorlander) → ui-review-
In fact the Subscribe item should always have an icon but greyed out when disabled.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.)
Attachment #8376327 - Attachment is obsolete: true
Attachment #8376327 - Flags: review?(mdeboer)
Attachment #8377737 - Flags: ui-review?(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.
Attachment #8377737 - Flags: ui-review?(shorlander) → ui-review-
This patch qfolds onto attachment 8377737 [details] [diff] [review] and makes this work well on linux. I'll upload an animation in a moment.
Attached image 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.
Attached patch The next version of the patch… (obsolete) — Splinter Review
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?
Attachment #8377737 - Attachment is obsolete: true
Attachment #8381715 - Attachment is obsolete: true
Attachment #8382285 - Flags: ui-review?(shorlander)
(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.
Attachment #8382285 - Attachment is obsolete: true
Attachment #8382285 - Flags: ui-review?(shorlander)
Attachment #8383208 - Flags: ui-review?(shorlander)
As per Stephen's showing me the misalignment in real life.
Attachment #8383208 - Attachment is obsolete: true
Attachment #8383208 - Flags: ui-review?(shorlander)
Attachment #8384656 - Flags: ui-review?(shorlander)
Attachment #8384656 - Flags: review?(dao)
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!
Attachment #8384656 - Flags: ui-review?(shorlander) → ui-review+
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.
Attachment #8384656 - Flags: review?(dao) → review-
(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.
Attachment #8384656 - Attachment is obsolete: true
Attachment #8384692 - Flags: ui-review+
Attachment #8384692 - Flags: review?(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.
Attachment #8384692 - Flags: review?(dao) → review-
Attached patch The next version of the patch… (obsolete) — Splinter Review
(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.
Attachment #8384692 - Attachment is obsolete: true
Attachment #8384742 - Flags: ui-review+
Attachment #8384742 - Flags: review?(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.
Filed bug 978881, to make the bookmarks panel an actual panel, instead of a menu that tries to look like a panel.
(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?
We'll rename the class when we make it an actual panel.
In the interests of moving this bug forward, I've also filed bug 978922.
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
Attachment #8384742 - Flags: review?(dao) → review+
(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.
(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 on attachment 8385016 [details] [diff] [review]
An alternate version of the patch.

Carrying forward r=dao and ui-r=shorlander.
Attachment #8385016 - Flags: ui-review+
Attachment #8385016 - Flags: review+
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.)
Attachment #8384742 - Flags: ui-review+
Attachment #8384742 - Flags: review+
Keywords: checkin-needed
Attachment #8384742 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/3d267302cbd6
Keywords: checkin-needed
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
This is still not resolved and sadly the bookmark toolbar submenu has a double checkmark when it is enabled.
https://hg.mozilla.org/mozilla-central/rev/3d267302cbd6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Flags: needinfo?(shorlander)
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(shorlander)
Resolution: --- → FIXED
(There's also bug 938603 which seems related, but even more focused…)
Depends on: 980534
Depends on: 980895
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
Attachment #8385016 - Flags: approval-mozilla-aurora?
Attachment #8385016 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.