Problem/Motivation

When edit mode is enabled via the toolbar "Edit" button the contextual button become visible and the tabbing is constrained to the contextual buttons.

When you load a page with edit mode already enable the contextual buttons are visible on reload but the tabbing is no longer restricted.
Edit mode is suppose to persist when the page is reloaded see 

Drupal.contextualToolbar.VisualView.persist()

This a problem for instance when you click "Configure Block". This will take you the Edit block form. If you either save the block form or click the "Back to Site" link when return the original page Edit mode will be in effect but the tabbing will not be constrained.

Proposed resolution

Ensure tabbing is constrained is edit mode is enabled on page reload.

Remaining tasks

create tests for tabbing.
create patch fixing

User interface changes

Tabbing will be restricted on page reloaded

API changes

None

Data model changes

None

CommentFileSizeAuthor
#11 2919147-11.patch4.63 KBtedbow
#11 interdiff-8-11.txt1.38 KBtedbow
#8 2919147-8.patch4.62 KBtedbow
#8 interdiff-5-8.txt2.65 KBtedbow
#8 2919147-8-TEST_ONLY.patch3.56 KBtedbow
#5 2919147-5.patch4.83 KBtedbow
#5 interdiff-2-5.txt3.12 KBtedbow
#5 2919147-5-TEST-ONLY.patch3.76 KBtedbow
#2 2919147-2-annouce_on_reload.patch3.06 KBtedbow
#2 2919147-2-annouce_on_reload-test-only.patch1.99 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2919147-2-annouce_on_reload-test-only.patch1.99 KB
2919147-2-annouce_on_reload.patch3.06 KB

Ok lets start by updating the existing test \Drupal\Tests\contextual\FunctionalJavascript\EditModeTest::testAnnounceEditMode
to reload the page twice to confirm that the tabbing constraint is announced with the second page load when edit mode will already be enabled.

I think this is the correct behavior. If I look at the toolbar tray items if they are already open when the page is loaded this does get announced. So this would make edit mode act the same way.

The test-only patch should fail.

The fix in the other patch does work to announce and constrain the tabbing but I have just noticed that on page reload the message is

Tabbing is constrained to a set of 0 contextual links and the edit mode toggle.
Press the esc key to exit.

So we will have to call Drupal.contextualToolbar.AuralView.manageTabbing at a later point after the contextual links are load on the page(even though the tabbing is actually constrained to the correct links).

The last submitted patch, 2: 2919147-2-annouce_on_reload-test-only.patch, failed testing. View results

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Issue tags: -a11y

removing "a11y" tag since we aren't using it.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2919147-5-TEST-ONLY.patch3.76 KB
interdiff-2-5.txt3.12 KB
2919147-5.patch4.83 KB

Ok I tried to figure how to test the tabbing restriction by pressing tab and count how many elements were tabbed to because you can't get the currently focused element from $this->getSession()->getPage().
I tried a bunch of things such as executeScript() find the currently active element marking it with an attribute and then retrieving it from $page. Nothing worked well.

So instead I created \Drupal\Tests\contextual\FunctionalJavascript\EditModeTest::getTabbableElementsCount. Which with a couple calls to \Behat\Mink\Session::executeScript can find the count.

So this test now asserts that the tabbing is restricted in edit mode.

The last submitted patch, 5: 2919147-5-TEST-ONLY.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Title: When edit mode is enabled Contextual tabbing is not constrained on page reload » When edit mode is enabled new page loads will not have Contextual tabbing constrained
Issue summary: View changes

Updated summary to include "Configure Block" example.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2919147-8-TEST_ONLY.patch3.56 KB
interdiff-5-8.txt2.65 KB
2919147-8.patch4.62 KB

Cleaned up the test changes. Variable names and comments.

The last submitted patch, 8: 2919147-8-TEST_ONLY.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
  1. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -44,27 +44,45 @@ protected function setUp() {
+    for ($page_get_cnt = 1; $page_get_cnt <= 2; $page_get_cnt++) {

    Please! rename this variable.
    Also starting at 0 and going to < 2 is clearer IMO 

  2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -44,27 +44,45 @@ protected function setUp() {
+        $this->assertTrue($unrestricted_tab_count > $expected_restricted_tab_count);

    Can use assertGreaterThan()

Otherwise, looks good!

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-8-11.txt1.38 KB
2919147-11.patch4.63 KB

@tim.plunkett thanks for the review!

fixed issues in #10

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2919147-11.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Reviewed & tested by the community

🤷‍♀️

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.5.x. Thanks!

  • webchick committed f9445b4 on 8.5.x 
    Issue #2919147 by tedbow, tim.plunkett: When edit mode is enabled new...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.