Changed position for data scroll token

[DantrazTrev]

Moved the data-scroll-token back to li

Fixes element-hq/element-web/issues/17567

From how much I could understand the changes that occurred in #6079

Specifically changing the EventTile to be rendered as li and changing the data-scroll-token to be in the list

matrix-react-sdk/src/components/structures/MessagePanel.js

Lines 653 to 654 in abfe6d8

	as="li"
	data-scroll-tokens={scrollToken}

In short data-scroll-tokens were missing and caused the scrollstate to be back at the beginning as no scroll state is saved resets to start after every state change

---

Detailed Explanation (That might help find an alternative rather than just reverting it back )

This specifically caused a few issues namely the EventTile list due to being rendered as li is now a series of divs that don't make use of much of the data-scroll-token (Screen shot below)

This is really different from the list with data-scroll-token from the <li> approach where the EventTile is wrapped around an li element with data-scroll-token

The token is important as it is used in the ScrollPanel's getScrollState to mark the scroll state of the list.

This caused the bug so every state update for FilePanel or its children caused it to reset to the end of the list. This explains the part in element-hq/element-web/issues/17567 where onFillRequest causes backpaginating state to change.

An alternative could be adding the token to the EventTile but for now this might suffice.

PS: The failing tests are the ones that had been adjusted previously for the later approach, can be updated if we get approval for this change, otherwise the current approach could be changed to fit with them

Signed-off-by: Ayush Pratap Singh [email protected]

[germain-gg]

Thank you for working on this task!

The removal of the list item surrounding the EventTile was intentional and is probably something that we'd like to keep. Having two elements does not serve any purpose in this case and inflates the DOM size which can lead to a slow down in runtime performances

What I didn't notice unfortunately is that there is a couple of return statement in EventTile.tsx meaning that the root element of that component does not always have the same props

I'd recommend you to update the following to change the tag to a li and add a data-scroll-tokens attribute

matrix-react-sdk/src/components/views/rooms/EventTile.tsx

Line 1049 in 00dd6e8

<div className={classes} aria-live={ariaLive} aria-atomic="true">

matrix-react-sdk/src/components/views/rooms/EventTile.tsx

Line 1077 in 00dd6e8

<div className={classes} aria-live={ariaLive} aria-atomic="true">

matrix-react-sdk/src/components/views/rooms/EventTile.tsx

Line 1114 in 00dd6e8

<div className={classes} aria-live={ariaLive} aria-atomic="true">

[t3chguy]

Hmm, doesn't having random child divs in the <ol> mess with the semantics and thus the accessibility tree here? Those divs don't have the appropriate role set if they're meant to be list items. I don't think I was involved in the original review but this PR not only fixes what it claims but also an A11Y regression.

I guess @gsouquet recommendation also resolves it though, as long as all children are considered, e.g also things like

matrix-react-sdk/src/components/views/elements/EventListSummary.tsx

Line 66 in 8a9d38b

<div className="mx_EventListSummary" data-scroll-tokens={eventIds}>

[germain-gg]

Yes, I do believe those div should be updated to be li. and have a way to update the tag name depending on the context. That was the intention with the original as prop but I missed those three return statements in EventTile.tsx

[t3chguy]

but I missed those three return statements in EventTile.tsx

And the pass-thru mode of EventListSummary presumably?

[germain-gg]

Thank you again for your contribution @DantrazTrev

Unfortunately due to timing constraint due to the severity of the issue I had to write a fix as part of #6154 that pretty much follows the requested changes that me and Michael provided in this PR

[DantrazTrev]

Thank you again for your contribution @DantrazTrev

Unfortunately due to timing constraint due to the severity of the issue I had to write a fix as part of #6154 that pretty much follows the requested changes that me and Michael provided in this PR

No issues I was stuck in a bit of an academic program. I'll work on this element-hq/element-web/issues/17561 which also regressed in the same PR