Enabling "Edit" to show all contextual links breaks tabbing in edit forms in the backend

Perhaps we deactivate it if there are no items to tab to?

is it desired to stay enabled? Maybe it defaults to off when page is loaded. If it should stay enabled shouldn't that be a setting somewhere?

Something like this?

Couple of unintended changes in that file

Sorry! But what do you think about turning that one feature off?

It's there for accessibility, we can't turn it off unconditionally, but maybe we can turn it off if there's no contextual links on the page

Is it still an accessibility issue if there are contextual links and tabable components on the page?

The intent of the edit button is to trap tabbing to only contextual links when its enabled.

Gotcha. So the javascript seems to run before the page load. Any suggestion on how to check the contents?

Thoughts?

This should hopefully do it.

1. +++ b/core/modules/contextual/js/contextual.toolbar.js
   @@ -32,8 +32,9 @@
   +          localStorage.getItem('Drupal.contextualToolbar.isViewing') !== 'false'
   +            ? true
   +            : !$('body .contextual-region').length,
   

   I think we still need to refine this.
   Steps to reproduce:
   * Visit a page with contextual links, toggle into edit mode
   * Visit a page without contextual links, because it was previously set in your local storage, edit mode remains on and tabbing is broken.

   So in other words, I think we need the lack of contextual region items to trump the local storage option.

2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
   @@ -86,6 +100,12 @@ public function testEditModeEnableDisable() {
   +      // Test while Edit Mode is enabled it doesn't interfere with pages with
   +      // no contextual links.
   +      $this->drupalGet('admin/structure/block');
   +      $web_assert->elementContains('css', 'h1.page-title', 'Block layout');
   +      $this->assertGreaterThan(0, $this->getTabbableElementsCount());
   

   We also need a negative assert here that there are no contextual links/regions on this page.

Like this?

Adding WCAG 2.1.1 tag

+++ b/core/modules/contextual/js/contextual.toolbar.js
@@ -31,9 +31,10 @@
+        isViewing: !$('body .contextual-region').length
+          ? true
+          : localStorage.getItem('Drupal.contextualToolbar.isViewing') !==
+            'false',

I found this hard to read - I think we can do this

document.querySelector('body .contextual-region') === null || localStorage.getItem('Drupal.contextualToolbar.isViewing') !== 'false'

and ditch jQuery too

Didn't manually test, but will keep on my list to do so (keeping at NR)

Made the change in #29 and quick testing shows it still works.

Enabled contextual links on an Article page
Went to /admin/content, where there are no contextual links and could tab normally.

Manually tested this and it works as designed.

Before: tabbing doesn't work
After: tabbing works

Thanks for accommodation lots of rounds of feedback here @smustgrave

Random failure

Who knows how often this tripped someone up? t seems like the kind of problem someone might attribute to their own user error when it's actually a fixable bug. Good fix and solid test coverage in the patch.

I committed to 10.1.x, and I'm checking to see if commit checks pass for 10.0.x to cherry-pick to account for any possible lint/formatting requirements.

Cherry picked this to 10.0.x. This addresses a bug that could otherwise be quite frustrating, so backporting this to 9.5.x would be nice to do. This will need a 9.5.x specific patch as 9x requires .es6.js files. Setting issue status to "Patch (to be ported)" and the provider of the patch can switch back to RTBC or Needs Review (depending on their patch-porting confidence level)

I have attached patch for 9.5.x. please review

Random failure

Fixed test failures, please review.

So even if it's a random failure we wouldn't solve it here. It would need to be it's own ticket.

Also the patch in #43 doesn't include the original patch so hiding that.

9.5 is now security only, so marking this as fixed and moving version to 10.0