Closed Bug 610773 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: faaborg, Assigned: Margaret)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Blocks: 590069
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → margaret.leibovic
Attachment #489538 - Flags: ui-review?(faaborg)
Attached image screenshot (obsolete) —
Oh, I just noticed bug 610924. This fix should probably go over there.
>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 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.
Attachment #489538 - Flags: ui-review?(faaborg) → ui-review+
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #489538 - Attachment is obsolete: true
Attachment #492514 - Flags: review?(dao)
Attached file patch v2 screenshot (obsolete) —
Attachment #489539 - Attachment is obsolete: true
Attachment #492515 - Flags: ui-review?(faaborg)
Duplicate of this bug: 610940
Duplicate of this bug: 610924
Duplicate of this bug: 610830
Attachment #492515 - Attachment is patch: false
Attached image patch v2 screenshot
Sorry, I failed at uploading an image.
Attachment #492515 - Attachment is obsolete: true
Attachment #492517 - Flags: ui-review?(faaborg)
Attachment #492515 - Flags: ui-review?(faaborg)
Attachment #492517 - Flags: ui-review?(faaborg) → ui-review+
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.
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.
(In reply to comment #13)
> Should I just delete them?

Yes.
Attachment #492514 - Flags: review?(dao)
Attached patch patch v3Splinter Review
Attachment #492514 - Attachment is obsolete: true
Attachment #493059 - Flags: review?(dao)
Attachment #493059 - Flags: review?(dao) → review+
Component: General → Theme
QA Contact: general → theme
Attachment #493059 - Flags: approval2.0?
Attachment #493059 - Flags: approval2.0? → approval2.0+
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; ?
(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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
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?
Blocks: 617051
(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.
You need to log in before you can comment on or make changes to this bug.