Bug 194619 – Web Inspector: Styles: valid values in style attributes are reported as unsupported property values

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

[Nikita Vasilyev]

<rdar://problem/47917373>

[Nikita Vasilyev]

Created attachment 361965 [details]
Patch

[Nikita Vasilyev]

Created attachment 361971 [details]
[Video] Bug

The problem was mismatching text ranges, which caused all kind of data corruption bugs.

[Nikita Vasilyev]

Created attachment 361972 [details]
Patch

I removed unrelated code changes.

[Devin Rousso]

Comment on attachment 361972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361972&action=review

rs=me, please wait for EWS

> LayoutTests/inspector/css/modify-inline-style.html:7
> +    let nodeStyles = null;

Style: missing newline after.

> LayoutTests/inspector/css/modify-inline-style.html:11
> +        name: "AddPropertyAndEdit",

Please prefix the test case name with the test suite name.

    name: "ModifyInlineStyle.AddPropertyAndEdit",

> LayoutTests/inspector/css/modify-inline-style.html:27
> +            fontProperty.rawValue = "12px normal sans-serif!important";

Style: missing space before `!important`.

> LayoutTests/inspector/css/modify-inline-style.html:28
> +            let colorProperty = null;

Style: missing newline before.

> LayoutTests/inspector/css/modify-inline-style.html:30
> +            WI.CSSProperty.addEventListener(WI.CSSProperty.Event.Changed, function(event) {

NIT: just in case, you should remove this listener when the test case completes.
NIT: arrow function?

> LayoutTests/inspector/css/modify-inline-style.html:38
> +            fontProperty.awaitEvent(WI.CSSProperty.Event.Changed).then((event) => {

Please put the `.then(` on a new line.

> LayoutTests/inspector/css/modify-inline-style.html:56
> +            }).then(() => {
> +                resolve();
> +                return true;
> +            });

Rather than put `resolve()` inside a `.then(`, you can use it directly as the callback.

    .then(resolve, reject);

> Source/WebInspectorUI/ChangeLog:10
> +        the actual text of the payload â it has an extra empty line at the end.

non-ascii character :(

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:539
> +        if (payload.range) {

In order to avoid retyping `payload.range` everywhere, you should pull it out into a variable (like all the other ones at the top of this function) and include a fallback value (an empty object).

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:540
> +            // Last property of inline style has mismatching range.

If this is only supposed to happen for inline styles, we should assert inside the last `if` that `styleDeclaration.type === WI.CSSStyleDeclaration.Type.Inline`.  If not, please remove this comment (or make it more general).

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:544
> +                let textLineCount = payload.text.lineCount;

You can just use `text`, since it has a fallback as well.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:547
> +                    payload.range.endColumn = payload.range.startColumn + payload.text.lastLine.length;

Ditto (>544).

[Nikita Vasilyev]

Comment on attachment 361972 [details]
Patch

Thanks for the review!

View in context: https://bugs.webkit.org/attachment.cgi?id=361972&action=review

>> LayoutTests/inspector/css/modify-inline-style.html:27
>> +            fontProperty.rawValue = "12px normal sans-serif!important";
> 
> Style: missing space before `!important`.

I think we should use all kind of code styles for test data. The no-space !important is used on medium.com.

>> Source/WebInspectorUI/ChangeLog:10
>> +        the actual text of the payload â it has an extra empty line at the end.
> 
> non-ascii character :(

Our review tools are bad :(

[Nikita Vasilyev]

Created attachment 361984 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 361984 [details]
Patch

Unrelated test failures on Mac Debug. cq+

[WebKit Commit Bot]

Comment on attachment 361984 [details]
Patch

Clearing flags on attachment: 361984

Committed r241497: <https://trac.webkit.org/changeset/241497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The new test inspector/css/modify-inline-style.html

added in https://trac.webkit.org/changeset/241497/webkit

is a constant timeout on Mac Release WK2.

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fmodify-inline-style.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 194676

[Nikita Vasilyev]

Created attachment 362101 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 362101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362101&action=review

> LayoutTests/inspector/css/modify-inline-style.html:85
> +                nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) {
> +                    // Normally, this would get called from views if the styles sidebar is visible.
> +                    // This doesn't work in tests.
> +                    event.target.refresh();
> +                });

Joe and I investigated the problem. This fixed test timeouts on WK2. The rest of the patch is the same as the one that was rolled out.

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #13)
> Comment on attachment 362101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362101&action=review
> 
> > LayoutTests/inspector/css/modify-inline-style.html:85
> > +                nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) {
> > +                    // Normally, this would get called from views if the styles sidebar is visible.
> > +                    // This doesn't work in tests.
> > +                    event.target.refresh();
> > +                });
> 
> Joe and I investigated the problem. This fixed test timeouts on WK2. The
> rest of the patch is the same as the one that was rolled out.

Yeah, I really think this is something all of the DOMNodeStyles tests might want to consider.

Maybe we should add a TestExtras.js that does something like this, because we are almost certainly going to forgot to do this in other tests!

[Joseph Pecoraro]

Comment on attachment 362101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362101&action=review

rs=me

>>> LayoutTests/inspector/css/modify-inline-style.html:85
>>> +                nodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, function(event) {
>>> +                    // Normally, this would get called from views if the styles sidebar is visible.
>>> +                    // This doesn't work in tests.
>>> +                    event.target.refresh();
>>> +                });
>> 
>> Joe and I investigated the problem. This fixed test timeouts on WK2. The rest of the patch is the same as the one that was rolled out.
> 
> Yeah, I really think this is something all of the DOMNodeStyles tests might want to consider.
> 
> Maybe we should add a TestExtras.js that does something like this, because we are almost certainly going to forgot to do this in other tests!

I think you should probably do something like this at the top level:

    WI.DOMNodeStyles.addEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, (event) => {
        event.target.refresh();
    });

So that any DOMNodeStyles that needs a refresh will refresh. Let me know if that works.

[Nikita Vasilyev]

Created attachment 362157 [details]
Patch

[Nikita Vasilyev]

Yes, it worked.

[WebKit Commit Bot]

Comment on attachment 362157 [details]
Patch

Clearing flags on attachment: 362157

Committed r241623: <https://trac.webkit.org/changeset/241623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

*** Bug 194662 has been marked as a duplicate of this bug. ***