Fix sidebar collapse regression #1409
Conversation
There was a problem hiding this comment.
This fix is not appropriate, as it reverts the original commit, and therefore reintroduces the duplicate HTML element ID that was removed as part of PR #1297.
I don't have time right now to test if that would actually work in all cases, but I believe that changing the other sidebar div's ID (see layout_sidebar_begin() function above) would be sufficient to both fix the regression and avoid duplicate IDs.
If not, the proper fix will be more complex, likely requiring changes at javascript level.
|
I could also remove the id in |
|
It might work too, I didn't test. I'm fine either way (as long as it works 😉) but it may indeed be better not to have an ID if it's not being used. |
atrol
force-pushed
the
24976-sidebar-collapse
branch
from
141ba3a
to
c4e7dab
Compare
Removing the ID works at first sight, updated the PR. |
|
Did some more tests. Of course, it will reintroduce the duplicate HTML element ID, but it seems to work (tested on various desktop browsers and various window sizes, no mobile tests). Sorry @dregad I can't invest time for a clean solution. |
atrol
force-pushed
the
24976-sidebar-collapse
branch
from
c4e7dab
to
5b22430
Compare
|
The problem with duplicated ID is that it is impossible to reliably predict which element will be selected e.g. when using The erratic behavior you noticed during testing is a direct consequence of that - some parts of the code expect to reference the "button" div (e.g. common.js), while others are targetting the actual sidebar div. The bug was introduced in 8a3eedc (when @syncguru added sidebar collapse state persistence), and is a mix of referencing the wrong element (since they share the same ID) and inconsistent logic to retrieve the sidebar div's name to store it in the cookie. I'll push a commit with what I believe is the proper fix shortly. As far as I can tell, the regression is gone and the narrow screen issue with the burger menu is not present, |
dregad
force-pushed
the
24976-sidebar-collapse
branch
from
0b78319
to
089864c
Compare
There was a problem hiding this comment.
Thanks @dregad your fix looks good.
I tried different screen sizes on latest versions of Firefox, Chrome and Safari on MacOS, IE11 and Edge on Windows 10 and Chrome on Android.
Can't try latest iOS at the moment, but risk is low, so I will merge.
Regression introduced in version 2.13.0 commit b7b9145
The initial attempt at fixing this issue [1] introduced a regression in the persistence of the sidebar's collapse state. Using a different ID for the toggle button or the sidebar div caused other issues (e.g. malfunctioning hambuger menu on narrow screens, or failure to save collapse state). The root cause was a bug in the javascript code saving the sidebar's state, which was referencing the toggle button's id instead of the sidebar div's. We now have ID 'sidebar' for the actual sidebar, and 'sidebar-btn' for the toggle button, and the javascript has been modified accordingly. Fixes #24976 [1]: commit b7b9145
- replace the old collapse state using a regex - use a single reference to the sidebar div - calculate collapse state instead of using if statement
atrol
force-pushed
the
24976-sidebar-collapse
branch
from
089864c
to
14b9582
Compare
Regression introduced in version 2.13.0 commit b7b9145
Fixes #24976