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.

Members fund testing for the Drupal project. Drupal Association Learn more

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.

Status: Fixed » Closed (fixed)

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