Bug 159028

Summary:

Web Inspector: No open/copy src resource in context menu

Product:

WebKit

Reporter:

Chris Chiera <chris>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2	none
Patch	none
Patch	none

Chris Chiera

Reported 2016-06-22 11:10:57 PDT

In Chromium/Chrome (all versions), you can right click a src="" url in the html box or style box and there are two options in the context menu including, "Copy Link" and "Open Link in a New Tab". This option appears consistently whether in the HTML or Style box. In Safari/Webkit, those options do not exist. In the css box right clicking a src url opens up a context menu with spelling, substitutions, transformations, and other irrelevant options rather than a Copy Link and "Open Link in a New Tab". In the HTML box right clicking a src link provides some relevant options compared to the style box, but does not have an option to copy that link or open it in a new tab (or in the resource center). Right clicking the src link in style box should show relevant contextual items (like html box does), and right clicking src link in HTML or style box should show an option for copying the link and opening the link in a new tab like Chrome.

Attachments
Patch (3.62 KB, patch) 2016-08-20 20:47 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (3.94 KB, patch) 2016-08-22 20:35 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (660.17 KB, application/zip) 2016-08-22 21:56 PDT, Build Bot	no flags	Details
Patch (3.93 KB, patch) 2016-08-23 00:21 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (3.94 KB, patch) 2016-08-23 08:59 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2016-06-22 11:11:39 PDT

<rdar://problem/26947825>

Devin Rousso

Comment 2 2016-08-20 20:47:22 PDT

Created attachment 286550 [details] Patch

Nikita Vasilyev

Comment 3 2016-08-21 14:40:26 PDT

Comment on attachment 286550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286550&action=review > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:666 > + } Works great! > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:677 > + let pseudoSubMenu = contextMenu.appendSubMenuItem(WebInspector.UIString("Forced Pseudo-Classes")); Nit: this seems irrelevant to this patch.

Devin Rousso

Comment 4 2016-08-21 14:58:51 PDT

Comment on attachment 286550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286550&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:677 >> + let pseudoSubMenu = contextMenu.appendSubMenuItem(WebInspector.UIString("Forced Pseudo-Classes")); > > Nit: this seems irrelevant to this patch. Since "let" is preferred almost always over "var", I am trying to convert all instances of "var" to "let" whenever I modify functions.

Joseph Pecoraro

Comment 5 2016-08-22 14:27:00 PDT

Comment on attachment 286550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286550&action=review r- to get a new patch addressing a few of the comments below. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:650 > + if (event.target && event.target.tagName === "A") { What about src/href attributes on things like <script>, <link>, <source>, <video>, <audio>, <iframe>, <frame>, <img>, ...? I think it would be better to check: (1) Attribute names commonly used for links (CodeMirrorAdditions checks: "src", "srcset", "\bhref\b" [covers "href" and "xlink:href"]) (2) In some cases, such as Reveal in Resources, ensure they are real resource links and not actions like "javascript:" or "data:". > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:657 > + contextMenu.appendItem(WebInspector.UIString("Open Link in Resources Panel"), () => { Nit: We should use "Tab" not "Panel". For example we have a pre-existing string "Reveal in Debugger Tab". So perhaps that would be best here. "Reveal in Resources Tab" or "Reveal in Debugger Tab". What if this URL does not appear in the page's Resources? For example, what if I am on bogojoker.com and there is a <a href="http://webkit.org">WebKit</a>. I wouldn't expect "Reveal in Resources Tab" since that resource would not have been loaded on bogojoker.com. In showing this option we should first ensure we have the resource (WebInspector.frameResourceManager.resourceForURL) first. Likewise, we should use the proper frame. DOMNode has a frameIdentifier which we can use to find the WebInspector.Frame. For example, each of these links should reveal the correct script.js, not always the first: <body> <script src="script.js"> <iframe ...> <script src="script.js"> </iframe> </body>

Devin Rousso

Comment 6 2016-08-22 20:35:05 PDT

Created attachment 286660 [details] Patch

Devin Rousso

Comment 7 2016-08-22 20:35:37 PDT

(In reply to comment #5) > > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:650 > > + if (event.target && event.target.tagName === "A") { > > What about src/href attributes on things like <script>, <link>, <source>, > <video>, <audio>, <iframe>, <frame>, <img>, ...? I think it would be better > to check: > > (1) Attribute names commonly used for links (CodeMirrorAdditions checks: > "src", "srcset", "\bhref\b" [covers "href" and "xlink:href"]) > (2) In some cases, such as Reveal in Resources, ensure they are real > resource links and not actions like "javascript:" or "data:". AFAICT, <a> are only added to a DOMTreeElement when the attribute in question is "src", "href", or "srcset" (see DOMTreeElement.prototype._buildAttributeDOM).

Build Bot

Comment 8 2016-08-22 21:56:39 PDT

Comment on attachment 286660 [details] Patch Attachment 286660 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1925222 New failing tests: fast/scrolling/ios/scroll-event-from-scrollTo.html

Build Bot

Comment 9 2016-08-22 21:56:41 PDT

Created attachment 286669 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5

Devin Rousso

Comment 10 2016-08-23 00:21:02 PDT

Created attachment 286686 [details] Patch

Devin Rousso

Comment 11 2016-08-23 08:59:37 PDT

Created attachment 286715 [details] Patch

Joseph Pecoraro

Comment 12 2016-08-23 11:20:52 PDT

(In reply to comment #7) > (In reply to comment #5) > > > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:650 > > > + if (event.target && event.target.tagName === "A") { > > > > What about src/href attributes on things like <script>, <link>, <source>, > > <video>, <audio>, <iframe>, <frame>, <img>, ...? I think it would be better > > to check: > > > > (1) Attribute names commonly used for links (CodeMirrorAdditions checks: > > "src", "srcset", "\bhref\b" [covers "href" and "xlink:href"]) > > (2) In some cases, such as Reveal in Resources, ensure they are real > > resource links and not actions like "javascript:" or "data:". > > AFAICT, <a> are only added to a DOMTreeElement when the attribute in > question is "src", "href", or "srcset" (see > DOMTreeElement.prototype._buildAttributeDOM). Oops, I was thinking the target was a DOMNode here.

Joseph Pecoraro

Comment 13 2016-08-23 11:23:32 PDT

Comment on attachment 286715 [details] Patch r=me

WebKit Commit Bot

Comment 14 2016-08-23 12:02:31 PDT

Comment on attachment 286715 [details] Patch Clearing flags on attachment: 286715 Committed r204846: <http://trac.webkit.org/changeset/204846>

WebKit Commit Bot

Comment 15 2016-08-23 12:02:35 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.