Settings screen icon button labels should be set via aria-label instead of 0 font sized text.

RESOLVED FIXED in Firefox OS v2.2

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: obara.justin, Assigned: obara.justin)

Tracking

({access})

unspecified
2.2 S6 (20feb)
access

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The icon button labels shouldn't be specified as text nodes in the spans. Rather they should be specified using an aria-label attribute.

<div role="toolbar">
  <button class="settings toolbar-item">
    <span class="icon"
          data-l10n-id="advanced-settings-short">Settings</span>
  </button>
  <button class="sync update toolbar-item">
    <span class="icon"
          data-l10n-id="drawer-sync-button">Sync</span>
  </button>
</div>
(Assignee)

Comment 1

4 years ago
Created attachment 8552767 [details] [review]
Rather than having the text as invisible text nodes. The labels for the sync and settings buttons in the calendar settings are now set using an aria-label.
Attachment #8552767 - Flags: a11y-review?(yzenevich)
Looks good, some comments in Github. Flip back the review on once you make the changes. Thanks!

Updated

4 years ago
Attachment #8552767 - Flags: a11y-review?(yzenevich)
(Assignee)

Updated

4 years ago
Attachment #8552767 - Flags: a11y-review?(yzenevich)
Comment on attachment 8552767 [details] [review]
Rather than having the text as invisible text nodes. The labels for the sync and settings buttons in the calendar settings are now set using an aria-label.

Looks good once the last comments are addressed. Feel free to file a followup for back button stuff. Once you updated the PR, please ask mmedeiros@mozilla.com for a review. Thanks!
Attachment #8552767 - Flags: a11y-review?(yzenevich) → a11y-review+

Updated

4 years ago
Attachment #8552767 - Flags: review?(mmedeiros)
Comment on attachment 8552767 [details] [review]
Rather than having the text as invisible text nodes. The labels for the sync and settings buttons in the calendar settings are now set using an aria-label.

LGTM! thanks.
Attachment #8552767 - Flags: review?(mmedeiros) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
https://github.com/mozilla-b2g/gaia/commit/e4956b2ecd5357ba4aa7604ca2ab244b01366f7a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8552767 [details] [review]
Rather than having the text as invisible text nodes. The labels for the sync and settings buttons in the calendar settings are now set using an aria-label.

[Approval Request Comment] Fixes the way screen reader labels are set in settings.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: If declined, the labels will remain being set incorrectly.
[Testing completed]: on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: https://github.com/mozilla-b2g/gaia/pull/27580/files#diff-97b7fed57927556b91fb72d4d1b8c8b5
Attachment #8552767 - Flags: approval-gaia-v2.2?

Updated

4 years ago
Attachment #8552767 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/afc63e5c1a40b3587846cadb348996fc01998883
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S6 (20feb)
You need to log in before you can comment on or make changes to this bug.