1415627 - Reflows in menus cause e10s <select> menupopups to be repositioned

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Markus Stange [:mstange]]

This bug is the underlying bug behind bug 1414926.

Steps to reproduce:
 1. Be on macOS.
 2. Apply the patch at the end of this comment.
 3. Visit https://www.mozilla.org/en-US/newsletter/
 4. Click on the <select> input to choose your country.
 5. Scroll to either the top or bottom of the range of <options> (arrow keys work well to reproduce)

Expected results:
The <select> input should stay put when you reach the end of an <options> list.

Actual results:
The <select> input moves around the screen in an amusing way.


Patch:

diff --git a/toolkit/themes/osx/global/menu.css b/toolkit/themes/osx/global/menu.css
--- a/toolkit/themes/osx/global/menu.css
+++ b/toolkit/themes/osx/global/menu.css
@@ -129,16 +129,17 @@ menubar > menu[_moz-menuactive="true"][o
 menupopup > menu,
 menupopup > menuitem,
 menupopup > menucaption {
   max-width: 42em;
 }

 menu[_moz-menuactive="true"],
 menuitem[_moz-menuactive="true"] {
+  padding-right: 1px;
   -moz-font-smoothing-background-color: -moz-mac-active-menuitem;
   color: -moz-mac-menutextselect;
   background-color: Highlight;
 }

 menuitem[customoptionstyling="true"] {
   -moz-appearance: none;
   padding-top: 0;

Comment 1

[Neil Deakin]

This issue here is that on Mac the menu is normally positioned so that selection is over the dropdown button, but that adjustment should only happen when the menu is first opened.

Comment 2

[Neil Deakin]

Attachment: menulist-adjustpos-once

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8930902 [details] [diff] [review]
menulist-adjustpos-once

Review of attachment 8930902 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. Thanks for doing this! I'm curious, does the test fail without this patch, even without an artificial CSS tweak?

Comment 4

[Neil Deakin]

Yes, it fails.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2ab0c45233
only calculate the offset that positions the current selection over the menulist button when the popup is first opened. After that use that cached value. r=mstange

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9f2ab0c45233

Comment 7

[Julien Cristau [:jcristau]]

Do we need this fix in 58 or should it ride the trains to 59?

Comment 8

[Neil Deakin]

My understanding is that the fix for 1414926 means that this bug isn't noticeable normally.

However when I run the test changes here without the C++ changes (modified to run slower), I can see the menu flickering about a bit although it always end up in the right position. With all changes, the menu doesn't flicker. So this patch improves things, regardless of 1414926 and I don't think the patch is particularly risky.

Asking Marcus if he thinks there is an issue here.

Comment 9

[Julien Cristau [:jcristau]]

Getting late for 58 now, we only have a couple beta builds left in January.