Skip to content

Follow the mobile menu state instead of flipping the header class - #227

Draft
boo-code wants to merge 1 commit into
PrestaShop:developfrom
boo-code:fix/mobile-menu-class-follows-state-35939
Draft

Follow the mobile menu state instead of flipping the header class#227
boo-code wants to merge 1 commit into
PrestaShop:developfrom
boo-code:fix/mobile-menu-class-follows-state-35939

Conversation

@boo-code

@boo-code boo-code commented Sep 2, 2026

Copy link
Copy Markdown
Contributor
Questions Answers
Description? TopMenu.toggleMobileMenu() has two callers: the #menu-icon click, where a toggle is right because #mobile_top_menu_wrapper was just toggled, and prestashop.on('responsive update'), which fires on every crossing of the 768px breakpoint. A crossing opens and closes nothing, so the unconditional $('#header').toggleClass('is-open') flips the class for free.

The consequence is worse than a stray class. After an odd number of crossings is-open means the opposite of the menu state, and every later click preserves that inversion until the next crossing puts it back. The hide/show of #notifications, #wrapper, #footer in the same method was always driven by the wrapper's real visibility; the class is now driven by the same value.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#35939
Sponsor company -
How to test? Open any front office page with this theme, watch #header in the inspector, and drag the window across 768px without touching the menu. Before this change is-open appears on the way down and disappears on the way up; after it, it appears only when the mobile menu is actually open.

The sharper check is the phase: at mobile width, cross the breakpoint once, then open and close the menu. Before, the class is present while the menu is closed and absent while it is open.

Measured

Real browser, real window resizes, the theme's own compiled bundle. menuOpen is
#mobile_top_menu_wrapper visibility, isOpen is #header.is-open.

Fresh load already at mobile width, no crossing — this is the control, and it agrees:

                            menuOpen  isOpen  agree
loaded, menu untouched        false    false   yes
1st click (opened)            true     true    yes
2nd click (closed)            false    false   yes

Loaded at desktop, then resized down across 768px — one crossing:

                            menuOpen  isOpen  agree      after the fix
crossed, menu untouched       false    true    NO         false / false  yes
1st click (opened)            true     false   NO         true  / true   yes
2nd click (closed)            false    true    NO         false / false  yes

Three positions out of three inverted, and the only difference from the control is the crossing.
Restoring the unpatched bundle brings all three NOs back, with the served file checked for
toggleClass("is-open") versus toggleClass("is-open", in the same call that reads the table.

Menu left open at mobile width, then resized up: after the fix the class is removed and the page
content is restored, agree: yes.

Why the load path is quiet

responsive.js also emits responsive update from its own $(document).ready when the page loads
already narrow, which would toggle the class on load too. It does not, because that ready callback
is registered at import time (theme.js:33) and so runs before the one that constructs TopMenu
(theme.js:61) — at the moment of the load-time emit there is no listener. That is why the reporter
sees a clean class on a fresh mobile load and a wrong one only after resizing, and the control run
above confirms it.

Notes

  • The class is styled nowhere: is-open occurs once in the whole theme, the toggleClass itself,
    and zero times in its SCSS or compiled CSS. It exists to tell child themes and third-party scripts
    whether the mobile menu is open, which is exactly the contract the crossing breaks.
  • assets/ is gitignored in this repo, so no rebuilt bundle belongs in the diff. The build was run
    only to produce the numbers above (npm run build in _dev, exit 0; npm run lint on the file,
    exit 0).
  • The method keeps its name even though it now syncs rather than toggles, so a child theme
    overriding or calling toggleMobileMenu() is unaffected.
  • Reported against 8.1.x. The same code is in the 2.x branches; happy to backport if wanted.

toggleMobileMenu() is called from the menu button and from the
'responsive update' event, which fires on every breakpoint crossing.
A crossing opens and closes nothing, so the unconditional toggleClass
left #header.is-open inverted with respect to the menu until the next
crossing put it back.

Read #mobile_top_menu_wrapper's visibility once and drive both the class
and the content hiding from it, as the content hiding already did.

See PrestaShop/PrestaShop#35939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Header class ".is-open" doesn't appears as expected when browser window is resized

1 participant