Created attachment 405841 [details] [Video] Bug When clicking on the header of the graph column, the graph scale shouldn't shift.
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>