Problem/Motivation

Some ajax-enabled buttons are no longer keyboard accessible:

  • Field UI - the widget and formatter settings buttons on manage display + manage form display tabs
  • Image field widgets - the "remove" button, seen on imagefields that already have an image
  • "Add another item" button on mutli-value field widgets.

Currently (D8.1.0) these buttons are keyboard-focusable, but do not respond to SPACE or ENTER.
This is working in D7.43, and was previously working in D8. It stopped working after #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation.

Proposed resolution

Restore the expected behaviour:

  • Using the keyboard, I can press TAB until focus reaches the cog-wheel settings icon.
  • Pressing ENTER or SPACE will open the settings form.

Remaining tasks

  • Find out where the regression happened - DONE, git bisect says commit 4c49e1e
  • Fix it, restore expected behaviour - DONE, patching core/misc/ajax.js
  • Possible to test? Use JavascriptTestBase and Mink's NodeElement::keyPress() method. It should test with the ENTER and spacebar keys.

User interface changes

None as such. This is about restoring keyboard-accessible behaviour that stopped working. No changes are intended for pointing devices such as mice.

API changes

None expected.

Data model changes

None expected.

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Priority: Normal » Major

Marking this as major as it prevents keyboard-only users from carrying out lots of site-building tasks.

Edit: Also affects some field widgets, so that means content editors are affected as well as site builders. See comments 10 and 11.

andrewmacpherson’s picture

The regression happened between 8.0.0-beta10 and 8.0.0-beta11.

andrewmacpherson’s picture

andrewmacpherson’s picture

I took a look though some of the other UI areas which use buttons to activate ajax forms (Views UI, quickedit) and didn't find any that weren't keyboard operable. So far the Field UI buttons are the only ones 've found that aren't keyboard-operable. I'll keep looking.

droplet’s picture

I closed my issue, your report have more details. :)

#2621344: Keyboard doesn't work in Ajax form

swentel’s picture

Component: field_ui.module » ajax system

This belongs essentially to the ajax system then, no ?

andrewmacpherson’s picture

@swentel, yes probably. Field UI was just where I first noticed this.

andrewmacpherson’s picture

I found a JS error. When attempting to operate the field UI formatter settings button, this error is reported:

ReferenceError: reference to undefined property ajax.element_settings.element
http://core.d8/core/misc/ajax.js?v=8.1.0-dev
Line 638
andrewmacpherson’s picture

Title: [regression] Formatter + Widget settings button is not keyboard operable » [regression] Some ajax-enabled buttons are not keyboard operable
Issue summary: View changes

I found another ajax button that doesn't work with the keyboard - the "remove" button on imagefield widgets.
Same error as in comment #10.

mckinzie25’s picture

I have also found a couple other ajax buttons that do not respond to the space bar or enter key.

  1. the "Remove" button for the file upload widget (similar to the "Remove" button issue mentioned by andrewmacpherson above).
  2. the "Add another item" button on the multiple textfield.

An interesting twist here is that when you hit the space key on these fields, the highlighting on the button in focus disappears, but when you hit the enter key, the highlighting remains. This shows that the button is at least responding to the space key in some way, but apparently not to the enter key.

I am going to look more into this at DrupalCon NOLA2016 mentored sprint.

andrewmacpherson’s picture

Issue summary: View changes

@mckinzie25 Thanks for reporting the add-another button, I've added it to the main summary.

The difference you report between space and enter is interesting - I hadn't noticed it before. I can see what you mean in Chrome, but it's not apparent in Firefox.

In Chrome, the focus style disappears while the space bar is held down, and reappears when releasing the key. I'm not sure that this confirms that a javascript handler is receiving the event; it might just be the web browser applying the :active pseudo-class.

mckinzie25’s picture

Thanks, @andrewmacpherson. This might help someone else go further with this issue, so here's what I have figured out. I have a hacky way to address the issue by adding one line to /core/misc/ajax.js at line 639. The line is $(element).mousedown(); at the end of the following code snippet. This causes the space bar and enter button to activate the "Add another item" and "Remove" buttons discussed above by triggering a mousedown event. But after discussing with other Drupalistas at NOLA2016, it seems like there should be a point in the code that is currently listening for mousedown events, and that that point in the code should also be listening for keypress events. That would probably be a good avenue for investigation, but this as far as I have reached on the issue today.

  /**
   * Handle a key press.
   *
   * The Ajax object will, if instructed, bind to a key press response. This
   * will test to see if the key press is valid to trigger this event and
   * if it is, trigger it for us and prevent other keypresses from triggering.
   * In this case we're handling RETURN and SPACEBAR keypresses (event codes 13
   * and 32. RETURN is often used to submit a form when in a textfield, and
   * SPACE is often used to activate an element without submitting.
   *
   * @param {HTMLElement} element
   *   Element the event was triggered on.
   * @param {jQuery.Event} event
   *   Triggered event.
   */
  Drupal.Ajax.prototype.keypressResponse = function (element, event) {
    // Create a synonym for this to reduce code confusion.
    var ajax = this;

    // Detect enter key and space bar and allow the standard response for them,
    // except for form elements of type 'text', 'tel', 'number' and 'textarea',
    // where the spacebar activation causes inappropriate activation if
    // #ajax['keypress'] is TRUE. On a text-type widget a space should always
    // be a space.
    if (event.which === 13 || (event.which === 32 && element.type !== 'text' &&
      element.type !== 'textarea' && element.type !== 'tel' && element.type !== 'number')) {
      event.preventDefault();
      event.stopPropagation();
      $(ajax.element_settings.element).trigger(ajax.element_settings.event);

      //New code to trigger mousedown on keypress event. -- mckinzie25
      $(element).mousedown();
    }
  };
andrewmacpherson’s picture

Issue summary: View changes
mckinzie25’s picture

Still working on this in my spare time. The spacebar and enter key trigger the desired event if you go to /core/misc/ajax.js and change

$(ajax.element_settings.element).trigger(ajax.element_settings.event);

to

$(element).trigger(ajax.element_settings.event);

in the Drupal.Ajax.prototype.keypressResponse function.

console.log(ajax.element_settings);

shows that the reason why the current code doesn't work is that ajax.element_settings.element is not set in Drupal.Ajax.prototype.keypressResponse. ajax.element_settings.event is set--it just needs the right element given to it to be triggered for.

So it looks like somewhere, presumably prior to the Drupal.Ajax.prototype.keypressResponse function in the process, ajax.element_settings.element should be set to the button element. I haven't figured out where this is supposed to happen yet, though.

andrewmacpherson’s picture

Thanks @mckinzie25! The approach in comment #15 works well. I tested it with all the buttons we know about (Field UI settings cog, remove image, add-another) and they all respond as expected to space/enter keypresses.

Are you familiar with how to produce a patch for Drupal core issues?

andrewmacpherson’s picture

This patch just expresses the fix that @mckinzie25 suggests in comment #15.

Status: Needs review » Needs work

The last submitted patch, 17: ajax-buttons-not-keybpoard-operable-2711907-17.patch, failed testing.

mckinzie25’s picture

Hmm, that's odd that it failed PHP testing. The patch doesn't even involve a PHP change.

andrewmacpherson’s picture

Status: Needs work » Needs review

Just one of the occasional glitches with the CI set up. I queued a new test and it's green now.

hchonov’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community

I just came across the same problem and for me the solution was the same as the element is now attached directly to the ajax instance and not stored anymore under ajax.element_settings.
Everything looks fine, it has been just an oversight.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: ajax-buttons-not-keybpoard-operable-2711907-17.patch, failed testing.

mckinzie25’s picture

Version: 8.2.x-dev » 8.3.x-dev
mckinzie25’s picture

mckinzie25’s picture

Status: Needs work » Needs review
mckinzie25’s picture

Status: Needs review » Reviewed & tested by the community

Just restoring status to "Reviewed & tested by the community," since the patch passed automatic testing for 8.3.x-dev.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: regression_some-2711907-24.patch, failed testing.

hchonov’s picture

Status: Needs work » Reviewed & tested by the community

It has been a CI error..

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: regression_some-2711907-24.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Tentatively putting back to RTBC.

alexpott’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Maybe we can add a test for this using JavascriptTestBase? Also this is a bug fix and should be against 8.2.x

andrewmacpherson’s picture

Issue summary: View changes
Status: Needs work » Needs review

I took a crack at writing a test. I'm new to this, so could do with some help.

So far, I figured how to write one, and got it running locally with phpunit, but the test passes regardless of whether the fix from the earlier patches is applied :-( So something's not right.

Where should we create a test? I made a test for the ajax formatter settings buttons in field_ui module, but this bug is really about ajax buttons anywhere they appear. Do we have to write a test for individual instances where the ajax button is used, or just one test for the component?

The test I wrote uses NodeElement::keyPress() to simulate pressing the ENTER key. This part is important, since we're fixing a keyboard a11y regression here. Here's the test function, using a user and field created in setUp().

  /**
   * Tests if the field formatter settings edit button can be operated with the
   * keyboard ENTER key.
   */
  public function testFieldUiFormatterSettingsButtonKeyboardEnter() {

    // Get a Field UI manage-display page.
    $this->drupalGet('admin/config/people/accounts/display');
    $session = $this->assertSession();
    $session->statusCodeEquals(200);
    $page = $this->getSession()->getPage();

    // Select a field formatter which has settings, so that the settings button
    // will appear.
    // @todo Specify the formatter in setUp() so we can remove this step.
    $select = $page->findById('edit-fields-field-text-test-type');
    $select->selectOption('string');
    $session->assertWaitOnAjaxRequest();

    // We can't know the exact ID of the edit button; it has a random
    // suffix after the ajax request.
    $button = $page->find('css', 'input[type=image][id^=edit-fields-field-text-test-settings-edit]');
    $button->focus();
    $button->keyPress(13); // ENTER key
    $session->assertWaitOnAjaxRequest();

    // We expect the edit button has gone.
    $button = $page->find('css', 'input[type=image][id^=edit-fields-field-text-test-settings-edit]');
    $this->assertTrue(empty($button), 'After pressing field formatter settings button, it should no longer be present.');

    // We expect a checkbox for the string formatter's link-to-entity setting.
    $checkbox = $page->find('css', 'input[id^=edit-fields-field-text-test-settings-edit-form-settings-link-to-entity]');
    $this->assertTrue($checkbox->isVisible(), 'After pressing formatter settings button, formatter settings fields are present.');
  }

The problem is that the test passes regardless of whether the fix from comment #17 is applied. Manual testing confirms that the patch makes a difference. Before the patch the button doesn't respond to the enter/spacebar key, and after the patch it does.

Have I written this keyPress() test correctly?

I confirmed that it was using Zumba\Mink\Driver\KeyboardTrait's implementation of keyPress(). I'm aware that key events aren't supported by Goutte, so I wanted to rule that out.

andrewmacpherson’s picture

Patch with the full test class described in comment #32

andrewmacpherson’s picture

I found some reports about keyPress() not working as expected with some drivers, so perhaps this is a problem with the Zumba GastonJS driver, somewhere under the hood?

Is it possible to change drivers for specific tests?

There are some interesting ideas about using Mink's evaluateScript() method to simulate a keypress, so I'll try these:

https://stackoverflow.com/questions/17333842/can-i-send-raw-keyboard-inp...
https://stackoverflow.com/questions/13938422/behat-mink-send-a-key-press...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.