useKResponsiveWindow update fixing resizing behavior in Learn > Library

[thanksameeelian]

Description

this PR fixes a bug observed during QA review of Kolibri in Learn > Library - in certain cases, rapidly resizing the browser window would cause the sidepanel to automatically display when the screen was small, behavior we did not expect nor want at that screen size. beneath the surface, the components were receiving conflicting information about the current screen size because useKResponsiveWindow was not updating in the way we expected.

to address those issues, this PR introduces changes to the scope of variables within useKResponsiveWindow and to the way the composable uses useKWindowDimensions.

Issue addressed

reported as a Kolibri issue

Before/after screenshots

BEFORE, courtesy of @pcenov:

2023-09-08_17-03-37.mp4

AFTER (just a boring video of me resizing successfully):

resizing-successfully.mov

Steps to test

Please see reviewer guidance provided in learningequality/kolibri#11315 & test behavior of this change on that kolibri branch

Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

this doesn't necessarily introduce tech debt, but has revealed some existing underlying issues regarding the functioning of KResponsiveWindowMixin (which is still in use in many places), as it is likely attaching an excessive amount of listeners which could have a performance impact. there is an issue forthcoming for retrofitting the mixin to use the composable updated here to reduce the amount of unnecessary and sometimes detrimental extra listeners.

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 has been added to the changelog

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests?
• Are all UI components LTR and RTL compliant (if applicable)?

[MisRob]

@thanksameeelian I just created the release branch and retargeted this PR into it since KDS 1.5. is going to Kolibri's 0.16. Could you please rebase on top the lates release-v1.5.x branch to get the latest changelog updates into this PR? Thank you.

[pcenov]

Hi @thanksameeelian I confirm that the issue described in learningequality/kolibri#11212 is fixed now.

[nucleogenesis]

Code looks great - love the full test coverage of the changes here. I tested the associated Kolibri PR as well and tried out several areas of the app resizing windows without issue.

[MisRob]

Thanks for all the debugging and reworking the composable @thanksameeelian, this will be an impactful update for all the products. The PR as a whole looks like a high quality work.

I can see one important performance regression. Previously, we calculated breakpoints and all derived info, such aswindowIsSmalletc., by using media queries but now we rely heavily on useKWindowDimensions instead. I understand that the current implementation using local scopes is problematic and agree that moving them to global scope and making sure we don’t initialize same listeners many times is a good, robust solution.

However, I’m concerned about turning away from media queries as radically as removing them completely. Building everything around useKWindowDimensions which reads window.innerWidth/Height on every resize event has some performance issues that matchMedia solves. Relatedly, reading innerWidth/Height causes layout/reflow which can be a significant performance bottleneck.

(After it's introduced more widely instead of the mixin, strictly speaking -->) In Kolibri, we rarely need reach to window.innerWidth/Height as in the majority of use cases, we only access rough breakpoints that don’t need to use that logic. For illustration, see windowWidth usages (neads to use innerWidth, used 3x) vs windowIsSmall usages (doesn’t need to use innerWidth, used 58x and there's much more similar props). Building calculations for rough breakpoints around useKWindowDimensionswould eventually significantly change this. For that reason, I suggest that we don't use it whenever possible. If I recall, introducing media queries to the composable was an intentional step suggested by one of the team members for improving performance.

In this PR, I imagine we could keep the best of both worlds - using global props and making sure we don’t initialize listeners often than needed like you already did while still updating breakpoints by using media queries.

If this was something you already tried out and experienced some technical blockers I missed, let me know any time and we can have a look at it.

[MisRob]

Also opened related issue #461. This won't solve the above but should help in situations we absolutely need to read window.innerWidth/Height.

[MisRob]

For reference, you may also briefly look at

• Kolibri hangs loading content from Ciencia NASA (topics have thousands of child nodes) kolibri#7525
• https://develop--kolibri-design-system.netlify.app/kresponsiveelement/#guidelines
• Re-evaluate usage of TextTruncator and optimize its performance kolibri#8124
• Document KResponsiveElementMixin performance #237

where it's illustrated that we already experienced pretty serious performance problems. It's not the same situation, but similar issues were one of the contributing factors.

[rtibbles]

In this PR, I imagine we could keep the best of both worlds - using global props and making sure we don’t initialize listeners often than needed like you already did while still updating breakpoints by using media queries.

If this was something you already tried out and experienced some technical blockers I missed, let me know any time and we can have a look at it.

The use of mediaQueries was the cause of the main bug here, the move to globally registered listeners was to fix this while not causing a huge performance regression (by having a global variable to track, rather than multiple listeners for window resize).

Because the Media Query API requires a specific query, we were obliged to have a listener registered to the media query for every breakpoint - this meant that listeners were being triggered during a resize event for each media query as it changed, and then frequently updating the breakpoint value out of order. Because the media query listeners are triggered independently of each other, the only safe way to use them is when there is a 1:1 correlation between the media query and the value we are tracking (so using it to detect potrait mode is safe, because we are only using 1 media query listener to track it).

I don't see that @thanksameeelian's changes will cause a performance regression here, as we were already attaching the global window resize event listener to update the windowHeight and windowWidth by using the useKWindowDimensions composable from within useKResponsiveWindow - so any performance degradation caused by that resize listener is pre-existing.

[MisRob]

we were already attaching the global window resize event listener to update the windowHeight and windowWidth by using the useKWindowDimensions composable from within useKResponsiveWindow

I assumed we'd move away from attaching it completely eventually whenever possible. The main point was trying to use it less and less rather than more and more. I didn't realize there's a blocker though as mentioned below.

Because the Media Query API requires a specific query, we were obliged to have a listener registered to the media query for every breakpoint - this meant that listeners were being triggered during a resize event for each media query as it changed, and then frequently updating the breakpoint value out of order. Because the media query listeners are triggered independently of each other, the only safe way to use them is when there is a 1:1 correlation between the media query and the value we are tracking (so using it to detect potrait mode is safe, because we are only using 1 media query listener to track it).

I had to misunderstood the motivation for this bugfix then, it seemed to me that the main issue were local props + I didn't realize that independent working of more media queries is the main culprit. Consider my comment irrelevant, apologies. Thanks for clarification.

Not relevant

[rtibbles]

All changes make sense - the follow up issue filed by @MisRob will be nice to prevent excess recalculations even more so than previously too.

[MisRob]

@rtibbles @thanksameeelian For my better understanding of the issue and to know whether there's some space to do some improvements in the future or no, can I understand right that you see no way of using media queries at all for similar use cases? I wonder if reformulating the queries

from

 const queries = [
    '(max-width: 480px)',
    '(max-width: 600px)',
    '(max-width: 840px)',
    ...
  ];

to

 const queries = [
    '(max-width: 480px)',
    '(min-width: 481px) and (max-width: 600px)',
    '(min-width: 600px) and (max-width: 840px)',
    ...
  ];

would help to resolve some of the problems, particularly the 1:1 correlation. Or was it about something else?

That said, I don't know though how the number of around 8 listeners would compare to reading innerWidth/Height frequently performance wise, that might need some research. For now, I'd only like to understand better media queries issues to see if there are some ways still open towards planning some optimizations in this area eventually, taking into account wide usage.

[rtibbles]

It's possible that would ameliorate it, but the issue is more down to the fact that multiple callbacks can trigger a value update for the breakpoint - whereas a single callback that comes from watching both the width and height prevents this.

[MisRob]

the issue is more down to the fact that multiple callbacks can trigger a value update for the breakpoint

This means that more media queries, in general, should never update the same value, is that right? Here we'd need to rather have more variables instead of one, e.g. isBreakpoint0 that would map to the first query, isBreakpoint1 that would map to the second, etc. for them to work as expected? That's not a suggestion at this point, just an example to confirm I get their limitations as I wouldn't expect this before diving deeper here.

[rtibbles]

Yes, I think that would work - the only concern would be having multiple sources of truth - but if the media queries are working appropriately, they should all agree.

I do think the performance concerns will be mostly covered by #461 though, rather than using media queries.

[MisRob]

@rtibbles My curiosity is satisfied :) Thanks for going into detail.

[MisRob]

I do think the performance concerns will be mostly covered by #461 though, rather than using media queries.

I hope so, yes, especially if we make sure we keep using it globally.

[rtibbles]

One other possible follow up would be to retrofit the mixin to use the now globally scoped composable, which would remove all current performance concerns as well.

[MisRob]

One other possible follow up would be to retrofit the mixin to use the now globally scoped composable, which would remove all current performance concerns as well.

That'd be helpful for the current state of things for sure. I don't know whether that's high priority at this point, my concern was originally rather about not removing something that was meant to improve situation, however I didn't get that it was a deliberate step rather than just a side effect of refactoring of variables scope.

We also have learningequality/kolibri#11321 and #459 which will help us to move away from the mixin completely eventually.

I imagine that refactor of the mixin wouldn't be complicated, perhaps it's rather QA that would take time due to this logic being used frequently.