Bug 214563 – Web Inspector: Change DataGrid and Table styles to closer match macOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

DataGrid.css has some colors referenced (e.g. var(--border-color)) and some hardcoded. This is hard to maintain. When global colors change, the hardcoded values don't, often resulting in poor contrast between the two.

DataGrid needs to be refactored.

[Radar WebKit Bug Importer]

<rdar://problem/65841032>

[Nikita Vasilyev]

Table.css has several colors copy/pasted from DataGrid. It should be refactored, too.

[Nikita Vasilyev]

Created attachment 404807 [details]
Patch

[Nikita Vasilyev]

Created attachment 404808 [details]
[Image] Before, Light

[Nikita Vasilyev]

Created attachment 404809 [details]
[Image] After, Light

[Nikita Vasilyev]

Created attachment 404810 [details]
[Image] Before, Dark

[Nikita Vasilyev]

Created attachment 404811 [details]
[Image] After, Dark

[Blaze Burg]

Comment on attachment 404807 [details]
Patch

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

You haven't commented on which of these changes is appropriate for Big Sur only.

> Source/WebInspectorUI/ChangeLog:22
> +        In macOS Catalina and Big Sur, sorted table header has bold text but the background doesn't change.

Table headers are not bolded on Catalina. Please make this for Big Sur only.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-91
> -body[dir=ltr] .data-grid :matches(th, td):not(:last-child) {

Please explain why we've changed from styling the <th> to styling the inner div. I don't understand why the change is needed.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:95
> +    --horizontal-padding: 6px;

Why is this variable defined with so much specificity? It looks wrong to define it here.

Please rename to --data-grid-header-horizontal-padding or something. It needs to be prefixed with --data-grid.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:111
> +    font-weight: 600;

This should be a datagrid-specific variable.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:233
> +    -webkit-padding-end: calc(12px + var(--horizontal-padding));

Please add a comment that this extra 12px is for the sorting chevron.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:382
> +body[dir=ltr] .data-grid th:first-child > div {

Why is matching 'td' no longer needed?

> Source/WebInspectorUI/UserInterface/Views/Table.css:156
> +    --vertical-margin: 4px;

Please prefix with --data-grid-. Please put its declaration before its uses for ease of reading the code.

[Blaze Burg]

Please include before/after screenshots of the Graphics Tab. It's easier to check your work when the before/after is composited side-by-side into the same PNG.

[Nikita Vasilyev]

Created attachment 404827 [details]
[Image] Finder in Catalina

(In reply to Brian Burg from comment #8)
> Comment on attachment 404807 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404807&action=review
> 
> You haven't commented on which of these changes is appropriate for Big Sur
> only.

All these changes were intended for both Catalina and Big Sur.

> 
> > Source/WebInspectorUI/ChangeLog:22
> > +        In macOS Catalina and Big Sur, sorted table header has bold text but the background doesn't change.
> 
> Table headers are not bolded on Catalina. Please make this for Big Sur only.

It's bold in Finder and Activity Monitor on Catalina. However, I just noticed that in Big Sur the text is more bold (font-weight > 600).

[Nikita Vasilyev]

Comment on attachment 404807 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:111
>> +    font-weight: 600;
> 
> This should be a datagrid-specific variable.

Note that the same value is used in Table.css.

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:382
>> +body[dir=ltr] .data-grid th:first-child > div {
> 
> Why is matching 'td' no longer needed?

I removed vertical borders from table contents and only keep them on the table headers to match macOS.

[Nikita Vasilyev]

Created attachment 404828 [details]
[Image] Space between vertical and horizontal borders

(In reply to Brian Burg from comment #8)
> Comment on attachment 404807 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404807&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-91
> > -body[dir=ltr] .data-grid :matches(th, td):not(:last-child) {
> 
> Please explain why we've changed from styling the <th> to styling the inner
> div. I don't understand why the change is needed.

In macOS, vertical borders don't touch horizontal borders.

[Nikita Vasilyev]

(In reply to Brian Burg from comment #9)
> Please include before/after screenshots of the Graphics Tab. It's easier to
> check your work when the before/after is composited side-by-side into the
> same PNG.

Oops, GraphicsOverviewContentView.css changes shouldn't even be in this patch.

[Nikita Vasilyev]

Created attachment 404834 [details]
Patch

Everything in the before/after screenshots is the same with the exception of font-weight.

[Devin Rousso]

Comment on attachment 404807 [details]
Patch

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

How does this look for rows that have children (including Group Media Requests in the Network Tab)? Can you include screenshots?

> Source/WebInspectorUI/ChangeLog:9
> +        Remove vertical borders from table contents and only keep them on the table headers.

Should we also remove the vertical borders in `WI.TimelineRuler` (or at least add a border before the start)?  Right now, it's very odd to have no borders in the table and then suddenly have borders for the timeline part :/

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:91
> +.data-grid th > div {

Please audit the users/clients of `WI.DataGrid`, as I believe there's a way for users/clients to customize the DOM of the column header, so make sure there aren't any clashes with your reliance on `.data-grid th > div`.

We also should probably add a CSS class to this `<div>` so that it's easy to relate the CSS here with some specific DOM node in the JavaScript (i.e. searchability).

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:95
>> +    --horizontal-padding: 6px;
> 
> Why is this variable defined with so much specificity? It looks wrong to define it here.
> 
> Please rename to --data-grid-header-horizontal-padding or something. It needs to be prefixed with --data-grid.

I personally prefer having variables defined as specifically as possible to avoid clashing with other variables (which I realize can be avoided with a more specific name, but that can be more annoying/verbose to type) and to avoid "polluting" a higher "scope" with more variable names.

I agree that this needs a more specific name tho, as otherwise this variable could clash with something in the child tree.

> Source/WebInspectorUI/UserInterface/Views/Table.css:53
> +    font-weight: 600;

I think we need to change the `font-weight` of the non-sorted rows, as right now it's very hard to distinguish what's selected.  AFAICT, this is what's done in Big Sur.

>> Source/WebInspectorUI/UserInterface/Views/Table.css:156
>> +    --vertical-margin: 4px;
> 
> Please prefix with --data-grid-. Please put its declaration before its uses for ease of reading the code.

this is how we've been doing it so that the variables are separated from the CSS logic

> Source/WebInspectorUI/UserInterface/Views/Table.css:167
> -body[dir=ltr] .table .cell:first-child {
> +body[dir=ltr] .table > .header .cell:first-child {

I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?

[Nikita Vasilyev]

Created attachment 404858 [details]
[Image] Network - Group Media Requests

(In reply to Devin Rousso from comment #15)
> Comment on attachment 404807 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404807&action=review
> 
> How does this look for rows that have children (including Group Media
> Requests in the Network Tab)? Can you include screenshots?

Unaffected, see the screenshot. (Let me know if I misunderstood the question.)

[Nikita Vasilyev]

Comment on attachment 404807 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:9
>> +        Remove vertical borders from table contents and only keep them on the table headers.
> 
> Should we also remove the vertical borders in `WI.TimelineRuler` (or at least add a border before the start)?  Right now, it's very odd to have no borders in the table and then suddenly have borders for the timeline part :/

I don't think it's odd. It's easy to read text without the borders that I removed but the timeline ruler borders are actually helpful.

>> Source/WebInspectorUI/UserInterface/Views/Table.css:53
>> +    font-weight: 600;
> 
> I think we need to change the `font-weight` of the non-sorted rows, as right now it's very hard to distinguish what's selected.  AFAICT, this is what's done in Big Sur.

I don't understand you're suggesting in the 1st sentence. In Big Sur, sorted column header is bold and black, and non-sorted column header is gray and regular font-weight. I intend to fix it in bug 214366: Web Inspector: on Big Sur, match OS background, text, and border colors
but I can move it to fix bug if you insist.

>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
>> +body[dir=ltr] .table > .header .cell:first-child {
> 
> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?

What icon? How do I reproduce this?

[Devin Rousso]

Comment on attachment 404807 [details]
Patch

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

(In reply to Nikita Vasilyev from comment #16)
> Created attachment 404858 [details]
> [Image] Network - Group Media Requests
> 
> (In reply to Devin Rousso from comment #15)
> > Comment on attachment 404807 [details]
> > Patch
> > 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=404807&action=review
> > 
> > How does this look for rows that have children (including Group Media Requests in the Network Tab)? Can you include screenshots?
> 
> Unaffected, see the screenshot. (Let me know if I misunderstood the question.)
What about for `WI.DataGrid`, like in the Timelines Tab or the Storage Tab?

>>> Source/WebInspectorUI/UserInterface/Views/Table.css:53
>>> +    font-weight: 600;
>> 
>> I think we need to change the `font-weight` of the non-sorted rows, as right now it's very hard to distinguish what's selected.  AFAICT, this is what's done in Big Sur.
> 
> I don't understand you're suggesting in the 1st sentence. In Big Sur, sorted column header is bold and black, and non-sorted column header is gray and regular font-weight. I intend to fix it in bug 214366: Web Inspector: on Big Sur, match OS background, text, and border colors
> but I can move it to fix bug if you insist.

This bug is titled "Web Inspector: Change DataGrid and Table styles to closer match macOS".  It should include _all_ known/expected changes related to that topic.

Also, if you plan on fixing something in a followup/related bug, please add a FIXME comment in the code to indicate that.

>>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
>>> +body[dir=ltr] .table > .header .cell:first-child {
>> 
>> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?
> 
> What icon? How do I reproduce this?

By "icon" I mean the resource type icon in the "Name" column.  You can see the differing spacing by comparing your screenshots.

[Image] Before, Light (attachment 404808 [details]) VS [Image] After, Light (attachment 404809 [details])
[Image] Before, Dark (attachment 404810 [details]) VS [Image] After, Dark (attachment 404811 [details])

The lock icon (and subsequent text) in the "Domain" column doesn't shift, but the resource type icon (and subsequent text) in the "Name" column do shift.  Was this intentional?  If so, why do we want to do this?

[Nikita Vasilyev]

Created attachment 404861 [details]
[Image] Icon and text

(In reply to Devin Rousso from comment #18)
> >>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
> >>> +body[dir=ltr] .table > .header .cell:first-child {
> >> 
> >> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?
> > 
> > What icon? How do I reproduce this?
> 
> By "icon" I mean the resource type icon in the "Name" column.  You can see
> the differing spacing by comparing your screenshots.
> 
> [Image] Before, Light (attachment 404808 [details]) VS [Image] After, Light
> (attachment 404809 [details])
> [Image] Before, Dark (attachment 404810 [details]) VS [Image] After, Dark
> (attachment 404811 [details])
> 
> The lock icon (and subsequent text) in the "Domain" column doesn't shift,
> but the resource type icon (and subsequent text) in the "Name" column do
> shift.  Was this intentional?  If so, why do we want to do this?

I see, thanks. The icon shifted one pixel left and now it's aligned with the header text.

[Nikita Vasilyev]

Created attachment 404862 [details]
[Image] Storage tab with patch applied

[Nikita Vasilyev]

Created attachment 404863 [details]
[Image] Timelines with patch applied

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #19)
> Created attachment 404861 [details]
> [Image] Icon and text
> 
> (In reply to Devin Rousso from comment #18)
> > >>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
> > >>> +body[dir=ltr] .table > .header .cell:first-child {
> > >> 
> > >> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?
> > > 
> > > What icon? How do I reproduce this?
> > 
> > By "icon" I mean the resource type icon in the "Name" column.  You can see the differing spacing by comparing your screenshots.
> > 
> > [Image] Before, Light (attachment 404808 [details]) VS [Image] After, Light (attachment 404809 [details])
> > [Image] Before, Dark (attachment 404810 [details]) VS [Image] After, Dark (attachment 404811 [details])
> > 
> > The lock icon (and subsequent text) in the "Domain" column doesn't shift, but the resource type icon (and subsequent text) in the "Name" column do shift.  Was this intentional?  If so, why do we want to do this?
> 
> I see, thanks. The icon shifted one pixel left and now it's aligned with the
> header text.

AFAICT this doesn't match what macOS Big Sur does, where the column header text lines up with the text not the icon or disclosure arrow (who's icon in Web Inspector likely should also change).

[Devin Rousso]

(In reply to Devin Rousso from comment #22)
> AFAICT this doesn't match what macOS Big Sur does, where the column header text lines up with the text not the icon or disclosure arrow (who's icon in Web Inspector likely should also change).

To be clear, I'm not saying whether or not Web Inspector should change to match that.  I'm just stating that there is a difference.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #19)
> Created attachment 404861 [details]
> [Image] Icon and text
> 
> (In reply to Devin Rousso from comment #18)
> > >>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
> > >>> +body[dir=ltr] .table > .header .cell:first-child {
> > >> 
> > >> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?
> > > 
> > > What icon? How do I reproduce this?
> > 
> > By "icon" I mean the resource type icon in the "Name" column.  You can see the differing spacing by comparing your screenshots.
> > 
> > [Image] Before, Light (attachment 404808 [details]) VS [Image] After, Light (attachment 404809 [details])
> > [Image] Before, Dark (attachment 404810 [details]) VS [Image] After, Dark (attachment 404811 [details])
> > 
> > The lock icon (and subsequent text) in the "Domain" column doesn't shift, but the resource type icon (and subsequent text) in the "Name" column do shift.  Was this intentional?  If so, why do we want to do this?
> 
> I see, thanks. The icon shifted one pixel left and now it's aligned with the
> header text.

Also, I think this may cause a mis-alignment when there is no icon, such as in the Storage Tab (attachment 404862 [details]), as the text is slightly too far to the left compared to the text in the column header (but that also may just be the screenshot).

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #24)
> (In reply to Nikita Vasilyev from comment #19)
> > Created attachment 404861 [details]
> > [Image] Icon and text
> > 
> > (In reply to Devin Rousso from comment #18)
> > > >>> Source/WebInspectorUI/UserInterface/Views/Table.css:167
> > > >>> +body[dir=ltr] .table > .header .cell:first-child {
> > > >> 
> > > >> I think this is causing the spacing before the icon of the first column to be different than it was before.  Is that desired?
> > > > 
> > > > What icon? How do I reproduce this?
> > > 
> > > By "icon" I mean the resource type icon in the "Name" column.  You can see the differing spacing by comparing your screenshots.
> > > 
> > > [Image] Before, Light (attachment 404808 [details]) VS [Image] After, Light (attachment 404809 [details])
> > > [Image] Before, Dark (attachment 404810 [details]) VS [Image] After, Dark (attachment 404811 [details])
> > > 
> > > The lock icon (and subsequent text) in the "Domain" column doesn't shift, but the resource type icon (and subsequent text) in the "Name" column do shift.  Was this intentional?  If so, why do we want to do this?
> > 
> > I see, thanks. The icon shifted one pixel left and now it's aligned with the
> > header text.
> 
> Also, I think this may cause a mis-alignment when there is no icon, such as
> in the Storage Tab (attachment 404862 [details]), as the text is slightly
> too far to the left compared to the text in the column header (but that also
> may just be the screenshot).

There's a 0.5px transparent border (--data-grid-column-border-start). I'll remove it.

[Nikita Vasilyev]

Created attachment 404876 [details]
Patch

[Nikita Vasilyev]

Created attachment 404877 [details]
[Image] After, Network resource selected

[Nikita Vasilyev]

Created attachment 404878 [details]
[Image] After, Light

[Nikita Vasilyev]

Created attachment 404879 [details]
After, Dark

[Nikita Vasilyev]

Created attachment 404880 [details]
[Image] After - Storage

[Devin Rousso]

Comment on attachment 404876 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:101
> -body[dir=rtl] .data-grid :matches(th, td):not(:last-child) {
> +body[dir=rtl] .data-grid th:not(:last-child) > .header-cell-content {
>      border-left: var(--data-grid-column-border-end);

I think this may result in clipped UI wherever we have markers on top of the table's contents (e.g. `WI.TimelineMarker`), as there is no longer a `border` to indicate something along the lines of "the content of this column ends here".

ditto for `WI.Table`

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-104
> -    background-color: hsl(0, 0%, 90%);

I don't think this is a good change for columns that don't have text content (e.g. `WI.TimelineRuler` as a `headerView`).  In that case, I think the only way to identify the sorted column would be to look for the up/down arrow (or the absence of bolded text in every other column), and that's probably not the best experience.  Could we leave the `background-color` as-is for columns that have a custom `headerView`?

ditto for `WI.Table`

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:109
> +    font-weight: var(--sorted-header-font-weight);

I think this can be inherited by custom DOM if a `headerView` is supplied (e.g. the numbers in a `WI.TimelineRuler`).  We probably don't want that.

ditto for `WI.Table`

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:170
> +.network-table.showing-detail > .table > .data-container > .data-list > li {

NIT: I feel like this relies on a lot of internal DOM structure of `WI.Table`, which we probably shouldn't be exposing in other files (i.e. layering violation).  It looks like parts of this selector are used elsewhere in this (and one other) file, so there's some precedent.  Would be nice to avoid adding more cases tho so that future changes to `WI.Table` don't have to even think about additional things like this in other files.

> Source/WebInspectorUI/UserInterface/Views/Table.css:-41
> -    line-height: calc(var(--navigation-bar-height) - 1px);

Why was this removed?

> Source/WebInspectorUI/UserInterface/Views/Table.css:49
> +    background-color: var(--background-color-pressed);

Does this `background-color` extend to the full height of the header cell?  I would think that it wouldn't based on the `--table-header-cell-vertical-margin ` below.

> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
> -    border-bottom: solid 1px transparent;

Why was this removed?

> Source/WebInspectorUI/UserInterface/Views/Table.css:152
> +    --data-grid-vertical-margin: 4px;

NIT: did you mean `--table`?
NIT: this should be named more specific to its use, like `--table-header-cell-vertical-margin`

[Nikita Vasilyev]

Comment on attachment 404876 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:101
>>      border-left: var(--data-grid-column-border-end);
> 
> I think this may result in clipped UI wherever we have markers on top of the table's contents (e.g. `WI.TimelineMarker`), as there is no longer a `border` to indicate something along the lines of "the content of this column ends here".
> 
> ditto for `WI.Table`

I don't see a difference. Please let me know how to reproduce this.

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-104
>> -    background-color: hsl(0, 0%, 90%);
> 
> I don't think this is a good change for columns that don't have text content (e.g. `WI.TimelineRuler` as a `headerView`).  In that case, I think the only way to identify the sorted column would be to look for the up/down arrow (or the absence of bolded text in every other column), and that's probably not the best experience.  Could we leave the `background-color` as-is for columns that have a custom `headerView`?
> 
> ditto for `WI.Table`

I think it's odd to have inconsistent styling for columns with WI.TimelineRuler.

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:109
>> +    font-weight: var(--sorted-header-font-weight);
> 
> I think this can be inherited by custom DOM if a `headerView` is supplied (e.g. the numbers in a `WI.TimelineRuler`).  We probably don't want that.
> 
> ditto for `WI.Table`

DataGrid doesn't seem to have cases with `headerView` for columns that are sortable. I can define font-weight in .timeline-ruler just in case this changes.

[Devin Rousso]

Comment on attachment 404876 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:101
>>>      border-left: var(--data-grid-column-border-end);
>> 
>> I think this may result in clipped UI wherever we have markers on top of the table's contents (e.g. `WI.TimelineMarker`), as there is no longer a `border` to indicate something along the lines of "the content of this column ends here".
>> 
>> ditto for `WI.Table`
> 
> I don't see a difference. Please let me know how to reproduce this.

# STEPS TO REPRODUCE
1. open Web Inspector
2. go to the Timelines Tab
3. deselect any timeline
 => the current time marker (red circle) is cut off on the left

The difference isn't in an `overflow` itself, it's in the fact that since there is no longer a border the concept of "this is cut off because it's outside the column area" is not clear.  One possible solution would be to make it so that `WI.TimelineMarker` aren't clipped in this case (e.g. remove an `overflow: hidden;` somewhere).

>>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:-104
>>> -    background-color: hsl(0, 0%, 90%);
>> 
>> I don't think this is a good change for columns that don't have text content (e.g. `WI.TimelineRuler` as a `headerView`).  In that case, I think the only way to identify the sorted column would be to look for the up/down arrow (or the absence of bolded text in every other column), and that's probably not the best experience.  Could we leave the `background-color` as-is for columns that have a custom `headerView`?
>> 
>> ditto for `WI.Table`
> 
> I think it's odd to have inconsistent styling for columns with WI.TimelineRuler.

The fact is that the columns themselves are different.  A `WI.TimelineRuler` as a `headerView` is not basic text like the rest of the columns.  Treating it as if it is the same may result in a bad experience (I personally think so, especially given how much less obvious bolded text is compared to a `background-color`).

>>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:109
>>> +    font-weight: var(--sorted-header-font-weight);
>> 
>> I think this can be inherited by custom DOM if a `headerView` is supplied (e.g. the numbers in a `WI.TimelineRuler`).  We probably don't want that.
>> 
>> ditto for `WI.Table`
> 
> DataGrid doesn't seem to have cases with `headerView` for columns that are sortable. I can define font-weight in .timeline-ruler just in case this changes.

The Media & Animations timeline has a `WI.TimelineRuler` as a `headerView` of its `WI.DataGrid`.  FWIW, the Network Tab also does this with its `WI.Table`.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #33)
> Comment on attachment 404876 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404876&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:101
> >>>      border-left: var(--data-grid-column-border-end);
> >> 
> >> I think this may result in clipped UI wherever we have markers on top of the table's contents (e.g. `WI.TimelineMarker`), as there is no longer a `border` to indicate something along the lines of "the content of this column ends here".
> >> 
> >> ditto for `WI.Table`
> > 
> > I don't see a difference. Please let me know how to reproduce this.
> 
> # STEPS TO REPRODUCE
> 1. open Web Inspector
> 2. go to the Timelines Tab
> 3. deselect any timeline
>  => the current time marker (red circle) is cut off on the left
> 
> The difference isn't in an `overflow` itself, it's in the fact that since
> there is no longer a border the concept of "this is cut off because it's
> outside the column area" is not clear.  One possible solution would be to
> make it so that `WI.TimelineMarker` aren't clipped in this case (e.g. remove
> an `overflow: hidden;` somewhere).

Oh, I see what you mean. This has always annoyed me even with borders. I totally agree on making it not clipped.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #34)
> (In reply to Devin Rousso from comment #33)
> > Comment on attachment 404876 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404876&action=review
> > 
> > >>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:101
> > >>>      border-left: var(--data-grid-column-border-end);
> > >> 
> > >> I think this may result in clipped UI wherever we have markers on top of the table's contents (e.g. `WI.TimelineMarker`), as there is no longer a `border` to indicate something along the lines of "the content of this column ends here".
> > >> 
> > >> ditto for `WI.Table`
> > > 
> > > I don't see a difference. Please let me know how to reproduce this.
> > 
> > # STEPS TO REPRODUCE
> > 1. open Web Inspector
> > 2. go to the Timelines Tab
> > 3. deselect any timeline
> >  => the current time marker (red circle) is cut off on the left
> > 
> > The difference isn't in an `overflow` itself, it's in the fact that since
> > there is no longer a border the concept of "this is cut off because it's
> > outside the column area" is not clear.  One possible solution would be to
> > make it so that `WI.TimelineMarker` aren't clipped in this case (e.g. remove
> > an `overflow: hidden;` somewhere).
> 
> Oh, I see what you mean. This has always annoyed me even with borders. I
> totally agree on making it not clipped.

I made a separate bug:
Bug 214958 - Web Inspector: Hide clipped timeline markers

[Devin Rousso]

Comment on attachment 404876 [details]
Patch

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

>>> Source/WebInspectorUI/ChangeLog:9
>>> +        Remove vertical borders from table contents and only keep them on the table headers.
>> 
> Should we also remove the vertical borders in `WI.TimelineRuler` (or at least add a border before the start)?  Right now, it's very odd to have no borders in the table and then suddenly have borders for the timeline part :/
>
> I don't think it's odd. It's easy to read text without the borders that I removed but the timeline ruler borders are actually helpful.

I think we need to add some sort of border for columns in `WI.DataGrid`/`WI.Table` that have a graph (at least in the Timelines Tab).  Right now, if you make a selection in the Timelines Tab that only covers part of a record, the record appears (buggily) clipped without the border, rather than "I continue off to the left/right".

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:171
> +    border-right: 1px solid var(--border-color);

This doesn't work for pages that don't have enough resources to fill up the entire navigation sidebar (e.g. <https://webkit.org>).

[Devin Rousso]

Comment on attachment 404876 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:93
> +    padding: 0 var(--data-grid-header-horizontal-padding);

We may also need to adjust the initial width of some columns, as some of the ones in the Timelines Tab are clipping in english by default :(

[Nikita Vasilyev]

Created attachment 405717 [details]
[Image] White border

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

>> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
>> -    border-bottom: solid 1px transparent;
> 
> Why was this removed?

It results in the white border under selected item.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #38)
> Created attachment 405717 [details]
> [Image] White border
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404876&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
> >> -    border-bottom: solid 1px transparent;
> > 
> > Why was this removed?
> 
> It results in the white border under selected item.

I think we want to keep that, at the very least for pre Big Sur.  There are also some tables in Big Sure that have a separator between rows (e.g. history view in Safari).  Given that Web Inspector is already not perfectly matching the table/tree look (e.g. the background corners aren't rounded, the background doesn't extend to the edge of the container, etc.) we should consider keeping this for Big Sur too.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #39)
> (In reply to Nikita Vasilyev from comment #38)
> > Created attachment 405717 [details]
> > [Image] White border
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404876&action=review
> > 
> > >> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
> > >> -    border-bottom: solid 1px transparent;
> > > 
> > > Why was this removed?
> > 
> > It results in the white border under selected item.
> 
> I think we want to keep that, at the very least for pre Big Sur.  There are
> also some tables in Big Sure that have a separator between rows (e.g.
> history view in Safari).  Given that Web Inspector is already not perfectly
> matching the table/tree look (e.g. the background corners aren't rounded,
> the background doesn't extend to the edge of the container, etc.) we should
> consider keeping this for Big Sur too.

- It's only visible after the *selected* item.
- Only Table has this behavior.

This looks like a bug.

[Nikita Vasilyev]

Comment on attachment 404876 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/Table.css:-41
>> -    line-height: calc(var(--navigation-bar-height) - 1px);
> 
> Why was this removed?

It doesn't do anything. `.table > .header .cell` defines line-height, again.

>> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
>> -    border-bottom: solid 1px transparent;
> 
> Why was this removed?

It results in the white border under the selected item.

[Nikita Vasilyev]

Sorry for double commenting. Our review tool remembered my old comment :/

[Nikita Vasilyev]

Created attachment 405723 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 405723 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/Table.css:53
> +    /* FIXME: <https://webkit.org/b/214366> Big Sur should have lighter text color for unsorted columns */

I'll remove this comment before landing. I don't think it should be lighter, actually. In Big Sur, non-sorted columns in *Finder* have lighter text color. In many other places they are still the same color, e.g. Activity Monitor.

[Nikita Vasilyev]

Created attachment 405742 [details]
Patch

[Nikita Vasilyev]

Created attachment 405747 [details]
Patch

[Devin Rousso]

Comment on attachment 405747 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        Remove vertical borders from table contents and only keep them on the table headers.

Can you please describe in more detail all of the changes you're making?  I often use the ChangeLog to understand why something was added, and given how much there is in this patch I think it warrants a more thorough explanation/list of the changes.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:102
> +body[dir=ltr] .data-grid th:not(:last-child) > .header-cell-content {
>      border-right: var(--data-grid-column-border-end);
>  }
>  
> -body[dir=rtl] .data-grid :matches(th, td):not(:last-child) {
> +body[dir=rtl] .data-grid th:not(:last-child) > .header-cell-content {
>      border-left: var(--data-grid-column-border-end);
>  }

NIT: you can use `border-inline-end` to avoid two separate `body[dir=ltr]` and `body[dir=rtl]` rules (and drop the `--data-grid-column-border-end` variable too).
```
    .data-grid th:not(:last-child) > .header-cell-content {
        border-inline-end: 1px solid var(--border-color);
    }
```

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:231
> +    -webkit-padding-end: calc(12px + var(--data-grid-header-horizontal-padding)); /* Add 12px for the sorting chevron. */

NIT: we should switch to using `padding-inline-end`

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:407
> +    .data-grid th.sort-ascending > .header-cell-content:first-child::after,
> +    .data-grid th.sort-descending > .header-cell-content:first-child::after {

NIT: `.data-grid th:matches(.sort-ascending, .sort-descending) > .header-cell-content:first-child::after`

> Source/WebInspectorUI/UserInterface/Views/NetworkDetailView.css:84
> +body[dir=ltr] .network-table.showing-detail .network-detail {
> +    border-left: 1px solid var(--border-color);
> +}
> +
> +body[dir=rtl] .network-table.showing-detail .network-detail {
> +    border-right: 1px solid var(--border-color);
> +}

ditto (DataGrid.js:96 execept with `border-inline-start`)

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:185
> +body[dir=ltr] .network-table > .table :not(.header) .cell.waterfall {
> +    border-left: 1px solid var(--timeline-graph-line-color);
> +}
> +
> +body[dir=rtl] .network-table > .table :not(.header) .cell.waterfall {
> +    border-right: 1px solid var(--timeline-graph-line-color);
> +}

ditto (DataGrid.js:87 except with `border-inline-start`)

also, it's odd to use a `--timeline-*` variable in the Network Tab.  Could we rename that so it's not so specific to Timelines Tab (or perhaps use `--border-color`)?

> Source/WebInspectorUI/UserInterface/Views/Table.css:-41
> -    line-height: calc(var(--navigation-bar-height) - 1px);

I think we need to keep this for macOS Catalina

> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
> -    border-bottom: solid 1px transparent;

ditto (41)

> Source/WebInspectorUI/UserInterface/Views/Table.css:151
> +    --table-vertical-padding: 4px;

this should use a more descriptive name like `--table-header-cell-vertical-padding`

also, we normally put CSS variables below other properties

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:48
> +    margin: 0;

Why was this added?

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:53
> +    border-left: 1px solid var(--timeline-graph-line-color);

RTL?

> Source/WebInspectorUI/UserInterface/Views/Variables.css:137
> +    --timeline-graph-line-color: hsla(0, 0%, var(--foreground-lightness), 0.07);

Why not just use `--border-color`?

[Nikita Vasilyev]

Comment on attachment 405747 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/Table.css:-41
>> -    line-height: calc(var(--navigation-bar-height) - 1px);
> 
> I think we need to keep this for macOS Catalina

I'm fine removing it from this particular patch.

>> Source/WebInspectorUI/UserInterface/Views/Table.css:-129
>> -    border-bottom: solid 1px transparent;
> 
> ditto (41)

I'm fine removing it from this particular patch.

>> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:48
>> +    margin: 0;
> 
> Why was this added?

There was no margin before this patch. I added `.data-grid th > .header-cell-content {margin: 3px 0}` and here I'm reseting it back to 0.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:137
>> +    --timeline-graph-line-color: hsla(0, 0%, var(--foreground-lightness), 0.07);
> 
> Why not just use `--border-color`?

I need to use a semi-transparent color here so it works with selected rows. --border-color isn't semi-transparent (and making it such would break many other things).

[Nikita Vasilyev]

Comment on attachment 405747 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:102
>>  }
> 
> NIT: you can use `border-inline-end` to avoid two separate `body[dir=ltr]` and `body[dir=rtl]` rules (and drop the `--data-grid-column-border-end` variable too).
> ```
>     .data-grid th:not(:last-child) > .header-cell-content {
>         border-inline-end: 1px solid var(--border-color);
>     }
> ```

`border-inline-end: 1px solid  var(--data-grid-column-border-end)` doesn't draw any borders for me, yet
`border-inline-end: 1px solid  hsl(0, 0%, 70%)` works fine (that's the value the variable resolves to).
o_O
I'll look more into this on Monday.

[Nikita Vasilyev]

Created attachment 405880 [details]
Patch

[Nikita Vasilyev]

Created attachment 405881 [details]
[Video] RTL mode with patch applied

[Nikita Vasilyev]

I still haven't figured out why `border-inline-end` isn't working with CSS variables in this particular context. It works well in my reduction page. This might be an obscure WebKit bug; I'll continue working on the reduction separately from this issue.

I've addressed everything else.

[Devin Rousso]

Comment on attachment 405880 [details]
Patch

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

r=me, thanks for iterating.  A few last things:
 - (minor, can be followup) the border in the header and body before the graph column in the Timelines Tab don't align horizontally
 - (minor, can be followup) the border in the header and body before the name column in the Network Tab don't align horizontally
 - (minor, can be followup) the border in the header before and after each column should expand to the full height when that column is `:active`
 - (needs discussion, can be followup) columns that have icons/disclosures should have their header text indented so that all the text horizontally aligns

> Source/WebInspectorUI/ChangeLog:12
> +        - Refactoring: add "sorted" CSS class on sorted header columns to replace `th:matches(.sort-ascending, .sort-descending)`
> +          that is used 10 times with `.sorted`.

I don't think we should do this.  It just adds another class that can already be expressed otherwise in CSS, and adds yet another thing that needs to be kept track of when manipulating the DOM of a `WI.DataGrid`/`WI.Table`.

At the very least, this should be in its own patch as it's not critical to resolving this bug.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:58
> +    --background-color-pressed: hsla(0, 0%, var(--foreground-lightness), 0.05);

I think we normally use "active", as that matches the CSS `:active` (and the use cases above).

[Nikita Vasilyev]

Comment on attachment 405880 [details]
Patch

Thanks for reviewing!

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

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:58
>> +    --background-color-pressed: hsla(0, 0%, var(--foreground-lightness), 0.05);
> 
> I think we normally use "active", as that matches the CSS `:active` (and the use cases above).

I think "active" is too ambiguous. There's also a case with --glyph-color-active-pressed, where active means enabled.

[Nikita Vasilyev]

Created attachment 405898 [details]
Patch

[EWS]

Committed r265237: <https://trac.webkit.org/changeset/265237>

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