Bug 178328 - Web Inspector: [PARITY] Styles Redesign: Ability to modify style attributes
Summary: Web Inspector: [PARITY] Styles Redesign: Ability to modify style attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-15 20:51 PDT by Nikita Vasilyev
Modified: 2017-10-30 16:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.10 KB, patch)
2017-10-27 13:29 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] Before the patch (130.08 KB, image/gif)
2017-10-27 13:37 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] With patch applied (175.41 KB, image/gif)
2017-10-27 13:39 PDT, Nikita Vasilyev
no flags Details
Patch (5.52 KB, patch)
2017-10-27 16:40 PDT, Nikita Vasilyev
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Patch (18.19 KB, patch)
2017-10-27 18:44 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (6.28 KB, patch)
2017-10-30 15:39 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-10-15 20:51:07 PDT
Style attributes, as in <div style="color: red">, can't be edited yet.
Comment 1 Radar WebKit Bug Importer 2017-10-15 20:51:28 PDT
<rdar://problem/35000990>
Comment 2 Nikita Vasilyev 2017-10-27 13:29:56 PDT
Created attachment 325190 [details]
Patch
Comment 3 Nikita Vasilyev 2017-10-27 13:37:37 PDT
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
Comment 4 Nikita Vasilyev 2017-10-27 13:39:23 PDT
Created attachment 325194 [details]
[Animated GIF] With patch applied
Comment 5 Nikita Vasilyev 2017-10-27 16:40:02 PDT
Created attachment 325217 [details]
Patch

Added tests for WI.TextRange.prototype.resolveOffsets.
Comment 6 Nikita Vasilyev 2017-10-27 18:44:43 PDT
Created attachment 325229 [details]
Patch

Rebaselined.
Comment 7 Nikita Vasilyev 2017-10-27 18:45:55 PDT
Comment on attachment 325229 [details]
Patch

Whoops, wrong patch.
Comment 8 Matt Baker 2017-10-29 06:27:35 PDT
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 9 Nikita Vasilyev 2017-10-29 13:47:22 PDT
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 10 Joseph Pecoraro 2017-10-30 11:49:54 PDT
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.
Comment 11 Nikita Vasilyev 2017-10-30 15:39:52 PDT
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 12 WebKit Commit Bot 2017-10-30 16:11:10 PDT
Comment on attachment 325383 [details]
Patch

Clearing flags on attachment: 325383

Committed r224210: <https://trac.webkit.org/changeset/224210>
Comment 13 WebKit Commit Bot 2017-10-30 16:11:11 PDT
All reviewed patches have been landed.  Closing bug.