1700724 - Green up tests with native context menus enabled

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, task, P1)

Description

[Markus Stange [:mstange]]

Our tests are currently run with non-native context menus, because native context menus are behind prefs. We need to find out which tests fail if the prefs are flipped, and fix the tests (or the bugs), so that we can enable the prefs by default (bug 1700679) without causing the tree to go orange.

Comment 1

[Markus Stange [:mstange]]

I'm going to kick off a try build for this today.

Comment 2

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=7e137032516d7582bf5fbfbf40fdf4f7056d20c4

Comment 3

[Markus Stange [:mstange]]

Edit: This comment is no longer accurate, see comment 7.

My plan for this is the following:

In this bug, change all affected tests to set widget.macos.native-context-menus to false. This will keep the tree green even as the default is flipped to true, and it will retain existing test coverage. However, it means that tests will be running under a different configuration from what we're shipping to users.

Then, in follow-up bugs:

1. Make sure it's easy to write tests that work with native context menus. For example, some tests open a context menu and then use menuitem.click(). Maybe we can make that work for native context menus so that existing tests just work. In any case, there will need to be some kind of API to open a native context menu and activate an item in it.
2. Incrementally convert tests to use the agnostic APIs so that they no longer need to override the pref.

Comment 4

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=579b89544f3e7ebb46ec0808527b920ca79ed161

Comment 5

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=25a6f5951d7535510f58e614f544d072beef8a27

Comment 6

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&tier=1&revision=792f9df661219ccf694f5af3e78a4303803bb33b

Comment 7

[Markus Stange [:mstange]]

Due to the large number of test failures (initially around 293) and based on reviewer feedback in bug 1703428, I am changing the approach here.
I will try to get as many tests as possible passing with native context menus enabled, before flipping the pref. This bug's dependencies will help with that.

I am tracking progress in this spreadsheet.

Comment 8

[Markus Stange [:mstange]]

I am asking for help with the remaining test failures. If you want to help, please proceed as follows:

1. Apply the current patch stack (currently going up to D111684) locally on macOS. The most reliable way to do this seems to be via the tryserver: hg pull -r 72cb117f99f8a32f1d7e96dd91dd388b4ea352f0 https://hg.mozilla.org/try/ && hg up tip
2. Build.
3. Select a row without an assignee in this spreadsheet.
4. Put your name in the assignee column.
5. Confirm the test passes locally with non-native menus: ./mach mochitest testpath. If it doesn't, ping me.
6. Run the test locally with native menus: ./mach mochitest testpath --setpref widget.macos.native-context-menus=true
7. If it passes, mark the row as "Green with current patch stack: Yes" and go to step 3.
8. If it fails, find out why.

Backchannel is #macdev on Matrix.

So far, I've seen the following reasons for failures:

1. The test synthesizes mouse clicks on menu items to activate them (bug 1704211). Remedy: Tweak the test to call menu.activateItem(menuitem) instead. See the patches in bug 1704569 for many examples.
2. The test synthesizes mouse clicks on a menu item with a submenu to open the submenu. Remedy: Tweak the test to call menu.openMenu(true) instead. See the patches in bug 1704569 for many examples.
3. The test assumes certain popup events to be synchronous. Remedy: Listen for popupshown / popuphidden events as necessary. See bug 1704572 for some examples.
4. The test synthesizes an ESC key event to close a menu. Remedy: Tweak the test to call menupopup.hidePopup() instead.
5. The test checks menu implementation details that cannot be checked with native menus. Remedy: Turn off that part of the test if widget.macos.native-context-menus is true.
6. The test expects a submenu's open/close state to be reflected in submenupopup.state. This is bug 1704554 and I don't currently have a patch for it. Remedy: Set widget.macos.native-context-menus is false during the test, as a temporary workaround.

To find the cause for the test failure, I've used the following approach:

1. Open the test JS file in your editor and search for "contextmenu". If found, see if the test calls EventUtils.synthesizeMouseAtCenter for elements inside the menu. If it does, you probably have failure scenarios 1 or 2. Try the simple replacements and see if the test passes now. Also make sure that the test still passes with non-native menus.
2. If the failure cause isn't immediately obvious, use the profiler to compare a good run with a bad run. Run ./mach mochitest testpath --profiler to obtain a profile for the good run, and ./mach mochitest testpath --profiler --setpref widget.macos.native-context-menus=true to obtain a profile of the bad run. You may need to manually quit the browser during the bad run. Now go to the marker chart of the parent process main thread and look for "popup"-related DOMEvents, and use the markers in the "Test" category (scroll down) to orient yourself.

Comment 9

[Markus Stange [:mstange]]

We're green! https://treeherder.mozilla.org/jobs?repo=try&tier=1&revision=f5fb598b464533063acbe81e6241b0c526571d57

Thanks a ton to everyone involved in this effort, for maintaining the spreadsheet, debugging, writing fixes and doing reviews!

We fixed about 300 tests. The tests helped us find three issues which could be experienced by users: WebExtension tests found a bug with missing items in the sidebar bookmarks context menu (bug 1704127) and pointed out the requirement to handle dynamic changes in menu contents while the menu is open (bug 1705842) and the requirement to be able to respond differently based on which mouse button was used to click the menuitem (bug 1704883). The latter two requirements were initially considered out of scope.

We also made a number of changes to the native menu implementation, to allow tests more control over and insight into the state of native menus, and to comply better with existing assumptions about menu behavior in tests. As far as I'm aware, none of those changes were needed to fix bugs that users could experience; they were needed to retain test coverage which helps with product robustness.

The large majority of changes was to tests themselves, switching them to APIs which are agnostic of the menu implementation (i.e. which work with both native and non-native menus).