Closed Bug 1583531 Opened 2 months ago Closed 26 days ago

padding or margin wrong on bookmarks sub menu

Categories

(Firefox :: Menus, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 + fixed

People

(Reporter: asa, Assigned: surkov)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

Attached image 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

This probably regressed with bug 1555497, maybe?

Flags: needinfo?(surkov.alexander)

(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.

Flags: needinfo?(surkov.alexander)

(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?

Flags: needinfo?(emilio)

(the direction thing was bug 1562425, fwiw)

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

Flags: needinfo?(emilio)

(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.

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?

Regressed by: 1573158
Flags: needinfo?(bgrinstead)

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.

Flags: needinfo?(surkov.alexander)
Priority: -- → P1

(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.

Flags: needinfo?(surkov.alexander)

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

(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.

Flags: needinfo?(bgrinstead)

NI :surkov for comment 11.

Flags: needinfo?(surkov.alexander)

(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.

Flags: needinfo?(surkov.alexander)

(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.

Attachment #9101898 - Attachment description: Bug 1583531 - padding or margin wrong on bookmarks sub menu → Bug 1583531 - padding or margin wrong on bookmarks sub menu (via exportparts)
Assignee: nobody → surkov.alexander
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f285b3386fa
add in-bookmarks-menu class for arrowscrollboxes to adjust padding in bookmarks submenus r=bgrins
Status: NEW → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
You need to log in before you can comment on or make changes to this bug.