Closed Bug 1700724 Opened 4 years ago Closed 4 years ago

Green up tests with native context menus enabled

Categories

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

All
macOS
task

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 --- unaffected
firefox-esr78 --- unaffected
firefox87 --- unaffected
firefox88 --- unaffected
firefox89 --- fixed
firefox90 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: [proton-context-menus][mac:mr1])

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.

Type: defect → task

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

Assignee: nobody → mstange.moz
Status: NEW → ASSIGNED
Keywords: helpwanted

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.
Blocks: 1702664
Depends on: 1703333
Depends on: 1703419
Depends on: 1703422
Depends on: 1703428
Depends on: 1703930
Depends on: 1704127
Depends on: 1704206
Depends on: 1704209
Depends on: 1704211
Depends on: 1704212
Depends on: 1704215
Depends on: 1704216

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.

Depends on: 1704554
Depends on: 1704569
Depends on: 1704572

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.
Depends on: 1704628
No longer depends on: 1704216
Depends on: 1704682
Depends on: 1704691
Depends on: 1704879
Depends on: 1704885
Depends on: 1704888
Depends on: 1704900
Depends on: 1704902
Depends on: 1704904
Depends on: 1704912
Depends on: 1704925
Depends on: 1704935
Depends on: 1704936
Depends on: 1704938
Depends on: 1704950
Depends on: 1704972
Depends on: 1702633
Depends on: 1705102
Depends on: 1705103
Depends on: 1705120
Depends on: 1705142
Depends on: 1705205
Depends on: 1705207
Depends on: 1705213
Depends on: 1705216
Depends on: 1705218
Depends on: 1705221
Depends on: 1705226
Depends on: 1705240
Depends on: 1705245
Depends on: 1705253
Depends on: 1705257
Depends on: 1705451
Depends on: 1705544
Depends on: 1705582
Depends on: 1705801
Depends on: 1705838
Depends on: 1705842
Depends on: 1706127
Depends on: 1706178
Depends on: 1706389
Depends on: 1706434
Depends on: 1706454
Depends on: 1706462
Depends on: 1706463
Depends on: 1706472
Depends on: 1706483
Depends on: 1706490
Depends on: 1706502
No longer depends on: 1706502
Depends on: 1706725
Depends on: 1706735
Depends on: 1706750
Depends on: 1706756
Depends on: 1706775
Depends on: 1706788
Depends on: 1706789
Depends on: 1706806
Depends on: 1706816
Depends on: 1706834
No longer blocks: 1702664
No longer depends on: 1705120
No longer depends on: 1703428

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Depends on: 1707598
You need to log in before you can comment on or make changes to this bug.