Bug 149155

Summary: Web Inspector: Copying inline style text puts "undefined" in the pasteboard
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Devin Rousso
Reported 2015-09-15 01:31:28 PDT
* STEPS TO REPRODUCE: 1. Go to https://www.apple.com/ 2. Open the elements tab in Web Inspector 3. Select the <body> tag 4. Right click the "Style Attribute" text at the top of the Styles sidebar and select "Copy Rule" 5. Paste somewhere (Cmd+V) Expected: body { } Actual: undefined
Attachments
Patch (3.73 KB, patch)
2015-09-15 01:34 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (8.93 KB, patch)
2015-10-01 12:42 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (10.30 KB, patch)
2015-10-01 23:12 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-09-15 01:31:44 PDT
<rdar://problem/22698991>
Devin Rousso
Comment 2 2015-09-15 01:34:45 PDT
Created attachment 261179 [details] Patch
Blaze Burg
Comment 3 2015-09-15 10:19:43 PDT
Comment on attachment 261179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261179&action=review This approach definitely works, but it could be simpler, IMO. What do you think about adding getters to the CSSStyleDeclaration for mediaList and selectorText? Then you can avoid adding another special case parameter and hide that logic in the getter. let selectorText = this.selectorText; if (!selectorText) return; let styleText = ""; let mediaQueriesCount = 0; let mediaList = this.mediaList; if (mediaList && mediaList.length) { mediaQueriesCount = mediaList.length; for (let i = mediaQueriesCount - 1; i >= 0; --i) styleText += `${" ".repeat(mediaQueriesCount - i - 1)}@media ${mediaList[i].text} {\n`; } styleText += `${" ".repeat(mediaQueriesCount)}${selectorText} {\n`; > Source/WebInspectorUI/ChangeLog:7 > + It would be helpful to describe why it wasn't working before, and how you fixed it.
Blaze Burg
Comment 4 2015-09-15 10:22:19 PDT
Also, this behavior (what the generated string is for an inline style/rule) is definitely something for which we should write a test.
Devin Rousso
Comment 5 2015-10-01 12:42:34 PDT
Created attachment 262278 [details] Patch
Timothy Hatcher
Comment 6 2015-10-01 13:02:43 PDT
Comment on attachment 262278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262278&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:246 > + No need for the empty line here. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:254 > + Ditto. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:311 > + styleText += " ".repeat(mediaQueriesCount - i - 1) + "@media " + mediaList[i].text + " {\n"; Might be nice to put the " " in a local that would eventuality be controlled by a tab/spaces setting. > LayoutTests/inspector/css/generate-css-rule-string.html:30 > + InspectorTest.completeTest(); This should use the new test suite way for inspector tests.
Devin Rousso
Comment 7 2015-10-01 13:30:19 PDT
Comment on attachment 262278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262278&action=review >> LayoutTests/inspector/css/generate-css-rule-string.html:30 >> + InspectorTest.completeTest(); > > This should use the new test suite way for inspector tests. So I am a bit confused on this. It seems like the new suite uses InspectorTest.expectThat and if we are planning on supporting different tab widths for indentation in the future, how would I ensure that it prints correctly? Also, are we entirely using InspectorTest.expectThat or am I able to use InspectorTest.log?
Joseph Pecoraro
Comment 8 2015-10-01 14:25:04 PDT
> if we are planning on supporting different tab widths for indentation in the > future, how would I ensure that it prints correctly? Different tab widths would be a user preference. Tests would always use the default value of settings / preferences (or set to some non-default). Much like showing the shadow dom is a setting. > Also, are we entirely using InspectorTest.expectThat or am I able to use InspectorTest.log? You may use both. "expect" is nice because it prints out PASS / FAIL along with the message. With messages written of the form "X should be Y", if you get a Failure you know immediately what went wrong. X should be Y, but it wasn't.
Blaze Burg
Comment 9 2015-10-01 14:27:25 PDT
Comment on attachment 262278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262278&action=review >>> LayoutTests/inspector/css/generate-css-rule-string.html:30 >>> + InspectorTest.completeTest(); >> >> This should use the new test suite way for inspector tests. > > So I am a bit confused on this. It seems like the new suite uses InspectorTest.expectThat and if we are planning on supporting different tab widths for indentation in the future, how would I ensure that it prints correctly? Also, are we entirely using InspectorTest.expectThat or am I able to use InspectorTest.log? See here for rationale: http://trac.webkit.org/wiki/WebInspectorTests#HowtoWriteTests
Devin Rousso
Comment 10 2015-10-01 23:12:57 PDT
Created attachment 262317 [details] Patch
Devin Rousso
Comment 11 2015-10-01 23:14:20 PDT
(In reply to comment #10) > Created attachment 262317 [details] > Patch So I tried updating the test to use the new suite, but I'm not sure if I did it right so please let me know if I need to change anything.
Blaze Burg
Comment 12 2015-10-02 07:47:06 PDT
Comment on attachment 262317 [details] Patch Test looks good to me.
WebKit Commit Bot
Comment 13 2015-10-02 15:33:28 PDT
Comment on attachment 262317 [details] Patch Clearing flags on attachment: 262317 Committed r190528: <http://trac.webkit.org/changeset/190528>
WebKit Commit Bot
Comment 14 2015-10-02 15:33:32 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.