1583531 - padding or margin wrong on bookmarks sub menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Asa Dotzler [:asa]]

Attachment: menu-padding.png

The padding on bookmarks sub menus is wrong. There's not enough space between the edge of the menu and the first menu item.

Tested on latest Nightly build on Windows 10

Comment 1

[:Gijs (he/him)]

This probably regressed with bug 1555497, maybe?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to :Gijs (he/him) from comment #1)

This probably regressed with bug 1555497, maybe?

perhaps bug 1539651 would be a more plausible candidate, places menupoups overrides content of menupopups

interesting that on osx :-moz-lwtheme style is not applied to inner box of menupoup CE https://searchfox.org/mozilla-central/source/browser/themes/osx/customizableui/panelUI.css#47, which makes the inner box margin smaller than in Firefox 69 (release). I'm curious, if I observe a bug related with :-moz-lwtheme style or it's just because of my local settings which are different between the browsers.

Comment 3

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #2)

(In reply to :Gijs (he/him) from comment #1)

This probably regressed with bug 1555497, maybe?

perhaps bug 1539651 would be a more plausible candidate, places menupoups overrides content of menupopups

interesting that on osx :-moz-lwtheme style is not applied to inner box of menupoup CE https://searchfox.org/mozilla-central/source/browser/themes/osx/customizableui/panelUI.css#47, which makes the inner box margin smaller than in Firefox 69 (release). I'm curious, if I observe a bug related with :-moz-lwtheme style or it's just because of my local settings which are different between the browsers.

This description of the pseudoclass (or whatever the technically correct name for it is) not matching sounds similar to the same bug with :direction or :-moz-locale-dir - Emilio, does that sound plausible?

Comment 4

[:Gijs (he/him)]

(the direction thing was bug 1562425, fwiw)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Not really, :dir is local to the element.

The LWTheme pseudo-classes are global to the document and should match the same way for all the elements in the document: https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/servo/components/style/gecko/wrapper.rs#2198

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Not really, :dir is local to the element.

The LWTheme pseudo-classes are global to the document and should match the same way for all the elements in the document: https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/servo/components/style/gecko/wrapper.rs#2198

how do I control LWTheme pseudo-classes and how can I check whether they should be applied or not? Just trying to figure out if missing :-moz-lwtheme is a bug or just discrepancy in settings between release and nightly builds on my machine.

Comment 7

[alexander :surkov (:asurkov)]

it's a different issue on windows, there's no padding-top: 4px; style applied to submenues. The style was removed in bug 1573158 in changeset https://hg.mozilla.org/mozilla-central/rev/3f54e52c52eb5c4c9a0ff7ba8de2e21ed3fa1afa, the commit comment says:
"We were originally going to port these into document sheets, but after inspecting visually removing these aren't changing the UI."

Brian, what is the best way to fix this bug? I believe we cannot style XBL anonymous kids via shadow parts mechanism, so we cannot keep styles in the document sheets until arrowscrollbox is converted to CE (bug 1514926). So should we put those inline classes back? I think we can replace this.closest("#BMB_bookmarksPopup") condition on a class name and thus make shadow DOM fragments possible. Or should we wait for arrowscrollbox fixed?

Comment 8

[:Gijs (he/him)]

bug 1573158 is 71, but bug 1539651 (from comment #2) is 70 - is 70 affected? And could you please request tracking for the earliest affected release? We should fix this given it's a recent regression and it should be a straightforward, CSS-only patch.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to :Gijs (he/him) from comment #8)

bug 1573158 is 71, but bug 1539651 (from comment #2) is 70 - is 70 affected? > And could you please request tracking for the earliest affected release?

I think 70 is unaffected, I believe it's regressed by bug 1573158.

We should fix this given it's a recent regression and it should be a straightforward, CSS-only patch.

I outlined possible solution in comment #7, if it looks good, then I suppose I can come with a patch if Brian is not yet looking into this.

Comment 10

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Simple regression that we should fix before we ship.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #9)

(In reply to :Gijs (he/him) from comment #8)

bug 1573158 is 71, but bug 1539651 (from comment #2) is 70 - is 70 affected? > And could you please request tracking for the earliest affected release?

I think 70 is unaffected, I believe it's regressed by bug 1573158.

We should fix this given it's a recent regression and it should be a straightforward, CSS-only patch.

I outlined possible solution in comment #7, if it looks good, then I suppose I can come with a patch if Brian is not yet looking into this.

Thanks for looking into this, I haven't had time to look at it yet. What do you mean by "replace this.closest("#BMB_bookmarksPopup") condition on a class name" in Comment 7? For context, I'm hoping / planning that the arrowscrollbox custom element will land this week so we shouldn't have to worry about a temporary XBL fix.

Comment 12

[Julien Cristau [:jcristau] (back August 18)]

NI :surkov for comment 11.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: Bug 1583531 - padding or margin wrong on bookmarks sub menu (via exportparts)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #11)

Thanks for looking into this, I haven't had time to look at it yet. What do you mean by "replace this.closest("#BMB_bookmarksPopup") condition on a class name" in Comment 7? For context, I'm hoping / planning that the arrowscrollbox custom element will land this week so we shouldn't have to worry about a temporary XBL fix.

after arrowscrollbox is landed bug 1514926, I'd be super nice to use exportparts (the patch is attached), but exportparts is not implemented yet (see bug 1559076, Emilio says there's a wip on that). Giving timeframe of the upcoming release, we are on tight schedule here, so might be not worth to pursue the new feature implemented to fix the regression.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #14)

(In reply to Brian Grinstead [:bgrins] from comment #11)

Thanks for looking into this, I haven't had time to look at it yet. What do you mean by "replace this.closest("#BMB_bookmarksPopup") condition on a class name" in Comment 7? For context, I'm hoping / planning that the arrowscrollbox custom element will land this week so we shouldn't have to worry about a temporary XBL fix.

after arrowscrollbox is landed bug 1514926, I'd be super nice to use exportparts (the patch is attached), but exportparts is not implemented yet (see bug 1559076, Emilio says there's a wip on that). Giving timeframe of the upcoming release, we are on tight schedule here, so might be not worth to pursue the new feature implemented to fix the regression.

I think I will give a shot on approach from comment #7, and later when exportparts feature implemented, exportparts patch (D49624) can be taken in place.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: Bug 1583531 - padding or margin wrong on bookmarks sub menu

Comment 17

[alexander :surkov (:asurkov)]

Attachment: Bug 1583531 - add in-bookmarks-menu class for arrowscrollboxes to adjust padding in bookmarks submenus

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f285b3386fa
add in-bookmarks-menu class for arrowscrollboxes to adjust padding in bookmarks submenus r=bgrins

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1f285b3386fa