Bug 138812

Summary:

Web Inspector: Ability to Copy entire CSS Rule from Styles Sidebar

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, graouts, joepeck, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Joseph Pecoraro

Reported 2014-11-17 16:25:26 PST

* SUMMARY It would be helpful to copy an entire CSS rule from the sidebar. Currently each Rule's Selector and Properties are separate fields, it would be nice to be able to copy/paste the entire rule (Selector, Properties, and optionally surrounding @media rules block) from the Styles sidebar. * TEST: <style> @media (min-width:600px) { body { color: red; } } </style> <body>Test</body> * STEPS TO REPRODUCE 1. Inspect <body> on test page 2. Show Styles sidebar => Should be able to copy the body styles from the sidebar somehow * NOTES - Would be nice to copy with and without the @media. - Perhaps right clicking anywhere in the selector/property editors?

Attachments
Patch (3.73 KB, patch) 2015-06-04 10:41 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (3.96 KB, patch) 2015-06-04 14:13 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (3.94 KB, patch) 2015-06-04 16:19 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-11-17 16:25:45 PST

<rdar://problem/19009055>

Timothy Hatcher

Comment 2 2014-11-19 10:44:23 PST

Maybe if the entire selection range of the rule is copied, include the selector too?

Devin Rousso

Comment 3 2015-06-04 10:41:16 PDT

Created attachment 254272 [details] Patch

Timothy Hatcher

Comment 4 2015-06-04 10:46:50 PDT

Comment on attachment 254272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254272&action=review I think a context menu on the rule section would be best. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.css:70 > + cursor: pointer; Not sure pointer is the right cursor. This might be best as a context menu item. Click to Copy isn't a typical user action in most places. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:110 > + (function(thisObject) { > + thisObject._iconElement.addEventListener("click", function() { You should use .bind(this) on the event listener function instead. No need for a closure.

Joseph Pecoraro

Comment 5 2015-06-04 12:48:39 PDT

Comment on attachment 254272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254272&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:119 > + styleText += "@media" + thisObject._style.ownerRule.mediaList[0].text + " {\n "; Seems weird that this would always be mediaList[0]. Does that make sense?

Devin Rousso

Comment 6 2015-06-04 14:13:15 PDT

Created attachment 254299 [details] Patch

Joseph Pecoraro

Comment 7 2015-06-04 15:00:08 PDT

Comment on attachment 254299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254299&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:108 > + this._headerElement.addEventListener("contextmenu", function(event) { Style: We would probably have a _handleContextMenuEvent method down below and here we would just have: this._headerElement.addEventListener("contextmenu", this._handleContextMenuEvent.bind(this)); > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:110 > + if (window.getSelection().toString().length) > + return; What is this bail for? > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:115 > + InspectorFrontendHost.copyText(this._generateCSSRuleString.call(this)); Nit: this.foo.call(this) is no different than this.foo(); > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:293 > + _generateCSSRuleString: function() { Style: Brace on new line. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:301 > + function repeatText(text, numRepeats) { > + var output = ""; > + > + for (var i = 0; i < numRepeats; ++i) > + output += text; > + > + return output; > + } Actually, we support String.prototype.repeat. So you could just do: "test".repeat(3) And get: "testtesttest" > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:312 > + for (var i = numMediaQueries - 1; i >= 0; --i) > + styleText += repeatText(" ", numMediaQueries - i - 1) + "@media " + mediaList[i].text + " {\n"; I wonder if this would be a good place to adopt template strings: styleText += `${" ".repeat(numMediaQueries - i - 1)} @media ${mediaList[i].text} {\n` But maybe I'm just getting too excited. Stick with just strings for now. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:325 > + if (styleText.slice(-1) !== ";") > + styleText += ";"; I think: !styleText.endsWith(";") is more expressive.

Devin Rousso

Comment 8 2015-06-04 16:11:21 PDT

Comment on attachment 254299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254299&action=review >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:110 >> + return; > > What is this bail for? If part of the selector text is highlighted, don't replace the context menu as someone may want to just copy/paste a selector.

Devin Rousso

Comment 9 2015-06-04 16:19:01 PDT

Created attachment 254311 [details] Patch

WebKit Commit Bot

Comment 10 2015-06-18 13:44:44 PDT

Comment on attachment 254311 [details] Patch Clearing flags on attachment: 254311 Committed r185720: <http://trac.webkit.org/changeset/185720>

WebKit Commit Bot

Comment 11 2015-06-18 13:44:50 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.