Bug 199950

Summary: Web Inspector: Elements: Styles: move psuedo-selector rules before inherited rules
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

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>