Bug 215085

Also, the column header is missing the chevron icon. Every other column has it.

Created attachment 406074 [details]
Patch

Created attachment 406075 [details]
[Video] With patch applied

Comment on attachment 406074 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:53
> +    top: calc(var(--data-grid-header-height) / 2 - var(--data-grid-sorting-chevron-height) / 2);

This feels like an overly complicated way of doing things.  I think the underlying problem is that the `::after` sort indicator is attached to the `.header-cell-content:first-child` instead of the parent `th`.  As you say, `WI.TimelineRuler` is weird cause it uses `position: absolute;` to expand outside of the `th`, which I think is why the `::after` sort indicator was "missing" before (it was positioned in the middle of the `WI.TimelineRuler` not the `th`).  It'd be nice to adjust what node the `::after` sort indicator is attached to so that this can never happen again.

Comment on attachment 406074 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:53
>> +    top: calc(var(--data-grid-header-height) / 2 - var(--data-grid-sorting-chevron-height) / 2);
> 
> This feels like an overly complicated way of doing things.  I think the underlying problem is that the `::after` sort indicator is attached to the `.header-cell-content:first-child` instead of the parent `th`.  As you say, `WI.TimelineRuler` is weird cause it uses `position: absolute;` to expand outside of the `th`, which I think is why the `::after` sort indicator was "missing" before (it was positioned in the middle of the `WI.TimelineRuler` not the `th`).  It'd be nice to adjust what node the `::after` sort indicator is attached to so that this can never happen again.

You're correct that the sort indicator was "missing" before because it was positioned in the middle of the `WI.TimelineRuler` and not the `th`. I don't understand what you're suggesting, exactly. What node should the `::after` sort indicator be attached to?

Note that `position: relative` doesn't work for <th> (or any element with `display: table-cell`).

Comment on attachment 406074 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DataGrid.css:32
> +    --data-grid-sorting-chevron-height: 8px;

NIT: can we use `indicator` (or `indicator-arrow`) instead of `chevron`?  That matches the SVG file name :)

>>> Source/WebInspectorUI/UserInterface/Views/TimelineDataGrid.css:53
>>> +    top: calc(var(--data-grid-header-height) / 2 - var(--data-grid-sorting-chevron-height) / 2);
>> 
>> This feels like an overly complicated way of doing things.  I think the underlying problem is that the `::after` sort indicator is attached to the `.header-cell-content:first-child` instead of the parent `th`.  As you say, `WI.TimelineRuler` is weird cause it uses `position: absolute;` to expand outside of the `th`, which I think is why the `::after` sort indicator was "missing" before (it was positioned in the middle of the `WI.TimelineRuler` not the `th`).  It'd be nice to adjust what node the `::after` sort indicator is attached to so that this can never happen again.
> 
> You're correct that the sort indicator was "missing" before because it was positioned in the middle of the `WI.TimelineRuler` and not the `th`. I don't understand what you're suggesting, exactly. What node should the `::after` sort indicator be attached to?
> 
> Note that `position: relative` doesn't work for <th> (or any element with `display: table-cell`).

Interesting!  I didn't know about the undefined behavior of `position: relative;` and `display: table-cell;`.  That's very unfortunate :(

FWIW, `position: relative;` and `display: table-cell;` appears to work just fine in WebKit.  It would probably be best to create a wrapper between the `th` and the `headerView`, but given that we're trying to phase out `WI.DataGrid` anyways I suppose this is fine.

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:101
> +    position: absolute !important;

Can we create a rule to override this in `TimelineDataGrid.css` instead of using `!important`?  We should really avoid `!important` wherever possible.
```
    .data-grid.timeline th > .header-cell-content.timeline-ruler > .markers {
        position: absolute;
    }
```

Created attachment 406129 [details]
Patch

Committed r265356: <https://trac.webkit.org/changeset/265356>

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

<rdar://problem/66653722>