Bug 199950 - Web Inspector: Elements: Styles: move psuedo-selector rules before inherited rules
Summary: Web Inspector: Elements: Styles: move psuedo-selector rules before inherited ...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-19 10:36 PDT by Devin Rousso
Modified: 2019-08-03 16:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.06 KB, patch)
2019-07-19 12:03 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2019-07-19 12:04 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.24 KB, patch)
2019-08-03 15:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-19 10:36:41 PDT
Since pseudo-selector rules (usually) affect the selected element, it's more useful to have them near that element's rules than after all of it's inherited rules.
Comment 1 Nikita Vasilyev 2019-07-19 11:09:15 PDT
I agree! I would prefer to have pseudo rules be right after the rules that match directly.
Comment 2 Devin Rousso 2019-07-19 12:03:44 PDT
Created attachment 374486 [details]
Patch
Comment 3 Devin Rousso 2019-07-19 12:04:38 PDT
Created attachment 374487 [details]
Patch
Comment 4 Joseph Pecoraro 2019-08-02 20:44:00 PDT
Comment on attachment 374487 [details]
Patch

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

r=me!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:306
> +                beforePseudoId = InspectorBackend.domains.CSS.PseudoId.Before;

You probably already know but this conflicts with your other change, so be careful when landing both.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:331
> +            if (style.inherited && (!previousStyle || !previousStyle.inherited))
> +                addPseudoStyles();

Can this be simplified to just:

    if (style.inherited)
        addPsuedoStyles();

It seems like the `(!previousStyle || !previousStyle.inherited)` part is just to avoid calling the function again but the function already gracefully handles multiple times, so it will already do the work for us.
Comment 5 Devin Rousso 2019-08-03 15:31:55 PDT
Comment on attachment 374487 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:306
>> +                beforePseudoId = InspectorBackend.domains.CSS.PseudoId.Before;
> 
> You probably already know but this conflicts with your other change, so be careful when landing both.

Yep :)

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:331
>> +                addPseudoStyles();
> 
> Can this be simplified to just:
> 
>     if (style.inherited)
>         addPsuedoStyles();
> 
> It seems like the `(!previousStyle || !previousStyle.inherited)` part is just to avoid calling the function again but the function already gracefully handles multiple times, so it will already do the work for us.

Ooooo good point!
Comment 6 Devin Rousso 2019-08-03 15:32:18 PDT
Created attachment 375492 [details]
Patch
Comment 7 WebKit Commit Bot 2019-08-03 16:14:21 PDT
Comment on attachment 375492 [details]
Patch

Clearing flags on attachment: 375492

Committed r248204: <https://trac.webkit.org/changeset/248204>
Comment 8 WebKit Commit Bot 2019-08-03 16:14:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-08-03 16:15:16 PDT
<rdar://problem/53907105>