1018595 - Context menu items are inconsistently spaced

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: The first attachment

In linux with Adwaita theme iconised items are spaced differently compared to all other items.

For example the back button has 4 (or 5 depending on whether you include the border) pixels padding on the left and 4 pixels of padding on the top. Regular menu items have 0px padding and highlight borders with blue as well (see the first attachment)

Moreover, the icons are spaced doubly on the top if there’s items above those icon items. See the second attachment.

Comment 1

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: The second attachment

Comment 2

[:Gijs (he/him)]

Comment on attachment 8432103 [details]
The second attachment

There shouldn't be any items above (and we're fixing that in another bug), so I'm marking that screenshot as obsolete for now. We can focus on the other issues you highlighted. :-)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

The first issue will be fixed by bug 1016405.

Comment 4

[Simonas Kazlauskas [:nagisa/:simukis]]

I reopened this issue as bug 1016405 did not fix the issue. Patch attached.

Comment 5

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Patch v1

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8458696 [details] [diff] [review]
Patch v1

I'm afraid this makes the icons look misaligned when they're not hovered, as there's less space above them than below them.

Comment 7

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Context menu with single regular item before separator

I have 2 counter-arguments to that:

1. Currently context menu looks inconsistent with pretty much any theme unless the theme also has 4 pixel padding on top of context menu. I am not aware of any theme which is bundled with a major distribution and has 4 pixels of padding in that place (though I can’t say I’ve seen many of them either)
2. Following your complaint, in case context menu has only a single *regular* item before a separator, the text on first item should look misaligned too. The attachment is a screenshot of genuine context menu from GTK+ app.

Between what parts of context menu do you want icons to be aligned? Do you want icon to have the same amount of space between (the top of context menu and the top of icon) and (the bottom of icon and the visible part of separator)?

We should go for consistency and not appearance here IMO.

Comment 8

[Simonas Kazlauskas [:nagisa/:simukis]]

Also hardcoded value of 4 pixels is bad. For example GTK+2 Adwaita theme has 2 pixels margin-top on separator so I could say it looks as misaligned with those 4 pixels as without without them. I bet different themes also have different margins for the separator.

Comment 9

[Simonas Kazlauskas [:nagisa/:simukis]]

I still think my approach is the correct one. Can I get somebody to ui-review it?

Comment 10

[Dão Gottwald [:dao]]

If you request ui-review from [e-mail], somebody should theoretically take care of that.

Comment 11

[Sevaan Franks [:sevaan]]

Could you please attached a screenshot, simukis?

Comment 12

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Screenshot of context menus

(In reply to Sevaan Franks [:sevaan] from comment #11)
> Could you please attached a screenshot, simukis?

Sure, no problem! I will do screenshots for more themes if requested.

Note: Native screenshots are for GTK+3. The GTK+3 theme has 2 more pixels of margin-top on separators compared to GTK+2 (on which Firefox is currently based) theme.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8460309 [details]
Screenshot of context menus

Please also provide a screenshot using Ubuntu.

Comment 14

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Screenshots of context menus with Ubuntu

This is screenshots of context menus made with fresh Ubuntu install for all themes that are available.

I made several mistakes making screenshots for high contrast theme (reload button looks odd, native context menu is different). Please disregard them.

Comment 15

[Simonas Kazlauskas [:nagisa/:simukis]]

Attachment: Patch v1 (with r= appended)

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/428c28ca3e36

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/428c28ca3e36

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Reproduced this issue on
Nightly 32.0a1
Build Id: 20140531030204
User Agent: Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox 32.0

Verified as fixed on
Firefox 34 Beta 9
Build Id: 20141114133026
User Agent: Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox 34.0