[webkit-reviews] review granted: [Bug 217447] Web Inspector: update styles to use CSS properties with neutral directionality : [Attachment 412618] Web Inspector: update styles to use CSS properties with neutral directionality

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Federico Bucchi
<fbucchi at apple.com>'s request for review:
Bug 217447: Web Inspector: update styles to use CSS properties with neutral
directionality
https://bugs.webkit.org/show_bug.cgi?id=217447

Attachment 412618: Web Inspector: update styles to use CSS properties with
neutral directionality

https://bugs.webkit.org/attachment.cgi?id=412618&action=review




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 412618
  --> https://bugs.webkit.org/attachment.cgi?id=412618
Web Inspector: update styles to use CSS properties with neutral directionality

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

r=me, with a few remaining changes.  Nice work!  =D

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:27
> -body .sidebar > .panel.navigation.timeline > .timelines-content li.item.cpu,
> -body .timeline-overview > .graphs-container > .timeline-overview-graph.cpu {
> +.sidebar > .panel.navigation.timeline > .timelines-content li.item.cpu,
> +.timeline-overview > .graphs-container > .timeline-overview-graph.cpu {

this should probably stay as it was before, partly because it's not related to
this bug as described (i.e. there's no neutral direction property here) and
party because it may cause a regressions (it was added in r240457, not really
sure why)

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:62
> +    inset-inline-start: 150px;

because we set `left: 0;` and `right: 0;` above, this could change to be
`inset-inline: 150px 0;`, but it's not that big of a deal

> Source/WebInspectorUI/UserInterface/Views/FindBanner.css:176
> +    border-inline-start: var(--next-result-border-style-start);

this could just be `none` and remove `--next-result-border-style-start: none;`

also, we put CSS variables at the end of rules with a newline separator to
distinguish between "visual styles" and "variable styles"
```
    .find-banner > button.segmented.next-result {
	border-inline-start: none;

	--next-result-border-radius-start: 0;
    }
```

> Source/WebInspectorUI/UserInterface/Views/FindBanner.css:-241
> -body .find-banner.console-find-banner {

this should probably stay as it was before, partly because it's not related to
this bug as described (i.e. there's no neutral direction property here) and
party because it may cause a regressions (it was added in r196634, not really
sure why)

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.css:27
> -body .sidebar > .panel.navigation.timeline > .timelines-content
li.item.memory,
> -body .timeline-overview > .graphs-container >
.timeline-overview-graph.memory {
> +.sidebar > .panel.navigation.timeline > .timelines-content li.item.memory,
> +.timeline-overview > .graphs-container > .timeline-overview-graph.memory {

this should probably stay as it was before, partly because it's not related to
this bug as described (i.e. there's no neutral direction property here) and
party because it may cause a regressions (it was added in r195999, not really
sure why)

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.css:55
> -body[dir=rtl] .sidebar > .panel.navigation.search .item.source-code-match {
> +.sidebar > .panel.navigation.search .item.source-code-match {
>      direction: ltr;
> -    text-align: right;
> +    text-align: start;
>  }

this should probably stay as it was before, as it was explicitly added in
r214864

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.css:-81
> -    left: var(--spring-editor-timing-pseudo-offset-start);

Can we delete the `--spring-editor-timing-pseudo-offset-start: 0;` above?

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.css:-86
> -    right: var(--spring-editor-timing-pseudo-offset-end);

Can we delete the `--spring-editor-timing-pseudo-offset-end: 0;` above?

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:126
> +    inset-inline-start: var(--timeline-ruler-marker-before-offset);

Please inline the value here and remove `--timeline-ruler-marker-before-offset`
below.