Bug 210893 – Web Inspector: front-end shouldn't change the order of User Style Sheet rules

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

[Nikita Vasilyev]

<rdar://problem/61937118>

[Radar WebKit Bug Importer]

<rdar://problem/62227866>

[Nikita Vasilyev]

Created attachment 397305 [details]
Patch

[Devin Rousso]

Comment on attachment 397305 [details]
Patch

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

r-, as I think this would no longer conform to the CSS cascade spec <https://www.w3.org/TR/css-cascade-3/#cascading>.

It seems more likely that the issue is in the backend, specifically when we decide to use `Inspector::Protocol::CSS::StyleSheetOrigin::User`.

Also, this needs tests.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:791
>                  userAndUserAgentStyles.push(rule.style);

this should be renamed if it's only `UserAgent` styles

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 397305 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397305&action=review
> 
> r-, as I think this would no longer conform to the CSS cascade spec
> <https://www.w3.org/TR/css-cascade-3/#cascading>.
> 
> It seems more likely that the issue is in the backend, specifically when we
> decide to use `Inspector::Protocol::CSS::StyleSheetOrigin::User`.

Hm, it must be this:

```
Inspector::Protocol::CSS::StyleSheetOrigin InspectorCSSAgent::detectOrigin(CSSStyleSheet* pageStyleSheet, Document* ownerDocument)
{
    if (m_creatingViaInspectorStyleSheet)
        return Inspector::Protocol::CSS::StyleSheetOrigin::Inspector;

    if (pageStyleSheet && !pageStyleSheet->ownerNode() && pageStyleSheet->href().isEmpty())
        return Inspector::Protocol::CSS::StyleSheetOrigin::UserAgent;

    if (pageStyleSheet && pageStyleSheet->ownerNode() && pageStyleSheet->ownerNode()->nodeName() == "#document")
        return Inspector::Protocol::CSS::StyleSheetOrigin::User;

    auto iterator = m_documentToInspectorStyleSheet.find(ownerDocument);
    if (iterator != m_documentToInspectorStyleSheet.end()) {
        for (auto& inspectorStyleSheet : iterator->value) {
            if (pageStyleSheet == inspectorStyleSheet->pageStyleSheet())
                return Inspector::Protocol::CSS::StyleSheetOrigin::Inspector;
        }
    }

    return Inspector::Protocol::CSS::StyleSheetOrigin::Regular;
}
```

> 
> Also, this needs tests.

Any tips on how to test this?

[Nikita Vasilyev]

It looks like this

```
    if (pageStyleSheet && pageStyleSheet->ownerNode() && pageStyleSheet->ownerNode()->nodeName() == "#document")
        return Inspector::Protocol::CSS::StyleSheetOrigin::User;
```

is rather arbitrary and should be replaced with something that looks into the stylesheet's level.

WebCore/page/UserStyleSheet.h defines `level` method. It looks like this is what should be used to determine whether inspector receives `Inspector::Protocol::CSS::StyleSheetOrigin::User` or something else.

I don't understand the relationship between page/UserStyleSheet and css/CSSStyleSheet. I don't know if I can access the `level` method here.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #4)
> > Also, this needs tests.
> 
> Any tips on how to test this?

Specifically, I need to mock User Style Sheet in an inspector test. We haven't done anything like this.

[Nikita Vasilyev]

From what I observed, Web Inspector front-end receives style rules in the correct order. I see the order is determined here in WebCore/style/InspectorCSSOMWrappers.cpp:

void InspectorCSSOMWrappers::collectDocumentWrappers(ExtensionStyleSheets& extensionStyleSheets)
{
        ...
        collect(extensionStyleSheets.pageUserSheet());
        collectFromStyleSheets(extensionStyleSheets.injectedUserStyleSheets());
        collectFromStyleSheets(extensionStyleSheets.documentUserStyleSheets());
        collectFromStyleSheets(extensionStyleSheets.injectedAuthorStyleSheets());
        collectFromStyleSheets(extensionStyleSheets.authorStyleSheetsForTesting());
        ...
}

Given that the order is (correctly) determined on the backend, it seems strange to have a front-end method (WI.DOMNodeStyles.prototype._collectStylesInCascadeOrder) that changes the order.

I'm working on tests now.

[Nikita Vasilyev]

Created attachment 399217 [details]
Patch

(In reply to Nikita Vasilyev from comment #5)
> It looks like this
> 
> ```
>     if (pageStyleSheet && pageStyleSheet->ownerNode() &&
> pageStyleSheet->ownerNode()->nodeName() == "#document")
>         return Inspector::Protocol::CSS::StyleSheetOrigin::User;
> ```
> 
> is rather arbitrary and should be replaced with something that looks into
> the stylesheet's level.
> 
> WebCore/page/UserStyleSheet.h defines `level` method. It looks like this is
> what should be used to determine whether inspector receives
> `Inspector::Protocol::CSS::StyleSheetOrigin::User` or something else.
> 
> I don't understand the relationship between page/UserStyleSheet and
> css/CSSStyleSheet. I don't know if I can access the `level` method here.

I couldn't figure out how to do this but checking for "user-style-sheet://" protocol worked pretty well!

cq- for now as I'm still trying to figure out how to write a Web Inspector test.

[Nikita Vasilyev]

Created attachment 399218 [details]
Patch

[Devin Rousso]

Comment on attachment 399218 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:934
> -    if (pageStyleSheet && pageStyleSheet->ownerNode() && pageStyleSheet->ownerNode()->nodeName() == "#document")
> +    if (pageStyleSheet && pageStyleSheet->baseURL() && pageStyleSheet->baseURL().protocolIs("user-style-sheet"))

This seems fragile.  I wonder if it's possible for an app to have a custom URL scheme handler for `user-style-sheet://`?

Why not just check `pageStyleSheet->contents().isUserStyleSheet()`?

[Nikita Vasilyev]

Created attachment 399244 [details]
Patch

isUserStyleSheet() works well, too!

P.S. I your patch in Bug 211827 - Web Inspector: rename CSS.StyleSheetOrigin.Regular to CSS.StyleSheetOrigin.Author to match the spec.
I'll rebase mine if yours lands first.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #11)
>
> P.S. I your patch...

*I saw your patch...

[Devin Rousso]

Comment on attachment 399244 [details]
Patch

r=me

[Timothy Hatcher]

<rdar://problem/61937118>

[EWS]

Committed r261703: <https://trac.webkit.org/changeset/261703>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399244 [details].

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/261703/webkit

broke inspector/css/getMatchedStylesForNode.html

Tracking in https://bugs.webkit.org/show_bug.cgi?id=211918