Plugins: Prevent links with button-disabled from being clickable.
#6207
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
I was thinking more of adding the wordpress-develop/src/wp-admin/includes/plugin-install.php Lines 1007 to 1011 in 186eeaf But turns out there it is an actual So I suppose this makes sense 👍 Looks like the Requesting review from accessibility folks just in case. |
| */ | ||
| $document.on( 'click', '.button-disabled', function( event ) { | ||
| event.preventDefault(); | ||
| } ); |
There was a problem hiding this comment.
While this approach solves the 'clickability' of the link, there's nothing to semantically communicate the link doesn't do anything.
Links aren't expected to be 'disabled' or do northing. It shouldn't be a link in the first place.
There's a few things in the shiny updates that would deserve improvements, especially how this link / button behaves and its labeling. I do realize we need a quick, temporary, solution for now but then I'd recommend to address semantics and provide information for users:
When the button gets the button-disabled CSS class:
- remove the
hrefentirely - add
role="button'even if it's weird that a link changes to a button dynamically, as said this is a temporary fix - add
aria-disabled="true"
Since the link is still focusable and must be focusable otherwise there would be a focus loss for keyboard users, it would be nice to actually show a focus indication as we do in gutenberg for buttons with aria-disabled=true.
There was a problem hiding this comment.
@afercia Hmm, removing the href entirely means it isn't focusable, or at least I can't tab to it.
If I use an empty href, that works, but without the event.preventDefault(), it's still clickable and refreshes the page.
What about this?
- Change the
hrefto''. - add
role="button'even if it's weird that a link changes to a button dynamically, as said this is a temporary fix - add
aria-disabled="true" - keep
event.preventDefault()
There was a problem hiding this comment.
link changes to a button dynamically
Could the Install Now and Activate Now buttons have the aria-button-if-js class when the page loads? Or is there still a scenario in which one of them opens a separate page (other than purposely opening in a new tab)?
In 6.4, Update Now already had the aria-button-if-js class to switch the role to button. Clicking that (or pressing Enter) after updating a plugin does nothing, and the button does not have the aria-disabled or focus indication either.
There was a problem hiding this comment.
@afercia Hmm, removing the
hrefentirely means it isn't focusable, or at least I can't tab to it.
🤦🏽♂️ I forgot that would happen.
To recap:
Previously on WordPress 6.4:
When clicking 'Activate' after installing a new plugin on the plugin-install.php page, users were redirected to the plugin.php page. As such, there was no risk to have an 'Activate' link that wqs still clckable.
On trunk:
The increased AJAX-ified experience added by the Plugin Dependencies feature (as referred to in the Trac ticket) introduces a new problem that I'd call a regression. After Installing a new plucin on the plugin-install.php page there's no redirect to the plugin.php page any longer. As such, the Activate link is still on the page and it's still clickable.
Ideally, there should have been a larger refactor of all these links.
Historically, these are link to support the 'no-js' scennario. I wonder whether that makes still sense: should we still support a 'no-js' scenario?
Controls that behave like buttons should be buttons.
When JS is on, all these links now behave like buttons. They should have been refactored to buttons.
That said, at this point we can only implement a temporary fix, which is less than ideal.
If I use an empty href, that works, but without the event.preventDefault(), it's still clickable
Well I can think of two options, both hacky:
- Remove the href entirely and add
tabindex="0" role="button" aria-disabled="true" - Keep the href add
role="button" aria-disabled="true"and useevent.preventDefault()
There was a problem hiding this comment.
@afercia It seems like "Remove the href entirely and add tabindex="0" role="button" aria-disabled="true" may be the least undesirable as a temporary fix as it doesn't rely on JavaScript.
I totally agree that a wider overall of the links vs buttons is needed in 6.6 and beyond.
On the "regression", I would agree that it's a regression from an accessibility perspective due to the markup of the link/button. Once that's improved, I think the removal of the automatic redirect is an enhancement that will benefit all users to be able to install and activate multiple plugins without leaving their current context. As plugins can be installed numerous ways, onboarding experiences shouldn't rely on just this redirect to trigger the onboarding. FWIW: I'd actually be quite interested in exploring a "Configure" / "Set up" action that, if provided, would appear after the plugin is activated. This would remove the need for "takeover" onboarding experiences on page reload/navigation, and instead provide a direct action in the plugin row or plugin card for the user to select intentionally.
There was a problem hiding this comment.
It seems like this just needs a decision. I think we're all agreed that none of the available options in the short term are ideal. I think we should just move forward with one of these options and start addressing the wider issue in 6.7.
I lean towards the second option, because the entire behavior of this link without an href is already dependent on JS - the argument that removing it doesn't rely on JS doesn't really make sense to me.
Well, I'd argue the Activate link that stays clickable is a regression so it's not just accessibility, it's general usability. |
|
The still clickable activate link is absolutely a bug. The accessibility regression I was referring to was the use of an |
I'm not sure I clarified why many admin pages use an Some new contributors may not know that, historically, WordPress supported the scenario where users may have JavaScript support disabled in their browsers. That's not a so rare scenario. Some companies or government bodies or other organizations may still disable JavaScript support as a security policy for their employees browseers. For yeaers, supporting 'JS off' has been a concern for any development in WordPress. Many of these links in WordPress still work when JS is off. They just use a standard GET request e.g. That's the way 'JS off' has been historically supported. I'm not saying that was a perfect approach as it creates a clear problem with links that behave like buttons but still semantically are links. This has been discussed many times bu tthere's no real way to fully addess this issue. Personally I'm not opposed to stop supporting the 'JS off' scenario. That would allow to change these links and just use buttons. However, I think that is a decision that would need a broader discussion and can't be made in this PR. |
|
Originally pasted this as a comment in the PR files thread, but it's more appropriate here. I have a partially written ticket for raising the issue of changing link-buttons into real buttons. Just haven't had the time to finish it. I've just dropped it into a Google doc, if anybody wants to make comments. I wanted to make sure that the arguments were cogent and clear before actually opening the ticket. Regarding next steps on this PR, I think we just need to choose one of the two temporary paths presented by @afercia and move forward with that for now. |
|
This was closed in https://core.trac.wordpress.org/changeset/58254 |
5 participants
Previously, links with a
hrefattribute and the.button-disabledclass were still clickable.This prevents the default handling for elements with the
.button-disabledclass.Remember to run
npm run build:devafter applying the patch. 🙂Trac ticket: https://core.trac.wordpress.org/ticket/60663