Bug 217447 – Web Inspector: update styles to use CSS properties with neutral directionality

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/70064975>

[Federico Bucchi]

Created attachment 412051 [details]
Update styles to use CSS properties with neutral directionality

[Federico Bucchi]

Created attachment 412052 [details]
Update styles to use CSS properties with neutral directionality

[Devin Rousso]

Comment on attachment 412052 [details]
Update styles to use CSS properties with neutral directionality

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

r- for the reasons below

I think you're missing a lot of other `body[dir=*]` rules :(

I'd also suggest updating `-webkit-margin-*`/`-webkit-padding-*` to use `margin-inline-*`/`padding-inline-*` instead

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:80
> +    float: inline-start;

Safari doesn't support `inline-*` for `float`

ditto in other files

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:119
>      border-right: var(--cpu-timeline-view-overview-divider-border-end);

Why are we keeping this?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:120
> +    border-block-end: var(--cpu-timeline-view-overview-divider-border-end);

This should be `-inline-` since it's left/right (we only support a horizontal writing mode, so `-inline-` will always be left/right and `-block-` will always be top/bottom).

Also, please remove the variable and inline its value now that it's only used in one place.

ditto in other files

> Source/WebInspectorUI/UserInterface/Views/FindBanner.css:100
> +    border-start-start-radius: var(--find-banner-search-box-border-radius-start);
> +    border-end-start-radius: var(--find-banner-search-box-border-radius-start);
> +    border-start-end-radius: var(--find-banner-search-box-border-radius-end);
> +    border-end-end-radius: var(--find-banner-search-box-border-radius-end);

??? these are not CSS properties

ditto in other files

[Federico Bucchi]

Created attachment 412390 [details]
Update styles to use CSS properties with neutral directionality

[Devin Rousso]

Comment on attachment 412390 [details]
Update styles to use CSS properties with neutral directionality

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

Almost there.  A few more changes I think.
 - use shorthands where possible (e.g. `padding-inline` instead of both `padding-inline-start` and `padding-inline-end`)
 - replace `left`/`right` with `inset-inline-start`/`inset-inline-end`

As you've already been doing, please make sure to only modify styles inside `body[dir=___]` rules, as there are some parts of Web Inspector that are intentionally always LTR that probably shouldn't be touched in a large refactoring/renaming patch like this :P

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:40
> +    margin-inline-start: 65px;
> +    margin-inline-end: 55px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:52
>      border-left: 4px solid transparent;
>      border-right: 4px solid transparent;

`border-inline`

> Source/WebInspectorUI/UserInterface/Views/BreakpointPopover.css:82
> +    margin-inline-start: 0;
> +    margin-inline-end: 4px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/CallFrameTreeElement.css:99
> +    margin-inline-start: -5px;
> +    margin-inline-end: 1px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/FindBanner.css:85
> +    margin-inline-start: 6px;
> +    margin-inline-end: -1px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathComponent.css:141
> +    padding-inline-start: 5px;
> +    padding-inline-end: 1px;

`padding-inline`

> Source/WebInspectorUI/UserInterface/Views/Main.css:561
> +    margin-inline-start: 0.5px;
> +    margin-inline-end: 4px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:28
> +    margin-inline-start: 8px;
> +    margin-inline-end: 4px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.css:54
> +    margin-inline-start: 32px;
> +    margin-inline-end: 3px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.css:81
> +    padding-inline-start: 7px;
> +    padding-inline-end: 30px;

`padding-inline`

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.css:89
> +    padding-inline-start: 5px;
> +    padding-inline-end: 1px;

`padding-inline`

> Source/WebInspectorUI/UserInterface/Views/RecordingActionTreeElement.css:34
> +    margin-inline-end: var(--tree-outline-icon-margin-end);
> +    margin-inline-start: 0;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:128
> +    margin-inline-start: 6px;
> +    margin-inline-end: 2px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.css:131
> +    margin-inline-start: 2px;
> +    margin-inline-end: 5px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:209
> +body:not(.docked) .tab-bar > .tabs:not(.hide-border-end) > .item:nth-last-child(1 of :not(.hidden)) {

this last selector should be a separate rule
```
    body:not(.docked) .tab-bar > .tabs:not(.hide-border-end) > .item:nth-last-child(1 of :not(.hidden)) {
        border-inline-end: var(--tab-item-medium-border-style);
    }
```

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:257
> +body:not(.docked) .tab-bar > .tabs:not(.animating) > .item:last-child:not(.selected, .disabled):hover {

this last selector should be a separate rule
```
    :not(.docked) .tab-bar > .tabs:not(.animating) > .item:last-child:not(.selected, .disabled):hover {
        border-inline-end-color: var(--tab-item-dark-border-color);
    }
```

> Source/WebInspectorUI/UserInterface/Views/ThreadTreeElement.css:56
> +    margin-inline-start: 31px;
> +    margin-inline-end: 6px;

`margin-inline`

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.css:35
>      left: 0;

I think we can change this and the `right: 0;` to `inset-inline-start`

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:60
> +    border-inline-end: var(--record-bar-segment-border-end);
> +    border-inline-start: var(--record-bar-segment-border-start);

`border-inline`

[Federico Bucchi]

Created attachment 412457 [details]
Update styles to use CSS properties with neutral directionality

[Devin Rousso]

Comment on attachment 412457 [details]
Update styles to use CSS properties with neutral directionality

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

Almost there!  Only a few issues :)

Also, I think you missed one change in `Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css` :P

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.css:82
> +body .content-view.tab.audit .content-view .reference-page-link-container {

`body` shouldn't be necessary anymore, so please merge this with the above rule.  We used `body` as a way of representing "this is the direction of the entire page" rather than on some other node/selector (which would've been "this is the direction of this one component").

> Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:53
> +    border-inline: 4px;
> +    border-inline-style: solid;
> +    border-inline-color: transparent;

`border-inline: 4px solid transparent`

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:49
> +    inset-inline-end: var(--cpu-timeline-overview-graph-legend-offset-end);

We can get rid of this variable.  We used to have a rule that for LTR/RTL values, use a variable defined on a rule with the same selector after the `body[dir=*]` so that the value didn't have to be repeated twice.  Anywhere that you see this pattern in the changes you've made so far (you can easily spot this just by searching for the variable and seeing if it's used elsewhere), please just remove the variable.

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathComponent.css:110
> +    inset-inline-start: var(--path-component-select-offset-start);
> +    inset-inline-end: var(--path-component-select-offset-end);

`inset-inline`

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:-48
> -body[dir=ltr] .content-view.shader-program > .shader.fragment,
> -body[dir=rtl] .content-view.shader-program > .shader.vertex:not(.shares-vertex-fragment-shader) {
> -    width: calc(50% + 1px);

I don't think we want to remove this.
```
    .content-view.shader-program > .shader.compute,
    .content-view.shader-program > .shader.vertex {
        inset-inline-start: 0;
    }

    .content-view.shader-program > .shader.compute,
    .content-view.shader-program > .shader.vertex.shares-vertex-fragment-shader,
    .content-view.shader-program > .shader.fragment {
        inset-inline-end: 0;
    }

    body[dir=ltr] .content-view.shader-program > .shader.vertex:not(.shares-vertex-fragment-shader),
    body[dir=rtl] .content-view.shader-program > .shader.fragment {
        width: calc(50% - 1px);
    }

    body[dir=ltr] .content-view.shader-program > .shader.fragment,
    body[dir=rtl] .content-view.shader-program > .shader.vertex:not(.shares-vertex-fragment-shader) {
        width: calc(50% + 1px);
    }
```
IIRC, the `1px` in the `calc` is used to provide space for the divider line between the vertex shader editor and fragment shader editor.

> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.css:40
> +body .single-sidebar.trailing > .resizer,
> +body .single-sidebar.leading > .resizer {
> +    inset-inline: -3px;

This isn't quite right.  We don't want to set both `left` and `right` for both.
```
    .single-sidebar.trailing > .resizer {
        inset-inline-start: -3px;
    }

    .single-sidebar.leading > .resizer {
        inset-inline-end: -3px;
    }
```

> Source/WebInspectorUI/UserInterface/Views/SpringEditor.css:81
> +body .spring-editor > .spring-timing::before,
> +body .spring-editor > .spring-timing::after {
> +    inset-inline: var(--spring-editor-timing-pseudo-offset-start) var(--spring-editor-timing-pseudo-offset-end);

This isn't quite right.  We don't want to set both `left` and `right` for both.
```
    .spring-editor > .spring-timing::before {
        inset-inline-start: 0;
    }

    .spring-editor > .spring-timing::after {
        inset-inline-end: 0;
    }
```

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.css:34
> +body .timeline-overview > :matches(.navigation-bar.timelines, .tree-outline.timelines) {

please merge this with the previous rule

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:59
> +    border-inline: var(--record-bar-segment-border-start) var(--record-bar-segment-border-end);

this should be moved to `.timeline-record-bar > .segment`

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:268
> +body .timeline-ruler > .shaded-area.left,
> +body .timeline-ruler > .shaded-area.right {
> +    inset-inline: 0;

This isn't quite right.  We don't want to set both `left` and `right` for both.
```
    .timeline-ruler > .shaded-area.left {
        inset-inline-start: 0;
    }

    .timeline-ruler > .shaded-area.right {
        inset-inline-end: 0;
    }
```

[Federico Bucchi]

Created attachment 412618 [details]
Web Inspector: update styles to use CSS properties with neutral directionality

[Devin Rousso]

Comment on attachment 412618 [details]
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.

[Federico Bucchi]

Created attachment 412673 [details]
Patch

[Devin Rousso]

Comment on attachment 412673 [details]
Patch

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

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

oops, no leading space 😅

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

err, no, this should just go back to the way it was before
```
    .find-banner.console-find-banner {
        position: relative;
        top: auto;
        border: 0;
    }

    body .find-banner.console-find-banner {
        flex-wrap: nowrap;
    }
```

[Federico Bucchi]

Created attachment 412676 [details]
Patch

[EWS]

Committed r269166: <https://trac.webkit.org/changeset/269166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412676 [details].