Closed Bug 1543940 Opened 5 months ago Closed 4 months ago

DevTools context menus are broken when running in frames with type=content

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(6 files)

STRs:

  • set content="type" for devtools frames (in toolbox-hosts::createDevToolsFrame() add frame.setAttribute("type", "content");)
  • textbox context menus are not displayed, keyboard navigation in context menus is not working, some context menus break after swapping host...

Blocker for 1539979

I have several WIP patches to address those issues which I'll try to clean up and land here.

Small update on the test status, rebased on a recent central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e29a45ace0ed15e4da4274e87dfc6127e6d8d352

On opt build we still have 1 intermittent failure:

  • devtools/client/performance/test/browser_perf-overview-render-02.js

On debug builds, two tests are leaking:

  • devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_devtoolstoolbox_focus.js
  • devtools/client/debugger/test/mochitest/browser_dbg-xhr-breakpoints.js

(+ webextension test failure)

One of the issues we have with context menus when using type="content" is that they are not automatically focused for keyboard navigation. We create the popup at https://searchfox.org/mozilla-central/source/devtools/client/framework/menu.js#98.

We can emulate this behavior by focusing manually the document hosting the contextmenu (and restoring the focus after the menu if hidden):

diff --git a/devtools/client/framework/menu.js b/devtools/client/framework/menu.js
--- a/devtools/client/framework/menu.js
+++ b/devtools/client/framework/menu.js
@@ -104,23 +104,26 @@ Menu.prototype.popup = function(screenX,
     popup.id = this.id;
   }
   this._createMenuItems(popup);
 
   // Remove the menu from the DOM once it's hidden.
   popup.addEventListener("popuphidden", (e) => {
     if (e.target === popup) {
       popup.remove();
+      this.active.focus();
       this.emit("close");
     }
   });
 
   popup.addEventListener("popupshown", (e) => {
     if (e.target === popup) {
       this.emit("open");
+      this.active = toolbox.doc.activeElement;
+      toolbox.doc.documentElement.focus();
     }
   });
 
   popupset.appendChild(popup);
   popup.openPopupAtScreen(screenX, screenY, true);
 };
 
 Menu.prototype._createMenuItems = function(parent) {

or avoid the issue altogether by creating the context menu in the parent chrome document, accessing it via win.windowRoot.ownerGlobal.document. This second option has a huge impact on many of our tests however.

I am not sure why the behavior is different for frames with type=content though, maybe that's something we should change on the platform side. Neil, do you have a suggestion here?

Flags: needinfo?(enndeakin)

(In reply to Julian Descottes [:jdescottes] from comment #2)

One of the issues we have with context menus when using type="content" is that they are not automatically focused for keyboard navigation. We create the popup at https://searchfox.org/mozilla-central/source/devtools/client/framework/menu.js#98.

We can emulate this behavior by focusing manually the document hosting the contextmenu (and restoring the focus after the menu if hidden):
[...]

Actually a dead end. Trying to force the focus manually is not good enough, as it will force a blur on the previously selected element. This is not the case with the default behavior, and we would like to preserve this. That means the only valid workaround I have so far is to create the context menus in the topmost chrome document.

Blocks: 1544692
See Also: → 1544709
Depends on: 1544709

Depends on D27688
Using chromEventHandler will allow us to catch events fired from any frame.
By default when DevTools are in a type=chrome frame, events also bubble across frames.
With type=content this is no longer the case.

Depends on D27693

If a context menu is opened in the toolbox document when running in a frame with type=content, keyboard navigation will not move to the context menu when it's opened.

You have a structure like:

<toplevel>
<document1 type="content">
<document2>

Where the popup is defined in document1, but you have something in document2 focused. So key events will only fire on document2 and toplevel. If you remove type="content" on that frame, key events will fire on all three documents as they are all chrome documents.

I'm not sure where this distinction is handled, or if the chromeEventHandler can influence this. Maybe smaug would know more.

Flags: needinfo?(enndeakin) → needinfo?(bugs)

https://searchfox.org/mozilla-central/rev/d33d470140ce3f9426af523eaa8ecfa83476c806/dom/base/nsGlobalWindowInner.cpp#1861,1889-1890 is the part which makes events from a window to propagate to chrome event handler (possibly via message manager)

Flags: needinfo?(bugs)

Depends on D27693

Menu::popup and popupAtZoom are expecting a toolbox argument as last argument.
However, half of the callsites do not have access to the toolbox and just pass
a { doc } object. This is misleading when trying to work on menu.js because you
cannot rely on toolbox APIs.

Attachment #9059217 - Attachment description: Bug 1543940 - Tests for context-menu should use toolbox.topDoc → Bug 1543940 - Apply devtools framework menu changes to Debugger menu;
Blocks: 1548802
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fe9661e8510
Rely on chromeEventHandler for toolboxContextMenu events r=ochameau
https://hg.mozilla.org/integration/autoland/rev/b1e6e932873c
menu.popup() should take a document argument instead of toolbox r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a66967f17042
Use the toolbox top window for context menus r=ochameau
https://hg.mozilla.org/integration/autoland/rev/42e2136f684f
Update tests to use toolbox.topDoc to query context-menu elements r=ochameau
https://hg.mozilla.org/integration/autoland/rev/590b72185b69
Remove useTopLevelWindow option from DevTools menu/utils.js helper r=ochameau
https://hg.mozilla.org/integration/autoland/rev/aaa0f827616a
Apply devtools framework menu changes to Debugger menu; r=jlast

Backed out 1 changesets (Bug 1543940) for mochitest failure at devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=aaa0f827616a64aa8c5b347c83f9423fdaa915a7

Log failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245365991&repo=autoland&lineNumber=6661

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=245365991&revision=6986b4b5c3db2c68d9b8d694d942c01a265a1afd

16:27:53     INFO - TEST-START | devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html
16:27:53     INFO - GECKO(915) | ++DOMWINDOW == 47 (0x12ecb7800) [pid = 915] [serial = 47] [outer = 0x13623b4c0]
16:27:53     INFO - TEST-INFO | started process screencapture
16:27:53     INFO - TEST-INFO | screencapture: exit 0
16:27:53     INFO - Buffered messages logged at 16:27:53
16:27:53     INFO - Check contextmenu default behaviour.
16:27:53     INFO - Buffered messages finished
16:27:53     INFO - TEST-UNEXPECTED-FAIL | devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html | Accessibility row context menu is open 
16:27:53     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
16:27:53     INFO - window.onload/<@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:119:7
16:27:53     INFO - withMockEnv@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:43:13
16:27:53     INFO - window.onload@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:113:11
16:27:53     INFO - EventHandlerNonNull*@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:23:1
16:27:53     INFO - Not taking screenshot here: see the one that was previously logged
16:27:53     INFO - TEST-UNEXPECTED-FAIL | devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html | Print to JSON menu item is visible 
16:27:53     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
16:27:53     INFO - window.onload/<@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:120:7
16:27:53     INFO - withMockEnv@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:43:13
16:27:53     INFO - window.onload@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:113:11
16:27:53     INFO - EventHandlerNonNull*@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:23:1
16:27:53     INFO - Not taking screenshot here: see the one that was previously logged
16:27:53     INFO - TEST-UNEXPECTED-FAIL | devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html | Got an error: TypeError: printtojsonMenuItem is null
16:27:53     INFO - Stack: window.onload/<@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:134:7
16:27:53     INFO - withMockEnv@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:43:13
16:27:53     INFO - window.onload@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:113:11
16:27:53     INFO - EventHandlerNonNull*@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:23:1
16:27:53     INFO - Line: 134, column: 7 
16:27:53     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
16:27:53     INFO - window.onload@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:163:5
16:27:53     INFO - async*@chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html:23:1
16:27:53     INFO - GECKO(915) | MEMORY STAT | vsize 5441MB | residentFast 373MB | heapAllocated 147MB
16:27:53     INFO - TEST-OK | devtools/client/accessibility/test/mochitest/test_accessible_row_context_menu.html | took 407ms
16:27:53     INFO - GECKO(915) | [915, Main Thread] WARNING: NS_ENSURE_TRUE(weakFrame.IsAlive()) failed: file /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp, line 1061
16:27:53     INFO - GECKO(915) | ++DOMWINDOW == 48 (0x12ecbac00) [pid = 915] [serial = 48] [outer = 0x13623b4c0]
16:27:53     INFO - TEST-START | Shutdown
16:27:53     INFO - Passed:  28
16:27:53     INFO - Failed:  3
16:27:53     INFO - Todo:    0
16:27:53     INFO - Mode:    non-e10s
16:27:53     INFO - Slowest: 1959ms - chrome://mochitests/content/chrome/devtools/client/accessibility/test/mochitest/test_accessible_contrast.html
16:27:53     INFO - SimpleTest FINISHED
16:27:53     INFO - TEST-INFO | Ran 1 Loops
16:27:53     INFO - SimpleTest FINISHED
Flags: needinfo?(jdescottes)

sorry about that! Looks like we had one chrome mochitest that still needs to be updated.

Flags: needinfo?(jdescottes)
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a51c08b24de3
Backed out 5 changesets for failing browser_menu_api.js on a CLOSED TREE

There was another failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245372313&repo=autoland&lineNumber=21488

That second failure is harder to understand. It seems that the test browser_menu_api.js is creating two menu items with L10n IDs "editmenu-copy" and "editmenu-undo" but the FTL necessary to translate those strings is not inserted in the window.

Even on the current central, running this test will not show anything for those two menu items. I guess the localization file was always missing, but for some reason, the translation failure only makes the test fail on Windows and if the context menu is created in the topmost chrome window.

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0484e2e8f7fd
Rely on chromeEventHandler for toolboxContextMenu events r=ochameau
https://hg.mozilla.org/integration/autoland/rev/388f61b1b134
menu.popup() should take a document argument instead of toolbox r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a0768b78ff32
Use the toolbox top window for context menus r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6907e6f93141
Update tests to use toolbox.topDoc to query context-menu elements r=ochameau
https://hg.mozilla.org/integration/autoland/rev/8fa4f7a2ab42
Remove useTopLevelWindow option from DevTools menu/utils.js helper r=ochameau
https://hg.mozilla.org/integration/autoland/rev/eea9b4ad1dac
Apply devtools framework menu changes to Debugger menu; r=jlast
Depends on: 1550827
Regressions: 1570583
You need to log in before you can comment on or make changes to this bug.