[webkit-reviews] review granted: [Bug 215085] Web Inspector: Media & Animations timeline shouldn't shift when sorting : [Attachment 406074] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 215085: Web Inspector: Media & Animations timeline shouldn't shift when
sorting
https://bugs.webkit.org/show_bug.cgi?id=215085

Attachment 406074: Patch

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




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 406074
  --> https://bugs.webkit.org/attachment.cgi?id=406074
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;
    }
```