AX: Media Controls: Missing labels for the Time Labels.

[Aaron Chu]

rdar://problem/32005192

[Radar WebKit Bug Importer]

<rdar://problem/32009214>

[Aaron Chu]

Created attachment 309317 [details]
Patch

[chris fleizach]

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

[Aaron Chu]

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.

[Aaron Chu]

Created attachment 309450 [details]
Patch

[James Craig]

<rdar://problem/32005192>

[James Craig]

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.

[Aaron Chu]

<rdar://problem/32537359 >

[Aaron Chu]

Created attachment 312166 [details]
Patch

[Antoine Quint]

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.

[Aaron Chu]

Created attachment 312260 [details]
Patch

[Antoine Quint]

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.

[Aaron Chu]

Created attachment 312319 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312319 [details]
Patch

Clearing flags on attachment: 312319

Committed r217965: <http://trac.webkit.org/changeset/217965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.