Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts #2975

jongund · 2024-03-27T19:39:46Z

See issue in skipto.js repository:
[https://github.com/skipto-landmarks-headings/page-script-5/issues/11]

SkipTo Issue 13: Disable shortcuts when in a input or textarea

WAI Preview Link (Last built on Tue, 23 Apr 2024 18:47:22 GMT).

jongund · 2024-04-03T17:38:05Z

I have updated the pull request with SkipTo.js version 5.3.1 which disables the shortcut key when focus is on text input element.

For code changes see the handleDocumentKeydown event handler on lines 626-673:
https://github.com/skipto-landmarks-headings/page-script-5/blob/main/src/skiptoMenuButton.js

Test page:
https://skipto-landmarks-headings.github.io/page-script-5/test-input-controls.html

jongund · 2024-04-03T17:40:01Z

@howard-e
I have provided some information in the previous message for viewing and testing the code changes for disabling the shortcut when focus is on a text input element.

howard-e

@jongund thanks for the continued improvements to the script, as well as the detailed information on what's changed and how to test! I was able to verify the discussed change works as expected across both Windows and MacOS for several browsers.

I've left non-blocking feedback inline and a question that could be discussed separately.

howard-e · 2024-04-04T15:13:20Z

content/shared/js/skipto.js

+            !event.metaKey;
+
+          if ((optionPressed && this.config.optionShortcut === event.key) ||
+              (altPressed    && this.config.altShortcut    === event.key) ||


Suggested change

(altPressed && this.config.altShortcut === event.key) ||

(altPressed && this.config.altShortcut === event.key) ||

Minor nit that may not matter if this was a stylistic choice coming from the formatter in the source repository.

howard-e · 2024-04-04T17:04:13Z

content/shared/js/skipto.js

@@ -9,6 +9,7 @@
 * Neither the name of PayPal or any of its subsidiaries or affiliates, nor the name of the University of Illinois, nor the names of any other contributors contributors may be used to endorse or promote products derived from this software without specific prior written permission.
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 *
+ * CDN: https://skipto-landmarks-headings.github.io/page-script-5/dist/skipto.min.js
 * Documenation: https://skipto-landmarks-headings.github.io/page-script-5


Suggested change

* Documenation: https://skipto-landmarks-headings.github.io/page-script-5

* Documentation: https://skipto-landmarks-headings.github.io/page-script-5

howard-e · 2024-04-04T17:06:29Z

content/shared/js/skipto.js

@@ -9,6 +9,7 @@
 * Neither the name of PayPal or any of its subsidiaries or affiliates, nor the name of the University of Illinois, nor the names of any other contributors contributors may be used to endorse or promote products derived from this software without specific prior written permission.
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 *
+ * CDN: https://skipto-landmarks-headings.github.io/page-script-5/dist/skipto.min.js


Moving to the minified version of this script in the future could be great to reduce load.

Although subsequent reviews for updates in this repository would now only be functional and maybe require reviewing changes after the fact in the source repository, when necessary. I wonder how others may feel about that for this script?

(this feedback is not suggesting any further changes to this PR)

mcking65 · 2024-04-07T14:41:27Z

@jongund

When I open the menu, either by tabbing to it or with the shortcut key, most of the time the focus is on the third item in the menu instead of the first. This is apparently not a new bug; it happens in prod too. This is a pretty significant regression. I think it should be fixed as part of this update.

mcking65 · 2024-04-07T19:22:39Z

@jongund

I have a suspicion that the behavior I described in my previous comment has something to do with the position of the mouse before I press the shortcut key. BTW, I observed this in Chrome. This is because the menu was opening and focusing on the 3rd item consistently. Then, to work around a GitHub issue, I moved my mouse. Then when I came back to APG, the menu was opening with the first item focused.

I am wondering if placing the mouse in the position on the screen where the 3rd menu item will appear and then opening the menu triggers focus to be placed on the third item instead of the first?

…nu opens

jongund · 2024-04-19T14:33:09Z

@howard-e
Thank you for the code review I fixed the spacing and spelling issue for documentation.

jongund · 2024-04-19T14:35:17Z

@mcking65
I fixed the issue with focus moving to the menu item the mouse is hovering on in chrome by removing a pointerover event handler. I check our other menu examples and they are not using this event, so our existing examples should be good.

a11ydoer

I tried to use the shortcut key option+0 for both the English keyboard and the non-English keyboard. The shortcut key worked as expected.

updated skipto to fix shortcut key bug for non-english keyboard layouts

f83f4dd

jongund requested a review from mcking65 March 27, 2024 19:40

daniel-montalvo mentioned this pull request Mar 27, 2024

apg/update/skipto generated by aria-practices w3c/wai-aria-practices#308

Closed

mcking65 changed the title ~~Updated skipto.js to fix shortcut key bug for non-english keyboard layouts~~ Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts Apr 2, 2024

disables shortcut key for skipto when focus is on a text input

b5f2b0c

jongund requested a review from howard-e April 3, 2024 17:38

howard-e approved these changes Apr 4, 2024

View reviewed changes

mcking65 added this to the 2024-05-14 APG Publication Pull Requests milestone Apr 15, 2024

updated skipto.js to fix focus on hover issue with chrome when the me…

cb05d80

…nu opens

updated hover styling

f63141c

a11ydoer self-requested a review April 23, 2024 18:10

remove hover styling when pointer leaves menu

c8f061b

a11ydoer approved these changes May 21, 2024

View reviewed changes

mcking65 merged commit 504391a into main May 22, 2024
7 checks passed

mcking65 deleted the update/skipto branch May 22, 2024 18:09

howard-e mentioned this pull request May 23, 2024

For publication / deploy on Tuesday May 28, 2024 w3c/wai-aria-practices#327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts #2975

Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts #2975

jongund commented Mar 27, 2024 •

edited by daniel-montalvo

Loading

jongund commented Apr 3, 2024 •

edited

Loading

jongund commented Apr 3, 2024

howard-e left a comment

howard-e Apr 4, 2024

howard-e Apr 4, 2024

howard-e Apr 4, 2024

mcking65 commented Apr 7, 2024

mcking65 commented Apr 7, 2024

jongund commented Apr 19, 2024

jongund commented Apr 19, 2024

a11ydoer left a comment

	(altPressed && this.config.altShortcut === event.key) \|\|
	(altPressed && this.config.altShortcut === event.key) \|\|

	* Documenation: https://skipto-landmarks-headings.github.io/page-script-5
	* Documentation: https://skipto-landmarks-headings.github.io/page-script-5

Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts #2975

Infrastructure: Update skipto.js to fix shortcut key bug for non-english keyboard layouts #2975

Conversation

jongund commented Mar 27, 2024 • edited by daniel-montalvo Loading

jongund commented Apr 3, 2024 • edited Loading

jongund commented Apr 3, 2024

howard-e left a comment

Choose a reason for hiding this comment

howard-e Apr 4, 2024

Choose a reason for hiding this comment

howard-e Apr 4, 2024

Choose a reason for hiding this comment

howard-e Apr 4, 2024

Choose a reason for hiding this comment

mcking65 commented Apr 7, 2024

mcking65 commented Apr 7, 2024

jongund commented Apr 19, 2024

jongund commented Apr 19, 2024

a11ydoer left a comment

Choose a reason for hiding this comment

jongund commented Mar 27, 2024 •

edited by daniel-montalvo

Loading

jongund commented Apr 3, 2024 •

edited

Loading