Style attributes, as in <div style="color: red">, can't be edited yet.
<rdar://problem/35000990>
Created attachment 325190 [details] Patch
Created attachment 325193 [details] [Animated GIF] Before the patch A page with a style attribute: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/style-attributes.html
Created attachment 325194 [details] [Animated GIF] With patch applied
Created attachment 325217 [details] Patch Added tests for WI.TextRange.prototype.resolveOffsets.
Created attachment 325229 [details] Patch Rebaselined.
Comment on attachment 325229 [details] Patch Whoops, wrong patch.
Comment on attachment 325217 [details] Patch r-, for the following issue: Editing the style attribute via the sidebar causes a space to be prepended to the style attribute text in the DOM tree: 1. Open the test page (http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/style-attributes.html) 2. In the sidebar, change "darkslateblue" to "red" => Style attribute changes to " color: red"
Comment on attachment 325217 [details] Patch (In reply to Matt Baker from comment #8) > Comment on attachment 325217 [details] > Patch > > r-, for the following issue: > > Editing the style attribute via the sidebar causes a space to be prepended > to the style attribute text in the DOM tree: > > 1. Open the test page > (http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/style- > attributes.html) > 2. In the sidebar, change "darkslateblue" to "red" > => Style attribute changes to " color: red" This works the same in the old styles sidebar. I filed Bug 178990 - Web Inspector: Styles: Editing style attribute prepends line-break. I'm setting my patch back to "r?".
Comment on attachment 325217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325217&action=review > LayoutTests/inspector/unit-tests/text-range.html:7 > + let suite = InspectorTest.createAsyncSuite("WI.TextRange"); This should be a createSyncSuite. No need for async in this test. > LayoutTests/inspector/unit-tests/text-range.html:12 > + test(resolve, reject) { This would just be `test() {` in a Sync suite. > LayoutTests/inspector/unit-tests/text-range.html:25 > + resolve(); This would turn into `return true` in a Sync suite. > LayoutTests/inspector/unit-tests/text-range.html:47 > + }); What are the expected values when the text ends in multiple line breaks? let text = "\ncolor: blue;\n\n" We may want to add a test for that, since the CSSProperty.js change looks like it could make that happen. > LayoutTests/inspector/unit-tests/text-range.html:54 > + <p>Testing that WI.TextRange.prototype.resolveOffsets works.</p> Nit: The test description should be for all of TextRange. We only happen to be testing resolveOffsets but we could test anything in TestRange in this generically named test. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:352 > + range.resolveOffsets(styleText + "\n"); Should we only append a newline if it didn't end in a newline, otherwise we could have multiple newlines. Maybe it doesn't matter.
Created attachment 325383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325217&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:352 >> + range.resolveOffsets(styleText + "\n"); > > Should we only append a newline if it didn't end in a newline, otherwise we could have multiple newlines. Maybe it doesn't matter. It doesn't matter here, but I added a test with two trailing line breaks anyway.
Comment on attachment 325383 [details] Patch Clearing flags on attachment: 325383 Committed r224210: <https://trac.webkit.org/changeset/224210>
All reviewed patches have been landed. Closing bug.