Bug 178328 – Web Inspector: [PARITY] Styles Redesign: Ability to modify style attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Style attributes, as in <div style="color: red">, can't be edited yet.

[Radar WebKit Bug Importer]

<rdar://problem/35000990>

[Nikita Vasilyev]

Created attachment 325190 [details]
Patch

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 325194 [details]
[Animated GIF] With patch applied

[Nikita Vasilyev]

Created attachment 325217 [details]
Patch

Added tests for WI.TextRange.prototype.resolveOffsets.

[Nikita Vasilyev]

Created attachment 325229 [details]
Patch

Rebaselined.

[Nikita Vasilyev]

Comment on attachment 325229 [details]
Patch

Whoops, wrong patch.

[Matt Baker]

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"

[Nikita Vasilyev]

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?".

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 325383 [details]
Patch

Clearing flags on attachment: 325383

Committed r224210: <https://trac.webkit.org/changeset/224210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.