Summary: | Web Inspector: No context menu available via ENABLE_INSPECTOR_SERVER | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, Hironori.Fujii, inspector-bugzilla-changes, joepeck | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Ross Kirsling
2017-04-10 16:07:11 PDT
Created attachment 306752 [details]
Proposed context menu
Created attachment 306755 [details]
Patch
This patch reintroduces the old SoftContextMenu class and applies various fixes: - Remove legacy globals and prototype extensions. - Align JS/CSS style with current front-end code. - Update UI and fix UX to replicate the macOS native context menu. To verify without ENABLE_INSPECTOR_SERVER, simply "opt-in" to the non-native context menu by commenting out the conditional at SoftContextMenu.js:377. The appearance and behavior should match the native context menu (exceptions being the lack of a native-backed Inspect Element option and the fact that the menu itself can be right-clicked upon). Comment on attachment 306755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306755&action=review r=me with comments. After playing with it I found a few usability issues: 1. You should not be able to right click the SoftContextMenu and get another native menu! => You can detect clicks and preventDefault if its a right click (but still allow the original quick selection path) 2. Keyboard events do not work => I have comments below that get this working. Maybe you can address (1) in a follow-up. It looks very very close to native! > Source/WebInspectorUI/UserInterface/Base/InspectorFrontendHostStub.js:-168 > - showContextMenu: function(event, menuObject) > - { > - }, > - You should keep this: showContextMenu: function(event, menuObject) { new WebInspector.SoftContextMenu(items).show(event); }, We want the Stub to match InspectorFrontendHost.idl, if someone saw something missing they might add back a stub and break the conditional you added later. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:35 > + > + show(event) Style: Put a divider comment after the constructor and before here: // Public See: https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:39 > + // Create context menu. Nit: We can drop this comment, the code is clear. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:46 > + for (let i = 0; i < this._items.length; ++i) > + this._contextMenuElement.appendChild(this._createMenuItem(this._items[i])); Style: Use for..of: for (let item of this._items) this._contextMenuElement.appendChild(this._createMenuItem(item)); > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:60 > + this._repositionMenuOnScreen(isSubMenu); In order to make keyboard events work I had to add a few lines here: this._contextMenuElement.tabIndex = -1; this._contextMenuElement.focus(); > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:62 > + Style: Put a divider comment here: // Private > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:86 > + if (item.type === "separator") Ideally we would have an enum WebInspector.ContentMenu.MenuItemType.Separator, .SubMenu, etc. But since that didn't exist already what you have here is fine. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:96 > + const checkMarkElement = document.createElement("span"); > + // Unicode "CHECK MARK" > + checkMarkElement.textContent = item.checked ? "\u2713" : ""; Style: We would handle this now with const variables to avoid the comments: const checkmark = "\u2713"; const blackRightPointingTriangle = "\u25b6"; const checkMarkElement = document.createElement("span"); checkMarkElement.textContent = item.checked ? checkmark : ""; ... > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:116 > + } else { > + menuItemElement._actionId = item.id; > + } > + Style: Drop braces for a single statement block. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:306 > + this._highlightedMenuItemElement._subMenuTimer = > + setTimeout(this._showSubMenu.bind(this, this._highlightedMenuItemElement), 150); Style: Do not wrap this line. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:313 > + let menuItemElement = this._highlightedMenuItemElement ? > + this._highlightedMenuItemElement.previousSibling : this._contextMenuElement.lastChild; Style: Do not wrap this line. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:325 > + let menuItemElement = this._highlightedMenuItemElement ? > + this._highlightedMenuItemElement.nextSibling : this._contextMenuElement.firstChild; Style: Do not wrap this line. > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:377 > +if (!InspectorFrontendHost.showContextMenu) { Just keep this in the InspectorFrontendHostStub, see the earlier comment. Created attachment 307072 [details]
Patch
Feedback addressed, including squelching of native Inspect Element. Comment on attachment 307072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307072&action=review Excellent! r=me > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.css:3 > + * Copyright (C) 2017 Sony Interactive Entertainment Inc. Can you add this line to $inspectorLicense in Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl That is the copyright included appended to minified builds. I normally roll it every now and then but since I remembered now we should add this proactively! > Source/WebInspectorUI/UserInterface/Views/SoftContextMenu.js:195 > + // Squelch native Inspect Element. I don't think "Squelch" is clear terminology. Comment on attachment 307072 [details] Patch Attachment 307072 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3531908 New failing tests: webrtc/multi-video.html Created attachment 307083 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 307085 [details]
Patch
Feedback addressed. That test breakage shouldn't be my fault? Looks like this change just landed: https://bugs.webkit.org/show_bug.cgi?id=170796 Comment on attachment 307085 [details]
Patch
r=me, would you like me to cq+ this?
That would be great! :) Comment on attachment 307085 [details] Patch Clearing flags on attachment: 307085 Committed r215357: <http://trac.webkit.org/changeset/215357> All reviewed patches have been landed. Closing bug. |