1692081 - Menu/submenu positioning is misaligned

Status: RESOLVED WORKSFORME

(Firefox :: Menus, defect, P1)

Description

[Molly Howell (she/her) (no longer active)]

Since bug 1682522 affected <select> boxes, when a select box menu opens on the top side of the box (as happens when the box is too close to the bottom of the screen), the menu appears too far above the box, because of the space being left for the shadow. Perhaps the shadow should be removed for select boxes.

Also, submenus are offset from where they ought to appear for the same reason, because of the space being left for the shadow. Those probably can't disable the shadow, so they just need to be moved into place.

Comment 1

[:Gijs (he/him)]

This also affects the File menu in the menubar on maximized windows - it's shifted to the right to make room for the shadow. We should cut off the shadow instead.

Comment 2

[:Gijs (he/him)]

Neil was going to look into using the content box of the menupopup rather than the padding/border-box, to position it / determine whether it's on-screen.

Comment 3

[Neil Deakin]

Attachment: Bug 1692081, subtract off padding and border in menu positioning computation when there is a dropshadow.

Comment 4

[Neil Deakin]

The above is a proof-of-concept that computes the popup's position by subtracting out the padding and border. It will work for this particular popup styles used on windows 10 and seems to work ok for menubars, menu buttons and context menus.

There would likely need to be some flag that can be used to enable this behaviour instead of the usual margin computation behaviour. I suppose this would be ideal to be an opt-in that that could be specified in the stylesheet.

It doesn't work for menulists. Those would require additional layout work as the size of the menulist button is dependent on the size of the popup and the padding/border would need to be taken into account. Possibly, the existing menupopup style should be used for menulists instead.

Also, many menupopup tests will fail if the proton menus preference is enabled with or without this patch, so there would be quite a bit of work there to fix these up.

Comment 5

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #4)

The above is a proof-of-concept that computes the popup's position by subtracting out the padding and border. It will work for this particular popup styles used on windows 10 and seems to work ok for menubars, menu buttons and context menus.

Thanks, this looks great!

There would likely need to be some flag that can be used to enable this behaviour instead of the usual margin computation behaviour. I suppose this would be ideal to be an opt-in that that could be specified in the stylesheet.

It doesn't work for menulists. Those would require additional layout work as the size of the menulist button is dependent on the size of the popup and the padding/border would need to be taken into account. Possibly, the existing menupopup style should be used for menulists instead.

We could set padding/margins to 0 for menulists for now, as suggested in bug 1693866, and get all the styling benefits minus the box shadow, and file a follow-up bug for addressing that so we can have the shadows, too. Given there are no shadows for these on Windows right now, that is probably fine.

Also, many menupopup tests will fail if the proton menus preference is enabled with or without this patch, so there would be quite a bit of work there to fix these up.

Right, that makes sense. Do you have time / inclination to finish this patch (without dealing with the intricacies of the menulist case), or would you prefer if someone else dealt with the tests and the style-sheet based opt-in?

Comment 6

[randomreporter]

Similar bug when right-clicking near the bottom of a web page? See the screenshot, right dot is where I right clicked, menu is obviously misaligned.
https://i.imgur.com/V7VsErY.png

Comment 7

[Blake Winton (:bwinton) (:☕️)]

As per guidance from Vicky, for tracking, we're marking all the bugs that people are working on as P1.

Comment 8

[Neil Deakin]

This latest version of the patch fixes almost all tests. Some <select> tests in browser/base/content/test/forms still fail because menulist popups need to revert the other parts (box-shadow and border I think) that were added for the new menu look. I added an override for the popup to remove the padding, but the remaining would be work for someone else to look at.

Most of the changes just involve using a version of getBoundingClientRect that accounts for the additional padding/border. As a menupopup is now quite a bit larger on Windows 10, the sizes of various windows that are opened, and the margins around various menu buttons need to be increased to allow the menus to fit within their allotted area. I thought this might be a problem for the test test_popup_button.html which now needs a window 950x950 but it looks like the test machines on Windows 10 run at 1280x1024.

There isn't any conditional handling added yet, except for a check to see if the popup is a menulist, so several toolkit tests will also fail on linux if you run this patch as is, as popups there have a 1 pixel border on them. Once we work out what that conditional is, we can just disable all this extra code and the tests will work again.

Comment 9

[Neil Deakin]

The background here is that menupopups on Windows 10 (with the browser.proton.contextmenus.enabled preference set) now have a larger padding to accommodate a box-shadow drawn around them, however this means that the alignment of menus is off because while the widget takes up a certain amount of space, visibly they take up less space. So this patch adjusts the alignment computations to subtract off the new padding/border area from the computed margin.

However, we only want this special computation to occur on Windows 10, while preference is enabled, and not for popups in menulists (for now). Ideally, we could disable it for any other popups as well.

The question is, would adding something that can be specified and/or queried in a stylesheet be the best approach here, and what would that be? A toggle, a new margin that can be specified, or what?

Or is hard-coding the variation using in nsMenuPopupFrame a better choice?

Note that both nsMenuPopupFrame and tests need to be able to detect which appearance in being used.

An alternate option is to push this all into windows widget code and have it instead expand the widget size to account for the box-shadow placed directly on the menupopup instead, but I suspect that is quite a bit more work.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Does the menupopup code deal with margins? The usual way authors would deal with this is using negative margins... We also have -moz-window-transform, which right now only works for cocoa widgets, but seems like it could be generalized?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Ah, it seems what we're doing now is using negative margins... why does that not work?

Comment 12

[:Gijs (he/him)]

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

Ah, it seems what we're doing now is using negative margins... why does that not work?

I mean, at the simplest level, I don't think the XUL popup positioning code takes margins into account when assessing if the menu/panel is off-screen / needs flipping/sliding in order to be on-screen. IIRC, making it do that is tricky because the application of the top/bottom of the margins changes if the menu/panel is flipped (so the pre-patch use of a different top vs. bottom margin doesn't work well in those cases). Neil, is that accurate and/or are there other reasons we can't take the negative margins into account from the positioning / "is this within the screen bounds" code?

Comment 13

[Neil Deakin]

Yes, we could also just use negative margins and that should just work. It still means that we would need to use the same value on both horizontal sides and the same value of both vertical sides.

However, using the margin does mean that any menupopups won't easily be able to modify the margin if needed without having extra overrides for windows 10. I see a few existing menus do have margin assigned. For example: https://searchfox.org/mozilla-central/rev/49cdffbdaf57888d09ea9554f286ea73684e4397/browser/themes/shared/toolbarbuttons.inc.css#154 I suppose the same is true of the padding, but I suspect that means one expects the appearance to be different, and not just the position.

I'll try out a change that just uses a margin and see how it works.

Comment 15

[Neil Deakin]

Attachment: Bug 1692081, adjust margin of menus on Windows 10 to account for dropshadow

Comment 16

[Neil Deakin]

This latest version uses a negative margin instead. There are still some additional tests that use a margin that don't work, and a few that I think expect the trigger mouse events in the dropshadow area. If we think this version is an approach worth taking instead I can continue to fix the tests.

Comment 17

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #16)

This latest version uses a negative margin instead. There are still some additional tests that use a margin that don't work, and a few that I think expect the trigger mouse events in the dropshadow area. If we think this version is an approach worth taking instead I can continue to fix the tests.

I'm a little confused - I don't see any C++ changes in the second patch, and the horizontal margin/padding isn't changing, so how is this addressing the issue of positioning the menu close to the edge of the window/screen edge? Isn't the popup manager still not using the margin to discount the padding in terms of ensuring the popup's contents are fully within the screen/window boundaries?

Comment 18

[Neil Deakin]

Are you expecting the dropshadow to be allowed to appear off-screen?

Only the margin between the anchor point/rectangle and the popup edge is used for computing the screen rectangle. The opposite margin is not used.

There may be some nsMenuPopupFrame changes needed eventually if we decide that this latter approach is more viable. I haven't manually tested, but most of the existing tests that don't use manually assigned margins work.

Comment 19

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #18)

I think primarily I was confused because I didn't realize you also updated the first patch - bugzilla doesn't tell you unless the commit message changes - and the second patch had only test changes. However...

Are you expecting the dropshadow to be allowed to appear off-screen?

That was my intuition, yes. AIUI your patch accomplishes that for things on the top left, ie it's supposed to fix the issue I noted in comment #1 - right? But not when the bottom/right hand side of the menu (including shadow) would cross the bottom/right hand side of the screen? Maybe I misunderstand how your patch works...

Comment 20

[Neil Deakin]

The position will be correct in all directions. The edge of the screen is used as is, but the popup's border-box is used as it currently is, so the extra padding area used by the dropshadow will be included in the popup's size. If we want to instead allow the dropshadow to be ignored and use the content-box, we would need to use code as in the first patch.

We might need to add some special case for when a constraint rectangle is used, but I'm not sure if we have cases anymore where a content area can open a popup.

Comment 21

[Tim Nguyen :ntim]

Neil, would it be possible to allow setting the box-shadow on the menupopup directly without any padding/margin? AFAIK, nsMenuPopupFrame should be able to read the box-shadow blur radius, so I expect the following to work:

• Adding a new function (GetShadowSize?) that gets padding to account for box-shadow (that would possibly equal the blur radius + offsets or something like that)
• Have SetPopupPosition substract that padding from the coordinates it's sending to the widget code
• Having the size constraints include the new padding

That shouldn't touch any widget code, and would allow box-shadows to be used on the popups.

If you'd like to make this opt-in, adding a value to the CSS -moz-window-shadow property called box-shadow, then checking that in GetShadowSize should work.

The new tab tooltip is facing the same issue on macOS/Linux as well. On macOS it's even worse because the native shadow appears, so you have to disable it using -moz-window-shadow: none (so -moz-window-shadow: box-shadow would totally make sense too even if -moz-window-shadow mac-only atm).

Comment 22

[Tim Nguyen :ntim]

I guess one potential issue is that dynamic shadow changes would need to re-trigger layout, but the front-end approach would also need that for dynamic shadow changes...

For this you could check if box-shadow was changed in nsMenuPopupFrame::DidSetComputedStyle and call LayoutPopup (or a subset).

Alternatively you could allow specifying a fixed padding via a CSS property, then it would just be GetShadowSize changing.

Anyway, that shouldn't be too important, as the current popup shadows aren't dynamically changing (especially when popups are open).

Comment 24

[:Gijs (he/him)]

This is effectively worked around by bug 1699427. Neil, do you want to keep this bug open to investigate making the code more resilient to this kind of thing in future, or should we resolve this as worksforme / fixed?

Comment 25

[Neil Deakin]

A different approach was taken, so this doesn't apply anymore.