Combobox with grid popup: Make escape key behavior consistent with pattern #1334

mcking65 · 2020-02-16T19:31:07Z

Fix issue #1066 for the grid combobox.

Update keyboard documentation to reflect expected behavior
Change JavaScript so Escape functions as documented
Revise tests to test documented behavior

Preview Link

View the combobox with grid popup in compare branch of this PR

Review checklist

carmacleod

+1 for Editorial review.

jongund · 2020-02-19T21:45:52Z

@mcking65

I have updated the escape key behavior and escape key regression tests to match the pattern.

mcking65

When visual focus is in the grid, pressing escape does not hide the grid and change the combobox state to collapsed. It only moves visual focus back to the input. It is supposed to do both. Example:

Focus combobox
Type 'A'
Press down arrow to move visual focus into the grid.
Press escape.

Expected:

Visual focus moves back to input.
Grid is closed.

Actual: focus moves back to input.

This indicates the tests are also not testing correctly, otherwise the test for escape in the grid would fail.

Update the corresponding test to no longer expect a failure. Fixes #860.

zcorpan · 2020-03-06T14:11:07Z

I've fixed the logic so that escape always hides the popup if the popup is visible, regardless of focus.

We were testing this case correctly. It's just that the test was marked as expected to fail, since this was a known issue: #860

I've changed the test so it instead expects to pass, and it does.

zcorpan · 2020-03-06T14:12:34Z

@spectranaut can you review my changes?

zcorpan · 2020-03-06T14:28:46Z

Looks like we're missing some tests. But that was the case before this PR, too, so I've filed an issue to track this: #1349

zcorpan · 2020-03-06T14:13:29Z

examples/combobox/js/grid-combo.js

@@ -94,12 +94,16 @@ aria.GridCombobox.prototype.handleInputKeyDown = function (evt) {
      );
    }
    else {
+      if (!this.shown) {
+        setTimeout((function () {
+          // On Firefox, input does not get cleared here unless wrapped in


Do we know why this happens? Is it a bug in Firefox?

I think this is an issue of using keydown, instead of the keyup event. I was only asked to fix the ESC behavior, so I didn't think it would be good to make more changes than requested.

Thank you @jongund, that's good call. let's address that as a separate issue if there is a better way to resolve it than what is currently implemented.

zcorpan

code review and test review done.

mcking65

Functionally now working as documented.

mcking65 · 2020-03-09T05:06:58Z

OK, now just waiting on a review by @spectranaut of the test changes made by @zcorpan. I will merge if @spectranaut approves.

carmacleod · 2020-03-09T14:42:36Z

Should down arrow (and/or alt+down arrow) open the grid? (It currently doesn't, but I think it should).
It currently feels strange that you have to type an appropriate letter key to open the grid.
(If this comment would be better as a new issue, please let me know).

mcking65 · 2020-03-09T15:59:25Z

@carmacleod commented:

Should down arrow (and/or alt+down arrow) open the grid? (It currently doesn't, but I think it should).
It currently feels strange that you have to type an appropriate letter key to open the grid.
(If this comment would be better as a new issue, please let me know).

That is issue #1266.

carmacleod · 2020-03-09T16:21:25Z

@mcking65

That is issue #1266.

Ah, right. Sorry about that. :) Thanks!

carmacleod

+1 again, for the latest.

Combobox with grid popup: Make escape key behavior consistent with pattern (pull #1334) Fixes #860 and #1066: * Revise Escape key documentation to reflect expected behavior * updated escape key behavior to match combobox pattern * Make escape hide the popup if it's shown, regardless of where focus is * Update the corresponding test to no longer expect a failure (fixes #860). * Remove comment about failing test Co-authored-by: Jon <[email protected]> Co-authored-by: Simon Pieters <[email protected]>

…ttern (pull w3c#1334) Fixes w3c#860 and w3c#1066: * Revise Escape key documentation to reflect expected behavior * updated escape key behavior to match combobox pattern * Make escape hide the popup if it's shown, regardless of where focus is * Update the corresponding test to no longer expect a failure (fixes w3c#860). * Remove comment about failing test Co-authored-by: Jon <[email protected]> Co-authored-by: Simon Pieters <[email protected]>

Revise Escape key documentation to reflect expected behavior

4ee17f9

carmacleod approved these changes Feb 16, 2020

View reviewed changes

jongund self-requested a review February 19, 2020 20:03

updated escape key behavior to match combobox pattern

b65526d

mcking65 marked this pull request as ready for review February 25, 2020 19:33

mcking65 requested a review from zcorpan February 28, 2020 08:01

mcking65 linked an issue Feb 28, 2020 that may be closed by this pull request

Combobox: Escape key should not clear current input #1066

Closed

5 tasks

mcking65 commented Feb 28, 2020

View reviewed changes

carmacleod self-requested a review February 28, 2020 13:31

zcorpan self-assigned this Mar 6, 2020

zcorpan added 2 commits March 6, 2020 15:07

Make escape hide the popup if it's shown, regardless of where focus is

bbfe679

Update the corresponding test to no longer expect a failure. Fixes #860.

Remove comment about failing test

46be8ba

zcorpan requested a review from spectranaut March 6, 2020 14:12

zcorpan mentioned this pull request Mar 6, 2020

Combobox grid missing tests for textbox-key-down-arrow and textbox-key-up-arrow #1349

Open

zcorpan reviewed Mar 6, 2020

View reviewed changes

zcorpan approved these changes Mar 6, 2020

View reviewed changes

Merge branch 'master' into issue1066-for-grid

04f6b8c

mcking65 commented Mar 9, 2020

View reviewed changes

carmacleod approved these changes Mar 9, 2020

View reviewed changes

Merge branch 'master' into issue1066-for-grid

83410b4

mcking65 removed a link to an issue Mar 17, 2020

Combobox: Escape key should not clear current input #1066

Closed

5 tasks

mcking65 linked an issue Mar 17, 2020 that may be closed by this pull request

Combobox: Escape key should not clear current input #1066

Closed

5 tasks

mcking65 linked an issue Mar 17, 2020 that may be closed by this pull request

Bug in example combobox/aria1.1pattern/grid-combo.html: key "escape" does not close popup #860

Closed

mcking65 merged commit aabf040 into master Mar 17, 2020

mcking65 deleted the issue1066-for-grid branch March 17, 2020 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combobox with grid popup: Make escape key behavior consistent with pattern #1334

Combobox with grid popup: Make escape key behavior consistent with pattern #1334

mcking65 commented Feb 16, 2020 •

edited

Loading

carmacleod left a comment •

edited

Loading

jongund commented Feb 19, 2020

mcking65 left a comment

zcorpan commented Mar 6, 2020

zcorpan commented Mar 6, 2020

zcorpan commented Mar 6, 2020

zcorpan Mar 6, 2020

jongund Mar 9, 2020

mcking65 Mar 9, 2020

zcorpan left a comment

mcking65 left a comment

mcking65 commented Mar 9, 2020

carmacleod commented Mar 9, 2020 •

edited

Loading

mcking65 commented Mar 9, 2020

carmacleod commented Mar 9, 2020

carmacleod left a comment •

edited

Loading

Combobox with grid popup: Make escape key behavior consistent with pattern #1334

Combobox with grid popup: Make escape key behavior consistent with pattern #1334

Conversation

mcking65 commented Feb 16, 2020 • edited Loading

Preview Link

Review checklist

carmacleod left a comment • edited Loading

Choose a reason for hiding this comment

jongund commented Feb 19, 2020

mcking65 left a comment

Choose a reason for hiding this comment

zcorpan commented Mar 6, 2020

zcorpan commented Mar 6, 2020

zcorpan commented Mar 6, 2020

zcorpan Mar 6, 2020

Choose a reason for hiding this comment

jongund Mar 9, 2020

Choose a reason for hiding this comment

mcking65 Mar 9, 2020

Choose a reason for hiding this comment

zcorpan left a comment

Choose a reason for hiding this comment

mcking65 left a comment

Choose a reason for hiding this comment

mcking65 commented Mar 9, 2020

carmacleod commented Mar 9, 2020 • edited Loading

mcking65 commented Mar 9, 2020

carmacleod commented Mar 9, 2020

carmacleod left a comment • edited Loading

Choose a reason for hiding this comment

mcking65 commented Feb 16, 2020 •

edited

Loading

carmacleod left a comment •

edited

Loading

carmacleod commented Mar 9, 2020 •

edited

Loading

carmacleod left a comment •

edited

Loading