Currently when you enable Editing mode, leave editing mode and then enable it again.
The announcement message is wrong.

ChromeVox reads:

Tabbing is no longer constrained by the Contextual module.
Tabbing is constrained to a set of 11 contextual links and the edit mode toggle.
Press the esc key to exit.

This should just be:

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

CommentFileSizeAuthor
#15 2863222-15.patch4.85 KBtedbow
#15 interdiff-2863222-11-15.txt1.95 KBtedbow
#11 interdiff-2863222-10-11.txt1.85 KBGrandmaGlassesRopeMan
#11 2863222-11.patch4.86 KBGrandmaGlassesRopeMan
#10 2863222-10.patch4.2 KBdmsmidt
#10 interdiff-2863222-5-10.txt874 bytesdmsmidt
#8 interdiff-2863222-5-8.txt4.42 KBdmsmidt
#8 2863222-8.patch954 bytesdmsmidt
#5 2863222-5-TEST_ONLY.patch3.16 KBtedbow
#5 2863222-5.patch3.98 KBtedbow
#2 2863222-2-outside_in-fix_announce_mode_switch.patch838 bytesdmsmidt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dmsmidt created an issue. See original summary.

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented
Status: Active » Needs review
FileSize
2863222-2-outside_in-fix_announce_mode_switch.patch838 bytes

Here is a patch that fixes the problem.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

I think this should be testable.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Title: Outside-in Accessibility: Announcement broken after enabling editing mode a second time » Contextual Accessibility: Announcement broken after enabling editing mode a second time
Component: outside_in.module » contextual.module

This actually happens with the Contextual module without the Settings Tray module enabled. So switching component. I will work on a test for this.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
2863222-5.patch3.98 KB
2863222-5-TEST_ONLY.patch3.16 KB

Ok I added a test to the patch. I also added a TEST_ONLY patch.

The TEST_ONLY patch should fail as

1) Drupal\Tests\contextual\FunctionalJavascript\EditModeTest::testAnnounceEditMode
Behat\Mink\Exception\ElementHtmlException: The string "Tabbing is no longer constrained by the Contextual module." appears in the HTML of the element matching css "#drupal-live-announce", but it should not.

........
/var/www/d8_2_ux/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php:61

So line 61 is when edit mode has been enabled and disabled then re-enabled.

This should prove the problem and the fix.

Status: Needs review » Needs work

The last submitted patch, 5: 2863222-5-TEST_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review

Forgot to upload TEST_ONLY first, regular patch passed.

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented
FileSize
2863222-8.patch954 bytes
interdiff-2863222-5-8.txt4.42 KB

Test looks good and works as advertised.

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -0,0 +1,97 @@
+    $this->pressToolbarEditButton();
+    $this->assertAnnounceEditMode();
+    $this->pressToolbarEditButton();
+    $this->assertAnnounceLeaveEditMode();
+    $this->pressToolbarEditButton();
+    $this->assertAnnounceEditMode();

This benefits from some extra documentation. Patch attached.

Edit: booo, something went wrong here. New patch comming up. (Note to self: Check first, upload later)

Status: Needs review » Needs work

The last submitted patch, 8: 2863222-8.patch, failed testing.

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented
Status: Needs work » Needs review
FileSize
interdiff-2863222-5-10.txt874 bytes
2863222-10.patch4.2 KB
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Issue tags: -Needs tests
FileSize
2863222-11.patch4.86 KB
interdiff-2863222-10-11.txt1.85 KB

- reroll due to es6 implementation.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

#10 and #11 look to me.

I think ready to RTBC but I wrote the tests.

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
@@ -0,0 +1,102 @@
+  const ANNOUNCE_SELECTOR = '#drupal-live-announce';
...
+    $edit_button = $this->getSession()->getPage()->find('css', static::EDIT_BUTTON_SELECTOR);
+    $edit_button->press();

    No need for a constant just for one usage. Also this could be a one-liner 

  2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -0,0 +1,102 @@
+  const EDIT_BUTTON_SELECTOR = '#toolbar-bar div.contextual-toolbar-tab button';

    Idk where this habit started, but I personally think it's overkill. Also afaik it would need a docblock. 

  3. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -0,0 +1,102 @@
+    $user = $this->createUser([
+      'administer blocks',
+      'access contextual links',
+      'access toolbar',
+    ]);
+    $this->drupalLogin($user);

    No need for the $user variable, can just wrap createUser in drupalLogin 

  4. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/EditModeTest.php
@@ -0,0 +1,102 @@
+   * Test Drupal.announce messages appear.
...
+   * Press the toolbar edit mode.
...
+   * Assert that the correct message was announced when entering edit mode.
...
+   * Assert that the correct message was announced when leaving edit mode.

    Tests, Presses, Asserts

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt commented
Status: Needs review » Needs work

As per Tims comments needs work.

However, 1): ANNOUNCE_SELECTOR is used four times. I guess 1) and 2) are about EDIT_BUTTON_SELECTOR. Then I agree.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2863222-11-15.txt1.95 KB
2863222-15.patch4.85 KB

@tim.plunkett thanks for the review 😀

re #13

1. removed EDIT_BUTTON_SELECTOR because it was only used once. Left ANNOUNCE_SELECTOR because it is used for times.
2. Added a doc block for ANNOUNCE_SELECTOR. I like using them for strings used more that once
3. ✔ fixed
4. ✔ fixed

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

Got my dreditor highlights off a bit, glad you understood what I meant.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

🎉

  • catch committed 193bab8 on 8.4.x 
    Issue #2863222 by dmsmidt, tedbow, drpal, tim.plunkett: Contextual...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

This doesn't cherry-pick cleanly to 8.3.x, I think it's OK for it to be in 8.4.x and not backported.

Status: Fixed » Closed (fixed)

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