* 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
<rdar://problem/22698991>
Created attachment 261179 [details] Patch
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.
Also, this behavior (what the generated string is for an inline style/rule) is definitely something for which we should write a test.
Created attachment 262278 [details] Patch
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.
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?
> 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.
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
Created attachment 262317 [details] Patch
(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.
Comment on attachment 262317 [details] Patch Test looks good to me.
Comment on attachment 262317 [details] Patch Clearing flags on attachment: 262317 Committed r190528: <http://trac.webkit.org/changeset/190528>
All reviewed patches have been landed. Closing bug.