Fix inner items size changes in KListWithOverflow and add appendToBody prop to KModal

[AlexVelezLl]

Description

This PR:

• Adds appendToBody prop to KModal to teleport the modal to the body element.
• Adds a ResizeObserver to the innerList of KListWithOverflow, to watch inner element size changes and recalculate the overflowed Items. This should definitely fix errors caused by browsers' tricky rendering patterns.

Issue addressed

Helps to fix learningequality/kolibri#12447.

Changelog

• #680

  • Description: Adds boolean appendToBody prop to teleport the modal to the body element if true.
  • Products impact: new API.
  • Addresses: Display issues for login language switcher kolibri#12447.
  • Components: KModal.
  • Breaking: no
  • Impacts a11y: no
  • Guidance:

• #680

  • Description: Fixes the calculation of overflowed items when changes in the size of the list item slots occur..
  • Products impact: bugfix.
  • Addresses: Display issues for login language switcher kolibri#12447.
  • Components: KListWithOverflow.
  • Breaking: no
  • Impacts a11y: no
  • Guidance:

Steps to test

In Kolibri, follow learningequality/kolibri#12447.

Implementation notes

At the beginning of the development of KListWithOverflow I was skeptical about including a ResizeObserver for the inner list items because I thought it could have a significant impact on performance. But adding the ResizeObserver to the parent div means we only need to observe one node, and using the throttled setOverflowItems makes the performance impact minimal.

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

After review

• The changelog item has been pasted to the CHANGELOG.md

Comments

[rtibbles]

Do we need this and the resize observer for the element (provided by the responsive element mixin) or is this new resize observer all that is actually needed?

[AlexVelezLl]

Yes, we need both, one to watch the size of the wrapper of the list (which width is mainly set by the parent element), and this new Resize Observer to watch the internal changes. For example, without the outher one, if we shrink the window, and then we expand it again, the list will remain shrinked because we are not watching the width changes of the parent.

[MisRob]

When unmounting the component, I think it'd be good to .unobserve. Or perhaps .disconnect would be better? https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

Side note, I think I don't quite understand the previous discussion and haven't connect the dots with the example you provided yet. Would you try to briefly reformulate what's the situation when list items are being resized while the parent is not? That's rather for my understanding and learning. If it helps with the issue we're seeing, it's alright.

[AlexVelezLl]

Thank you! I totally missed to disconnect the observer.

And yes, the situation with both watchers are that we have two divs, the outher one is the root div of KListWithOverflow which takes 100% of the space available, it sets the limits, and the inner one is the div of the list which width depends on how many items fit inside the KListWithOverflow

The outher one will shrink/expand if the caller component is shrinked/expanded, but the inner one will only shrink/expand if the overflow items are recalculated (because of the wrapper size change and we are watching it) and there is one more/less item visible:

But the thing is that the inner list items can also grow/shrink, and as we are not watching it, we cant recalculate the overflow items, and also because the inner list cant tell the outer one to expand/shrink to trigger the recalculation. So if some item is expanded we can get something like this:

So thats why we need to watch the inner list. But now, if we just watch the inner list, we wouldnt be watching if the outher div expands, so we can reach a situation like this:

Compartir.pantalla.-.2024-07-18.15_42_57.mp4

So thats why we need to watch the outher div also.

[MisRob]

Ah tricky, thanks so much for explaining in detail

[MisRob]

I wonder if it'd make sense to have appendTo prop that would accept a selector rather than appendToBody. The main reason being is that for some components, such as tooltips, it's not recommended to append them to body, but rather to a div that's very high-level in the DOM structure. You can see here for what I am playing around for tooltips in Studio learningequality/studio@1457085.

I don't know if this recommendation applies to modals yet, but am thinking that appendTo would allow for more flexibility in any case, and also would be consistent with the interface I was thinking about for KTooltip eventually.

This is really nothing major - if we have clearer picture in the future, I don't think it would be such a big deal to change it. Just for consideration if you'd find it helpful too.

[MisRob]

Overall looks good, thank you @AlexVelezLl. I think the only important thing to address is to make sure that the ResizeObserver doesn't persist after the component is destroyed.

[rtibbles]

I think my only reason for suggesting to just append to the body here is that because of the nature of a modal, where it is intended to 'take over' the whole screen, putting it in a child div rather than the body will always leave it vulnerable to the kind of cascading style overrides that the teleport is meant to avoid.

From a quick google, having appropriate focus trapping aria labels seems to be the most important thing for modals, so the considerations may be slightly different than for other UI elements. I assume the switch of focus to the modal would be sufficient for the UI element to be announced for screen readers, for example.

[MisRob]

Thanks @rtibbles, that's all good points to consider.

I imagined that when that <div> would be the first element in the body, hopefully there wouldn't be much trouble. However, it's true that this is different than tooltips that are much smaller and presumably appear more frequently than modals.

Side note - from a bit of reading I did on tooltips, the concerns some developers have about attaching tooltips to body are motivated by what seemed to be pretty severe performance issues. I bet there's some important a11y perspectives around appending, however I would guess that <div> won't really solve them. So it was the main reason for my note rather than a11y - for that I think that all the things you mention will show to be indeed more important.

[MisRob]

@AlexVelezLl I think now when we have at least one good reason to stay with the current implementation in regard to the prop interface, I think it'd be better to leave it as is

[rtibbles]

Ah, yes, I think I have found a similar article to what you were reading and that does seem to be an important consideration, attaching to the body will invalidate the entire render tree and cause layout of thrashing.

It does seem like having a dedicated modal/tooltip container would be a good thing to have. I think the parallel with the aria live region is apposite, but it has made me think that KDS could take responsibility for attaching the container in either case to the root Vue element at vue initialisation, so it is always present and prevents this thrashing.

I don't think we should fix this in this PR, but maybe we can change the prop to appendToRoot instead, so that the prop name matches our eventual target?

[MisRob]

Ah I see, and in this one they event tested modals and other components too, which I previously didn't find much information on

Other components like modal, popover, dropdown had similar performance issues. In some cases, a modal took more than 1 second to appear while making the UI unresponsive.

Your suggestion sounds good to me.

[AlexVelezLl]

Thank you @MisRob! I have updated the PR 👐

[MisRob]

Thank you, I think this is ready :)

[AlexVelezLl]

Thank you @MisRob!