Skip to content

fix(NcAppNavigation): deactivate focus on resize#8646

Merged
Antreesy merged 1 commit into
mainfrom
fix/noid/trap
Jun 19, 2026
Merged

fix(NcAppNavigation): deactivate focus on resize#8646
Antreesy merged 1 commit into
mainfrom
fix/noid/trap

Conversation

@Antreesy

@Antreesy Antreesy commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

☑️ Resolves

  • Fix case when isMobile changed true -> false with open: true was never considered, and trap stayed active

Reproduction steps:

  1. Load the app on ≥ 1024px — isMobile = false, open = true
  2. Resize window to below 1024px — isMobile = true → watch(isMobile) fires → open changes from true to false → watch(open) fires → toggleFocusTrap() called, trap deactivates
  3. Open the navigation on mobile — open = true → watch(open) fires → toggleFocusTrap() called → isMobile=true && open=true → trap activates
  4. Resize back to ≥ 1024px — isMobile = false → watch(isMobile) fires → sets open.value = !false = true.
  • open was already true → Vue sees no change → watch(open) does NOT fire.
  • toggleFocusTrap() is never called.
  • Focus-trap remains active on desktop. ← BUG

🚧 Tasks

  • Manual tests:
    Bug is happening sometimes on production, so far it's the only reliable steps to reproduce it

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@Antreesy Antreesy added this to the 9.8.3 milestone Jun 17, 2026
@Antreesy
Antreesy requested review from ShGKme and susnux June 17, 2026 15:06
@Antreesy Antreesy self-assigned this Jun 17, 2026
@Antreesy Antreesy added bug Something isn't working 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component labels Jun 17, 2026
@Antreesy

Copy link
Copy Markdown
Contributor Author

stable8 has this handler in both watchers, so backports are not needed:

	watch: {
		isMobile(value) {
			this.open = !value
			this.toggleFocusTrap()
		},

		open(value) {
			this.toggleFocusTrap()
			emit('navigation-toggled', {
				open: value,
			})
		},
	},
	```

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.06%. Comparing base (65a6f32) to head (c05b977).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8646      +/-   ##
==========================================
+ Coverage   54.02%   54.06%   +0.04%     
==========================================
  Files         106      106              
  Lines        3478     3479       +1     
  Branches     1008     1008              
==========================================
+ Hits         1879     1881       +2     
+ Misses       1355     1354       -1     
  Partials      244      244              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- case when `isMobile` changed `true -> false` with `open: true` was never considered, and trap stayed active

Assisted-by: ClaudeCode:claude-sonnet-4-6
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy
Antreesy merged commit 5660951 into main Jun 19, 2026
27 checks passed
@Antreesy
Antreesy deleted the fix/noid/trap branch June 19, 2026 13:42
@susnux susnux modified the milestones: 9.8.3, 9.9.0 Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working feature: app-navigation Related to the app-navigation component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants