Add support for multi-row sticky headers with scroll wrapper

[grant-cleary]

I'll preface this PR with the note that this feels like a horrible solution to this problem, but I'm not super convinced a good solution exists. If it does, it escapes me personally, and hopefully someone will have a flash of insight that will help here. I originally hoped to ditch all of the manual width syncing we do in this case and rely on the browser to handle it, but unfortunately that turns out to be completely incompatible with the scroll wrapper.

This PR attempts to add support for multi-row sticky headers in tables so this can be adopted properly within LMS grids that need it (e.g. Enter Grades). I go about this using the following rough algorithm:

1. Make up a map of the table head that identifies column and row coordinates for every cell in the head.
2. For each column in the grid, find a row in this map in which a cell exists with colspan = 1. Assemble these cells into a pseudo-row that we can use to sync cell widths with the body.
3. Grab a representative body row (the first row that has one cell per column, i.e. colspan = 1 for all columns) and compare this cell by cell with the representative pseudo-row constructed in (2), adjusting widths accordingly using the same logic the component currently uses.
4. Let the browser sort the rest (e.g. cells with colspan > 1 or rowspan > 1).

This algorithm requires the following assumptions:

1. There will be at least one head row for each column in which that column contains a single cell with no column spanning.
2. There will be at least one body row in which no cell spans more than one column. This assumption could technically be relaxed, but I don't want to have to search every body row the same way I do for the head, given that data can technically be unbounded.
3. Both the head and body have balanced columns. i.e. All head and body rows have the same number of columns (though not necessarily the same number of cells).

... And that's more or less where I'm at. This seems to work okay, though I note that it doesn't render properly in the component demo on first render (it requires a resize to snap into place). I'm not convinced this is a result of my change, however, since this seems to be true on main as well. I'm going to look into how implementable this is in the LMS, but I would like some feedback on this monstrosity first, if possible.

[github-actions[bot]]

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL	https://live.d2l.dev/prs/BrightspaceUI/core/pr-4877/

[!NOTE] The build needs to finish before your changes are deployed. Changes to the PR will automatically update the instance.

[grant-cleary]

I'll deal with the vdiff goldens if we determine we're alright with this (or alternatively if we've figured out a better path forward).

[grant-cleary]

I think I'm following this mostly, and am not having a "flash of insight" on another innovative approach.

Not going to lie; I was kind of hoping I was just missing something that would make this a lot more pleasant. 😆

[dbatiste]

Hey Grant, apologies for the delay looking at this. Just getting my bearing... is there any chance you can attach a screenshot showing the issue we're trying to solve? I see something in the demo which I believe is simply due to how the demo is configured, but I am unsure.

[grant-cleary]

Hey Grant, apologies for the delay looking at this. Just getting my bearing... is there any chance you can attach a screenshot showing the issue we're trying to solve? I see something in the demo which I believe is simply due to how the demo is configured, but I am unsure.

All good, no worries on the delay. I figured this would need time for discussion and thought. I can get a screenshot a little later today to show you what's up, but the gist of it is that if a grid has multiple head rows and the first row has at least one cell with colSpan > 1, the sticky-positioning-with-scroll-wrapper functionality completely breaks. The grid doesn't size correctly and is entirely unusable.

[grant-cleary]

@dbatiste, sorry for the delay... I was trying to get a screenshot on my LMS instance where the issue is way more pronounced, but I realize I had wiped out my changes on there a bit ago, so an example from the core demo will have to do. All I did with this table was update the demo to match what I have in this PR, but without the table-wrapper changes. You can see that it's not syncing the column widths properly because the code assumes that all header and body rows will have a uniform number of cells.

[dlockhart]

Do you remember what got us to this point?

That was our stance -- that sticky headers combined with scroll wrappers were not possible. Then support was added last April, although I'm not totally sure what prompted it TBH (I see no Rally tickets associated).

I agree though... in retrospect it seems like a lot of hackery -- although ultimately if all this helps us clean up what grades is doing and bring everything into alignment, it might be worth it.

[grant-cleary]

I agree though... in retrospect it seems like a lot of hackery -- although ultimately if all this helps us clean up what grades is doing and bring everything into alignment, it might be worth it.

In some sense I think this was kind of inevitable though. What Grades is doing is pretty unsustainable and it's such a core workflow... I think it was bound to get to the point where we needed some solution there. Absent some complete overhaul of what's in that grid, I think we're just caught between a rock and a hard place.

[grant-cleary]

Given that we seem to have grudgingly accepted this as the way forward, I'll make those test changes and clean up vdiff as necessary. I'm leery about merging something like this so late in the release, so I'll likely give it until Monday to make sure this has as much soak time as possible.

[grant-cleary]

... I mean vdiff failed, but is it just me, or do the existing goldens just look broken?

[margaree]

... I mean vdiff failed, but is it just me, or do the existing goldens just look broken?

Agreed! It looks like it was me who did that and I'm not sure why. They now look correct to me.

[grant-cleary]

Agreed! It looks like it was me who did that and I'm not sure why. They now look correct to me.

Cool beans. Wanted to make sure I wasn't getting some weird/broken results. 😆

[grant-cleary]

@dbatiste, @dlockhart - I think this should be good to go now, unless I've forgotten/missed something.

[d2l-github-release-tokens[bot]]

:tada: This PR is included in version 3.34.0 :tada:

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot :package::rocket: