Bug 176034 - REGRESSION(r220278): Web Inspector: ContextMenu items are not getting triggered
Summary: REGRESSION(r220278): Web Inspector: ContextMenu items are not getting triggered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-28 13:45 PDT by Joseph Pecoraro
Modified: 2017-08-28 17:29 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.53 KB, patch)
2017-08-28 16:10 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-08-28 13:45:46 PDT
REGRESSION: Web Inspector: Checkbox ContextMenu items are not getting triggered

Steps to Reproduce:
1. Inspect this page
2. Show Console
3. Evaluate something in the console so it is non-empty
4. Right click empty space and "Clear Log"
  => Doesn't work


Steps to Reproduce:
1. Inspect this page
2. Show Network Tab
3. Right click the header and try to toggle any column
  => Doesn't work

Notes:
• InspectorFrontendAPI's contextMenuItemSelected is not getting called.
Comment 1 Joseph Pecoraro 2017-08-28 13:54:00 PDT
This appears to have regressed with: r220278
https://trac.webkit.org/changeset/220278/webkit
Comment 2 Joseph Pecoraro 2017-08-28 14:03:41 PDT
-    if (!ScriptGlobalObject::get(state, "InspectorFrontendAPI", frontendApiObject)) {
+    auto value = state.lexicalGlobalObject()->get(&state, JSC::Identifier::fromString(&state.vm(), "InspectorFrontendHost"));

InspectorFrontendAPI turned into InspectorFrontendHost!
Comment 3 Joseph Pecoraro 2017-08-28 16:09:30 PDT
I spent a bunch of time seeing what it would take to write a test for this. In general it seems we lack tests for selecting context menu items.

There are some LayoutTests to show a ContextMenu that use `eventSender.contextClick`. There are no LayoutTests to select an item in the ContextMenu, which seems okay given there might be platform differences there.

In this case the Web Inspector page, not the inspected test page, is what is triggering context menus. It doesn't have `eventSender`, `internals`, etc. We can show a ContextMenu with `InspectorFrontendHost.showContextMenu`, but selecting an item is not immediately possible. My best idea would be to introduce `InspectorFrontendHost.testContextMenuSelectItemAtIndex` but I don't like that. It adds surface area to an object we want try to keep as small as possible since its APIs extend the power of the Inspector frontend page.
Comment 4 Joseph Pecoraro 2017-08-28 16:10:33 PDT
Created attachment 319215 [details]
[PATCH] Proposed Fix
Comment 5 Devin Rousso 2017-08-28 16:20:32 PDT
Comment on attachment 319215 [details]
[PATCH] Proposed Fix

r=me
Comment 6 WebKit Commit Bot 2017-08-28 17:27:58 PDT
Comment on attachment 319215 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 319215

Committed r221283: <http://trac.webkit.org/changeset/221283>
Comment 7 WebKit Commit Bot 2017-08-28 17:28:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2017-08-28 17:29:16 PDT
<rdar://problem/34122845>