DevTools context menus are broken when running in frames with type=content
Categories
(DevTools :: General, defect, P2)
Tracking
(firefox68 fixed)
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
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.
|
Assignee | |
Comment 1•5 years ago
|
||
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)
|
|
Assignee | |
Comment 2•5 years ago
|
||
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?
|
|
Assignee | |
Comment 3•5 years ago
|
||
(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.
|
|
Assignee | |
Comment 4•5 years ago
|
||
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.
|
|
Assignee | |
Comment 5•5 years ago
|
||
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.
|
|
Assignee | |
Comment 6•5 years ago
|
||
Depends on D27695
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
||
Comment 8•5 years ago
|
||
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.
|
||
Comment 9•5 years ago
|
||
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)
|
|
Assignee | |
Comment 10•5 years ago
|
||
Depends on D27696
|
|
Assignee | |
Comment 11•5 years ago
|
||
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.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 12•5 years ago
|
||
|
|
Assignee | |
Comment 13•5 years ago
|
||
Depends on D27696
|
||
Comment 14•5 years ago
|
||
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
|
||
Comment 15•5 years ago
|
||
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
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
|
|
Assignee | |
Comment 16•5 years ago
|
||
sorry about that! Looks like we had one chrome mochitest that still needs to be updated.
|
|
||
Comment 17•5 years ago
|
||
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
|
||
Comment 18•5 years ago
|
||
Backed out the other patches too.
There was another failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245372313&repo=autoland&lineNumber=21488
Backout: https://hg.mozilla.org/integration/autoland/rev/a51c08b24de339dea7f16b9f8a6f37acf0c0ed6c
|
|
Assignee | |
Comment 19•5 years ago
|
||
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.
|
|
Assignee | |
Comment 20•5 years ago
|
||
Let's see if adding the FTL fixes the windows debug failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36209471a34ca21e013c153c46c1462ccab20c80
|
|
||
Comment 21•5 years ago
|
||
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
|
||
Comment 22•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/0484e2e8f7fd
https://hg.mozilla.org/mozilla-central/rev/388f61b1b134
https://hg.mozilla.org/mozilla-central/rev/a0768b78ff32
https://hg.mozilla.org/mozilla-central/rev/6907e6f93141
https://hg.mozilla.org/mozilla-central/rev/8fa4f7a2ab42
https://hg.mozilla.org/mozilla-central/rev/eea9b4ad1dac
Description
•