1315873 - additional margin around menu separator items

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Karl Tomlinson (:karlt)]

This adds spacing around separators inconsistent with native apps, and gaps between any backgrounds drawn in the menu items (to address bug 1287036).

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: separator spacing expected from gucharmap

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: separator spacing in Firefox

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1315873 remove unused non-native fallback menuseparator styling, including margin accidentally applied to native separators

border and background-image are overridden by non-none -moz-appearance.

Review commit: https://reviewboard.mozilla.org/r/91302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91302/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8808479 [details]
bug 1315873 remove unused non-native fallback menuseparator styling, including margin accidentally applied to native separators

Can you use padding instead of border-top/bottom, set a height, use background-origin: content-box and get rid of the transparent portions of the gradient?

Also the following rule (menulist > menupopup > menuseparator) needs an update.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Can you use padding instead of border-top/bottom, set a height, use
> background-origin: content-box and get rid of the transparent portions of
> the gradient?

If a height is set here, then that will be applied even with non-none -moz-appearance.  That's not ideal, but provided native menus are never smaller than this height, then perhaps zero padding and minimum size could be provided instead of a border size for native menuseparators.  I'll have to check this as (?xul) layout ignores minimum sizes for native widgets in some situations and I don't know the reasons for this or which situations are affected.

Would you prefer this solution?
It may be a little more complicated in the native widget code, but not crazy.
The main disadvantage is the height behaving as a minimum size even for native widgets.

Can you explain your concerns with background-origin:border-box and/or transparent gradient to help me understand, please?

> Also the following rule (menulist > menupopup > menuseparator) needs an
> update.

Thanks.  I will look at that.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Can you explain your concerns with background-origin:border-box and/or
> transparent gradient to help me understand, please?

It just seemed unnecessarily complicated to me.

I think we can also just remove the fallback styling. toolkit/themes/osx/global/menu.css doesn't provide this for instance.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Dão Gottwald [:dao] from comment #6)
> I think we can also just remove the fallback styling.
> toolkit/themes/osx/global/menu.css doesn't provide this for instance.

Oh, I had assumed they must be used somewhere, but panelUI and
xul|menulist are the only overrides mentioning menuseparator explicitly, and
they both override all of border-top/bottom, margin, and padding.

http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/browser/themes/shared/customizableui/panelUI.inc.css#1181
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/toolkit/themes/shared/in-content/common.inc.css#421

So that seems the perfect solution, thanks.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8808479 [details]
bug 1315873 remove unused non-native fallback menuseparator styling, including margin accidentally applied to native separators

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91302/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db092219b871
remove unused non-native fallback menuseparator styling, including margin accidentally applied to native separators r=dao

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db092219b871