610773 - In the Firefox menu items in the right and left panes need to be vertically aligned

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Aligning the first two items in both panes

The first two items in the right and left panes of the Firefox menu should be vertically aligned.

The third item and downward in the right pane will then no longer be aligned due to the first splitter after "Start Private Browsing" but we should start with the first two lined up.

Comment 1

[:Margaret Leibovic]

Attachment: patch

This patch fixes the problem by adding padding to the left pane to match the existing padding in the right pane. While I was modifying the padding, I decided to increase the padding on all sides of the app menu panes to better match this mockup: https://bug583386.bugzilla.mozilla.org/attachment.cgi?id=465479. This would also fix bug 610940. Alex, is this the correct change? I will attach a screenshot.

Comment 2

[:Margaret Leibovic]

Attachment: screenshot

Comment 3

[:Margaret Leibovic]

Oh, I just noticed bug 610924. This fix should probably go over there.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, is this the correct change? I will attach a
>screenshot.

looks good for addressing the vertical alignment of menu items.  In terms of padding, we could collapse all of those bugs together, top and bottom should be 9px [1] (this increases to 12 top and 11 bottom).  And the icon edge to the left edge should be 7px [2] (this increases to 10 left pane and 9 right pane)

[1] https://bug610924.bugzilla.mozilla.org/attachment.cgi?id=489407
[2] https://bug610940.bugzilla.mozilla.org/attachment.cgi?id=489419

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 489538 [details] [diff] [review]
patch

+ in terms of this specific bug, if we want to tweak padding see the comment above for new values.

Comment 6

[:Margaret Leibovic]

Attachment: patch v2

This patch fixes the alignment issue by fixing the padding in the app menu. It also fixes a bunch of other padding-related bugs, which I'll dupe to this.

Comment 7

[:Margaret Leibovic]

Attachment: patch v2 screenshot

Comment 11

[:Margaret Leibovic]

Attachment: patch v2 screenshot

Sorry, I failed at uploading an image.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 492514 [details] [diff] [review]
patch v2

>   #appmenuPrimaryPane {
>     -moz-margin-end: -1px;
>     background-color: rgba(255,255,255,0.5);
>     -moz-border-end: 1px solid #c4c4c5;
>-    -moz-padding-start: 2px;
>+    padding: 2px;
>   }
>   #appmenuSecondaryPane {
>     background-color: #f1f5fb;
>     box-shadow: 1px 0 2px rgb(204,214,234) inset;
>     border: 0;
>-    padding-top: 5px;
>+    -moz-padding-start: 3px;
>+    -moz-padding-end: 2px;
>+    padding-top: 2px;
>+    padding-bottom: 2px;
>     font-family: "Segoe UI Semibold", "Segoe UI", sans-serif;
>   }

Hm, maybe #appmenuPrimaryPane shouldn't have -moz-margin-end: -1px;? Not sure what that's doing.

Comment 13

[:Margaret Leibovic]

That negative margin makes the secondary pane overlap with the border, which seems strange because that border is being explicitly set. If I delete both the margin and the border rules the menu looks exactly the same. It seems like these rules may just be the result of iterating on the style for the original app menu patch. Should I just delete them?

Also, the border rule in #appmenuSecondaryPane doesn't seem to be necessary either.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> Should I just delete them?

Yes.

Comment 15

[:Margaret Leibovic]

Attachment: patch v3

Comment 16

[tymerkaev]

Comment on attachment 493059 [details] [diff] [review]
patch v3

>+    -moz-padding-start: 3px;
>+    -moz-padding-end: 2px;
>+    padding-top: 2px;
>+    padding-bottom: 2px;

  padding: 2px;
  -moz-padding-start: 3px; ?

Comment 17

[:Margaret Leibovic]

(In reply to comment #16)
>   padding: 2px;
>   -moz-padding-start: 3px; ?

Dão knows more about this than me, but I believe it's less efficient to do this because it would probably evaluate the padding declaration for all sides, then have to override one of those styles with -moz-padding-start.

I checked in the current patch:
http://hg.mozilla.org/mozilla-central/rev/eaecf12fbf6e

Comment 18

[:Gavin Sharp [email: [e-mail]]]

FWIW, the perf difference should be negligible (and "parse two rules and override one" may actually be slightly more efficient than "parse 4 rules"), so picking the most concise is generally best.

Comment 19

[Valerio]

After the land of this patch there is a grey line between the primary pane and the secondary pane of the Firefox menu that covers the blue shadow. It's a bug?

Comment 20

[:Margaret Leibovic]

(In reply to comment #19)
> After the land of this patch there is a grey line between the primary pane and
> the secondary pane of the Firefox menu that covers the blue shadow. It's a bug?

Yes, thanks for noticing this regression. I filed bug 617051 to fix it.