Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

firewall: accessibility fixes for NAT page #7599

Closed
wants to merge 1 commit into from

Conversation

jfayre
Copy link
Contributor

@jfayre jfayre commented Jul 4, 2024

Add aria-label to buttons and links in table.
Added role="cell" in last column of table to prevent screen readers from reading action buttons when navigating to other table rows.

Add aria-label to buttons and links in table.
Added role="cell" in last column of table to prevent screen readers from reading action buttons when navigating to other table rows.
<th class="text-nowrap">
<a href="firewall_nat_edit.php" class="btn btn-primary btn-xs" data-toggle="tooltip" title="<?= html_safe(gettext('Add')) ?>">
<th role="cell" class="text-nowrap">
<a href="firewall_nat_edit.php" class="btn btn-primary btn-xs" data-toggle="tooltip" title="<?= html_safe(gettext('Add')) ?>" aria-label="<?= html_safe(gettext('Add')) ?>"">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<a href="firewall_nat_edit.php" class="btn btn-primary btn-xs" data-toggle="tooltip" title="<?= html_safe(gettext('Add')) ?>" aria-label="<?= html_safe(gettext('Add')) ?>"">
<a href="firewall_nat_edit.php" class="btn btn-primary btn-xs" data-toggle="tooltip" title="<?= html_safe(gettext('Add')) ?>" aria-label="<?= html_safe(gettext('Add')) ?>">

@AdSchellevis
Copy link
Member

@jfayre
While reading this PR, I'm wondering if we are trying to apply the right fix here.

Looking at bootstrap.js on our end, on rendering the title is moved to data-original-title:

Tooltip.prototype.fixTitle = function () {
var $e = this.$element
if ($e.attr('title') || typeof $e.attr('data-original-title') != 'string') {
$e.attr('data-original-title', $e.attr('title') || '').attr('title', '')
}
}

Newer versions seem to add aria-label here too:

https://github.com/twbs/bootstrap/blob/7c392498fabed7b4327e7c798c6af152b5071d29/dist/js/bootstrap.js#L3554-L3564

Maybe we better backport this change on our end so all tooltips are equal and we don't need to copy titles.

What do you think?

@fichtner
Copy link
Member

fichtner commented Jul 8, 2024

I would prefer the deduplication too. The risk for regressions is also a factor here changing all these lines. Things happen.

@jfayre
Copy link
Contributor Author

jfayre commented Jul 8, 2024 via email

@AdSchellevis AdSchellevis self-assigned this Jul 8, 2024
@AdSchellevis AdSchellevis added the feature Adding new functionality label Jul 8, 2024
@AdSchellevis
Copy link
Member

I'll take a look

@AdSchellevis
Copy link
Member

b0cab3b should do the trick

@jfayre jfayre deleted the accessibility-firewall-nat branch July 13, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality
Development

Successfully merging this pull request may close these issues.

3 participants