Summary: | AX: Media Controls: Missing labels for the Time Labels. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Chu <aaron_chu> | ||||||||||||
Component: | Accessibility | Assignee: | Aaron Chu <aaron_chu> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aaron_chu, cfleizach, commit-queue, graouts, jcraig, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari 9 | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Aaron Chu
2017-05-04 23:22:58 PDT
Created attachment 309317 [details]
Patch
Comment on attachment 309317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309317&action=review > Source/WebCore/ChangeLog:9 > + Added descriptive labels to elapsed nad remaining time labels in modern media controls. nad -> and > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:37 > + this.element.setAttribute("aria-roledescription", timerDescription); is role-description appropriate here? I would have thought remaining/elapsed would be more appropriate as the labels Comment on attachment 309317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309317&action=review >> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:37 >> + this.element.setAttribute("aria-roledescription", timerDescription); > > is role-description appropriate here? I would have thought remaining/elapsed would be more appropriate as the labels I think so, because the label would be the time itself, remaining/elapsed serve to describe the timer. Created attachment 309450 [details]
Patch
Comment on attachment 309450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309450&action=review > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:37 > + this.element.setAttribute("aria-roledescription", timerDescription); Let's use aria-label rather than roledescription here. This seems like a solid attempt at reducing verbosity, but I believe it'd have some fallout. In a context where role description was unsupported (maybe Orca/Atk? or iTunes on Windows?), there would no way to distinguish the two timers. > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:65 > + this.element.setAttribute("aria-label", formattedTime); I think this label would duplicate the contents, so you'd hear something like "0:23, 0:23" in some browser+screenreader combinations (timers should speak both their label and value) I believe the previous version used: <span role="timer" aria-label="Remaining">0:23</span> > LayoutTests/media/modern-media-controls/time-control/time-control.html:36 > +shouldBeEqualToString("timeControl.elapsedTimeLabel.element.getAttribute('aria-roledescription')", "Elapsed"); > +shouldBeEqualToString("timeControl.elapsedTimeLabel.element.getAttribute('role')", "timer"); > shouldBe("timeControl.children[1]", "timeControl.scrubber"); > shouldBe("timeControl.children[2]", "timeControl.remainingTimeLabel"); > +shouldBeEqualToString("timeControl.remainingTimeLabel.element.getAttribute('aria-roledescription')", "Remaining"); > +shouldBeEqualToString("timeControl.remainingTimeLabel.element.getAttribute('role')", "timer"); See if you can validate the accessible note properties here rather than the DOM attributes. Created attachment 312166 [details]
Patch
Comment on attachment 312166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312166&action=review > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:74 > + this.element.setAttribute("aria-label", `${ariaLabel}: ${timeAsString}`); You can use `const` for these two since they don't change. > LayoutTests/media/modern-media-controls/time-label/time-label.html:18 > +timeLabel.element.setAttribute("id", "elasped"); This is fine but you can shorten this to `timeLabel.element.id = "elapsed"`. > LayoutTests/media/modern-media-controls/time-label/time-label.html:27 > +remainingTimeLabel.element.setAttribute("id", "remaining"); Same here about using the "id" property directly. > LayoutTests/media/modern-media-controls/time-label/time-label.html:52 > + finishMediaControlsTest(); Please remove that extra white space. Created attachment 312260 [details]
Patch
Comment on attachment 312260 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312260&action=review > LayoutTests/media/modern-media-controls/time-label/time-label.html:51 > + Remove this extra white space when committing. Created attachment 312319 [details]
Patch
Comment on attachment 312319 [details] Patch Clearing flags on attachment: 312319 Committed r217965: <http://trac.webkit.org/changeset/217965> All reviewed patches have been landed. Closing bug. |