The Vector 2022 persistent saving should be reconsidered now that it saves to cookie as well as user preference. It also doesn't quite work as expected. In particular
- Should we reconsider the debounce time length? 1s is likely too high
- Make sure we are cancelling API calls when we trigger a new one to avoid conflicting saves.
- Improve UI rendering to happen outside the timeout
In T300826#8568297 @Od1n wrote:
The code to trigger the window resize event is inside the saveSidebarState() function, which has a 1000 ms debounce when it is called from bindSidebarClickEvent(). Thus, there is an undesirable delay of 1 second before repositioning the elements when the sidebar is toggled. This trigger of a "fake" window resize event should be decoupled from the API save, and executed immediately. This will result in a much snappier interface. And of course, triggering the fake window resize event and saving the state to API are two completely different things, which shouldn't be blended together.
In the current case, there is no need to throttle/debounce the handler, executing such event once (and in response to an user click) is very cheap, the issue might be it gets executed repeatedly, for example in a real window resize.
For the record, I noticed this issue when editing AbuseFilters, as there was a delay before the Ace editor got resized.
Another thing: in collapseSidebar() and expandSidebar() functions, note they call saveSidebarState()() in a way that every time they are called, they create and call a new debounced saveSidebarState function… The debouncing isn't coded correctly (there should be an single instance of the debounced function). As a result, it's basically as if the function wasn't debounced. Thus, the API save happens without debouncing, but fortunately, the event is triggered only when the media query passes through the max-width: 999px threshold.
Also notice the timeout of the debounce is 0 ms. I think it's intended so that the handler gets lower priority than the most important things such as recalculation of display. Still, the debouncing should be fixed as I exposed in the above paragraph. (edit: or maybe it's because the only way to make an API save (and nothing else) is to call saveSidebarState() with timeout=0 and shouldTriggerResize=false, as the code is nested into this function)
QA
Check sidebar
- As a logged in user, load page with sidebar pinned (visible by default)
- Hide the sidebar
- Quickly refresh the page
Expected: sidebar remains hidden
Check toggle
- As a logged in user, load page with limited width enabled
- Click the limited width toggle in bottom right corner to disable limited width
- Quickly refresh the page
Expected: limited width remains disabled.
Check editor
When reviewing this task, please take care to ensure the patches y'all write to fix this issue do not "dislodge" / "undo" the fixes written in T328121:
- Visit a page in Vector 2022
- Open the visual editor
- Toggle limited content width using the tiny button in bottom-right corner
- Check that the position and width of the visual editor toolbar reflects the new position and width of the page content
- Also repeat the above: when starting from limited vs unlimited content width when opening the editor; when scrolled down on a long page and opening the editor from the sticky header