Bug 217447 - Web Inspector: update styles to use CSS properties with neutral directionality
Summary: Web Inspector: update styles to use CSS properties with neutral directionality
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-07 14:25 PDT by BJ Burg
Modified: 2020-10-29 14:48 PDT (History)
6 users (show)

See Also:


Attachments
Update styles to use CSS properties with neutral directionality (25.78 KB, patch)
2020-10-21 17:15 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Update styles to use CSS properties with neutral directionality (25.91 KB, patch)
2020-10-21 17:26 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Update styles to use CSS properties with neutral directionality (115.44 KB, patch)
2020-10-26 22:15 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Update styles to use CSS properties with neutral directionality (139.48 KB, patch)
2020-10-27 13:26 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Web Inspector: update styles to use CSS properties with neutral directionality (144.23 KB, patch)
2020-10-28 23:46 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Patch (143.64 KB, patch)
2020-10-29 13:10 PDT, Federico Bucchi
no flags Details | Formatted Diff | Diff
Patch (142.66 KB, patch)
2020-10-29 13:19 PDT, Federico Bucchi
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2020-10-07 14:25:05 PDT
.
Comment 1 Radar WebKit Bug Importer 2020-10-07 14:25:16 PDT
<rdar://problem/70064975>
Comment 2 Federico Bucchi 2020-10-21 17:15:41 PDT
Created attachment 412051 [details]
Update styles to use CSS properties with neutral directionality
Comment 3 Federico Bucchi 2020-10-21 17:26:37 PDT
Created attachment 412052 [details]
Update styles to use CSS properties with neutral directionality
Comment 4 Devin Rousso 2020-10-21 17:42:42 PDT
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
Comment 5 Federico Bucchi 2020-10-26 22:15:54 PDT
Created attachment 412390 [details]
Update styles to use CSS properties with neutral directionality
Comment 6 Devin Rousso 2020-10-26 23:52:42 PDT
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`
Comment 7 Federico Bucchi 2020-10-27 13:26:31 PDT
Created attachment 412457 [details]
Update styles to use CSS properties with neutral directionality
Comment 8 Devin Rousso 2020-10-27 18:16:37 PDT
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;
    }
```
Comment 9 Federico Bucchi 2020-10-28 23:46:10 PDT
Created attachment 412618 [details]
Web Inspector: update styles to use CSS properties with neutral directionality
Comment 10 Devin Rousso 2020-10-29 11:55:38 PDT
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.
Comment 11 Federico Bucchi 2020-10-29 13:10:14 PDT
Created attachment 412673 [details]
Patch
Comment 12 Devin Rousso 2020-10-29 13:14:43 PDT
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;
    }
```
Comment 13 Federico Bucchi 2020-10-29 13:19:11 PDT
Created attachment 412676 [details]
Patch
Comment 14 EWS 2020-10-29 14:00:33 PDT
Committed r269166: <https://trac.webkit.org/changeset/269166>

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