Tap event obstructs keyboard-actionable hard link

[pewgeuges]

Bug Description

When a tap event is defined, the hard link is not keyboard actionable.

While clicking a footnote referrer both expands the footnotes list and scrolls to the footnote in the list, hitting Enter only expands the list, but does not scroll to the footnote, whether the footnotes list is currently expanded or not.

Per WCAG the footnotes must be keyboard navigatable. When a referrer has got focus using the tab key, hitting Enter either expands the footnotes list if it’s collapsable, or scrolls to the footnote if the list is not collapsable. Doing both actions requires a mouse click on the referrer. An Enter keypress only does one action.

If the footnotes list is not collapsable, the tap event is not present in the footnote referrer.

Expected Behaviour

When a footnote referrer has focus, and the footnotes list is collapsable, hitting Enter should scroll to the footnote (after expanding the footnote list if it’s not already expanded), like it does when clicking the referrer.

Steps to reproduce

1. Install https://wordpress.org/plugins/footnotes/
2. Check enable AMP compatibility mode.
3. Add a ((footnote)) in a post.
4. Use Tab to navigate to the referrer.
5. Press Enter.
6. Scroll down to the References: It’s now expanded.
7. Hit Enter on the referrer again.
8. Click on the referrer: Now it scrolls down.
9. Set General settings > Reference container > Collapse by default: to No.
10. Redo steps 4–5: Now it scrolls down.

Screenshots

N/A

Additional context

• WordPress version: 5.7
• Plugin version: Version 2.1.0-beta1-20210320T024922Z-4bf1b79
• Footnotes plugin version: 2.6.5
• Gutenberg plugin version (if applicable): N/A
• AMP plugin template mode: Standard
• PHP version: 7.3
• OS: Ubuntu
• Browser: Chrome, Firefox
• Device: Laptop

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

[westonruter]

This is going to rather be an upstream issue in AMP core. I'm going to transfer it.

[westonruter]

Please provide the AMP markup that uses the tap action. I suggest putting a minimal test case example on https://playground.amp.dev/ and then click the Share button.

[pewgeuges]

@westonruter

This is going to rather be an upstream issue in AMP core. I'm going to transfer it.

 Thank you for looking into this and transferring it. I mainly wished to test your new beta and respond to your call acknowledging your and @milindmore22’s help in getting a footnotes WP plugin to work in AMP. It only has now an AMP compatibility mode, and is not fully AMP compatible as many users like to use it in jQuery mode. So I don’t know if it’s a good fit for submission. AMP compatibility was released under extrinsic deadline pressure due to a WPForms compatibility requirement of a user’s client under deadline while internally a bad new release policy devised by a new graduate I’d recruited on the Forum prohibited the release of bugfix versions and only allowed v2.6.0 with AMP compatibility, that testing was not completed of. In particular, the bug reported here had been spotted and needed investigation and assessment wrt the plugin’s fitness for submission to AMP-WP. After being recently assigned this task, I’m not in a position any longer to take this step.

Please provide the AMP markup that uses the tap action. I suggest putting a minimal test case example on https://playground.amp.dev/ and then click the Share button.

It only works in a full page as it relies on many style rules. Extracting the minimal markup would take way too much time. Please just generate a page following the Steps above. Sorry for not being more helpful in this area. Also I’ve stepped down from the Footnotes project on April 2, 2021 as I’ve been deprived of commit access after I refused to keep spending time on internal process garbage, and stopped enacting the detrimental new release policy. I was only to fix bugs and add some realizable new settings and features. My mission is completed so far.

Feel free to get in touch with the new team, or alternatively wait and see it fulfill AMP compatibility. Beside the above, it now works like a charm thanks to using the on attribute, the tap event and the toggleClass method that works around the limitation about changing the CSS display property value from none to inline. Also the :focus-within you recommended in https://github.com/ampproject/amp-wp/issues/5913#issuecomment-785419655 works out fabulously for accessibility.

Thank you.

[westonruter]

@milindmore22 Would you be able to create a minimal test case to repro the issue?

[pewgeuges]

@westonruter @milindmore22 Sorry please let me know if this is too much: Playground example

[milindmore22]

I am able to reproduce the behavior at my end. https://plugins.dev6.rt.gw/2021/04/footnotes-test/?amp

[westonruter]

Thanks both.

As can be seen below, hitting the Enter key on the link seems to preventDefault() on the event whereas clicking the link does not:

Mouse Click 👍

https://user-images.githubusercontent.com/134745/113760938-b5597b80-96cb-11eb-8d2f-5316a4eb77fe.mov

Enter Keypress 👎

Here I tab to the link and keep pressing Enter but nothing is happening.

https://user-images.githubusercontent.com/134745/113760916-b094c780-96cb-11eb-8873-883ea295c356.mov

[westonruter]

As a workaround for the time being, you may consider using the scrollTo action along with the toggleClass action you're currently using.

[pewgeuges]

@milindmore22 @westonruter

Thanks for this test page and for the videos. Turns out the default action is prevented when using one of these roles:

button, checkbox, link, listbox, menuitem, menuitemcheckbox, menuitemradio, option, radio, radiogroup, scrollbar, slider, spinbutton, switch, tab, treeitem.

Role and tabindex came in from the base template accounting for the optional spans instead of anchor elements, according to an option available in jQuery mode.

If scrollTo() can be used instead of hard links, this option would be available in AMP mode too. (The syntax of scrollTo() is not clear in the documentation: it only has duration and position, the fragment ID is missing but just see it on SO that links to the place on AMP with full syntax.)

But since neither role="button" nor role="link" can be used with the AMP tap event, the anchor element must be used for accessibility.

To fix the issue for the Footnotes plugin, the role attribute with one of the problematic arguments can then be safely removed (and the tabindex altogether).

On AMP-HTML level the issue affects 23% of the existing roles, the TAPPABLE_ARIA_ROLES defined in src/service/action-impl.js:69..93.

The rationale to call event.preventDefault() is given as follows in src/service/action-impl.js:325:

“Only if the element has an action do we prevent the default. In the absence of an action, e.g. on="[event].method", we do not want to stop default behavior.”

Then I don’t understand why lines 325..330 aren’t duplicated after 308 for the click event.

But given how it works out, and this file is work in progress, best will be to delete the six lines src/service/action-impl.js:325..330.

[pewgeuges]

@westonruter I’ve updated the example.

@milindmore22 Thank you! I’ll respond later to keep posting minimal over there.

[pewgeuges]

@milindmore22 Thanks for the upvote!

@westonruter I will add that I did try tweaks attempting to debug it, suspecting that I’d screwed things up, but failed and thought I’d found an issue for the 2.1beta test call. Only when checking out @milindmore22’s footnotes-test page I tabbed through until the Continue reading link in the truncated tootip, and tried hitting Enter. I was baffled when it worked, and looked for the difference. The tooltip Read-on button is generated in PHP, and there I forgot the ARIA role. Then just to see I tested other roles, and noticed that some are working. That prompted me to test them all and determine the subset having an issue here. Based on this I searched the codebase for the very peculiar menuitemcheckbox which took me down to src/service/action-impl.js.

[pewgeuges]

@milindmore22 @westonruter I could free up some time to make a PR based on this, would that fit the workflow and meet expectations? As a more elaborate fix both mouse and keyboard access modes would make a step towards each other by uniting on double-tap / double keypress to enable defaults, and single tap / keypress to prevent them, out of an abundancy of security precautions protecting users against triggering unwanted actions. I think the double-click / double-tap / double keypress requirement to trigger default behavior vs single for triggering AMP actions exclusively might be a good compromise yet needs adding code, not removing. I’m available to request the latter for convenience as here there are no problems.

[westonruter]

@pewgeuges Neither Milind nor I are on the AMP core team which owns this component so we would not be best suited to review.

@samouri I understand you are the best to review? Thoughts on this and proposed fixes in #33892 or #33893?

[pewgeuges]

@westonruter @samouri Thanks for looking into these. Reviewing the mutually exclusive https://github.com/ampproject/amphtml/pull/33892 and https://github.com/ampproject/amphtml/pull/33893 and making a decision either way is all about policy making and might require feedback on a broad basis, so I think @milindmore22 and @westonruter are in a good position to know about end-user expectations as of whether event.preventDefault() should or should not be called — consistently across abilities — and whether handling double events as double-tap (and double-keydown) to first prevent, then not prevent default actions is feasible and advisable in this matter.

[samouri]

@samouri I understand you are the best to review? Thoughts on this and proposed fixes in #33892 or #33893?

I'm not familiar with this flow, and why we usually call event.preventDefault. Will need some extra time to ramp up here

[pewgeuges]

@samouri, @westonruter

I understand you are the best to review? Thoughts on this and proposed fixes in #33892 or #33893?

I'm not familiar with this flow, and why we usually call event.preventDefault. Will need some extra time to ramp up here

@dvoytenko You might tell more about the ins and outs since you are the author or assignee of a nearby todo item (src/service/action-impl.js:304).

@erwinmombay You are assigned twin https://github.com/ampproject/amphtml/pull/33892 and may wish to team up since at least one out of this and https://github.com/ampproject/amphtml/pull/33893 will need to be ditched anyway.