Bug 210893 - Web Inspector: front-end shouldn't change the order of User Style Sheet rules
Summary: Web Inspector: front-end shouldn't change the order of User Style Sheet rules
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: 2020-04-22 19:41 PDT by Nikita Vasilyev
Modified: 2020-05-14 13:41 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.68 KB, patch)
2020-04-22 19:50 PDT, Nikita Vasilyev
hi: review-
Details | Formatted Diff | Diff
Patch (2.06 KB, patch)
2020-05-12 18:28 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (1.91 KB, patch)
2020-05-12 18:38 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2020-05-12 23: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 2020-04-22 19:41:38 PDT
<rdar://problem/61937118>
Comment 1 Radar WebKit Bug Importer 2020-04-22 19:41:52 PDT
<rdar://problem/62227866>
Comment 2 Nikita Vasilyev 2020-04-22 19:50:51 PDT
Created attachment 397305 [details]
Patch
Comment 3 Devin Rousso 2020-04-23 09:44:35 PDT
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
Comment 4 Nikita Vasilyev 2020-04-23 14:15:03 PDT
(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?
Comment 5 Nikita Vasilyev 2020-04-23 15:01:38 PDT
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.
Comment 6 Nikita Vasilyev 2020-04-29 00:28:28 PDT
(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.
Comment 7 Nikita Vasilyev 2020-05-12 14:34:00 PDT
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.
Comment 8 Nikita Vasilyev 2020-05-12 18:28:39 PDT
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.
Comment 9 Nikita Vasilyev 2020-05-12 18:38:21 PDT
Created attachment 399218 [details]
Patch
Comment 10 Devin Rousso 2020-05-12 18:43:27 PDT
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()`?
Comment 11 Nikita Vasilyev 2020-05-12 23:39:20 PDT
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.
Comment 12 Nikita Vasilyev 2020-05-12 23:40:10 PDT
(In reply to Nikita Vasilyev from comment #11)
>
> P.S. I your patch...

*I saw your patch...
Comment 13 Devin Rousso 2020-05-14 08:20:30 PDT
Comment on attachment 399244 [details]
Patch

r=me
Comment 14 Timothy Hatcher 2020-05-14 10:47:01 PDT
<rdar://problem/61937118>
Comment 15 EWS 2020-05-14 12:28:45 PDT
Committed r261703: <https://trac.webkit.org/changeset/261703>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399244 [details].
Comment 16 Truitt Savell 2020-05-14 13:41:26 PDT
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