Bug 199950

Summary:

Web Inspector: Elements: Styles: move psuedo-selector rules before inherited rules

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, joepeck, nvasilyev, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Devin Rousso

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Nikita Vasilyev

Comment 1 2019-07-19 11:09:15 PDT

I agree! I would prefer to have pseudo rules be right after the rules that match directly.

Devin Rousso

Comment 2 2019-07-19 12:03:44 PDT

Created attachment 374486 [details] Patch

Devin Rousso

Comment 3 2019-07-19 12:04:38 PDT

Created attachment 374487 [details] Patch

Joseph Pecoraro

Comment 4 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.

Devin Rousso

Comment 5 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!

Devin Rousso

Comment 6 2019-08-03 15:32:18 PDT

Created attachment 375492 [details] Patch

WebKit Commit Bot

Comment 7 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>

WebKit Commit Bot

Comment 8 2019-08-03 16:14:23 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 9 2019-08-03 16:15:16 PDT

<rdar://problem/53907105>

Note You need to log in before you can comment on or make changes to this bug.