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.

Comments

dmsmidt created an issue. See original summary.

dmsmidt’s picture

Status: Active » Needs review
FileSize
838 bytes

Here is a patch that fixes the problem.

drpal’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I think this should be testable.

tedbow’s picture

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

Status: Needs work » Needs review
FileSize
3.98 KB
3.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

Status: Needs work » Needs review

Forgot to upload TEST_ONLY first, regular patch passed.

dmsmidt’s picture

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

Status: Needs work » Needs review
FileSize
874 bytes
4.2 KB
drpal’s picture

- reroll due to es6 implementation.

tedbow’s picture

#10 and #11 look to me.

I think ready to RTBC but I wrote the tests.

tim.plunkett’s picture

  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

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

Status: Needs work » Needs review
FileSize
1.95 KB
4.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

Status: Needs review » Reviewed & tested by the community

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

drpal’s picture

🎉

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

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.