Summary: | AX: Media controls are unlabeled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Chu <aaron_chu> | ||||||||
Component: | Accessibility | Assignee: | Aaron Chu <aaron_chu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, graouts, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari 10 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Aaron Chu
2017-03-21 23:54:57 PDT
Created attachment 305090 [details]
Patch
Comment on attachment 305090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305090&action=review Looks great overall. Minor comments included. Please get the r+ from Antoine since he wrote most of this. > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:3 > + "AirPlay Placard": "AirPlay Placard", The term "placard" may be redundant in end-user UI. Should this be just "AirPlay"? > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6 > + "Enter Fullscreen": "Enter Fullscreen", > + "Enter Picture-in-Picture": "Enter Picture-in-picture", Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix. > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:9 > + "Invalid Placard": "Invalid Placard", remove "placard" if possible > Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:111 > + this.element.setAttribute("aria-label", this.label); Is there ever a time when this could set an empty label? If so, this could be: if (this.label) this.element.setAttribute("aria-label", this.label); > Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:112 > + debugger; leftover > LayoutTests/media/modern-media-controls/mute-button/mute-button-expected.txt:10 > +PASS muteButton.label is "Volume" Seems like this should be "Mute" > LayoutTests/media/modern-media-controls/mute-button/mute-button-expected.txt:14 > +PASS mutedButton.label is "Volume Mute" And this "Unmute" Comment on attachment 305090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305090&action=review Overall comment: I think we shouldn't introduce a "label" property for IconButton and instead use the single "iconName" property to set the "aria-label" attribute since the icon name and labels are coupled right now. This should also make for a smaller source change. > Source/WebCore/ChangeLog:11 > + modern media constrols. Typo: "controls". > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:5 > + "Enter Fullscreen": "Enter Fullscreen", I think it should be "Full Screen". > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6 > + "Enter Picture-in-Picture": "Enter Picture-in-picture", No hyphenation, should be "Enter Picture in Picture". > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:15 > + "Picture-in-Picture Placard": "Picture-in-Picture Placard", No hyphenation. > Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:74 > + this._label = label; I think we should the attribute here directly, no need to do this in `layout()` since this is not connected to rendering… although, since the icon name and labels seem coupled, could we do without a distinct `label` altogether and just get the label for the icon name? > Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:111 > + this.element.setAttribute("aria-label", this.label); Per the previous comment, this belongs in the `label` setter I reckon. > Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:112 > + debugger; Definitely remove that :) > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:46 > + VolumeUp : {name: "volume-up", label: UIString("Volume Up")} Per WebKit style (I think!) there should be a space after the opening brace and before the closing brace: { name: "volume-up", label: UIString("Volume Up") } > Source/WebCore/Modules/modern-media-controls/js-files:2 > +main.js Why was this needed? > LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:14 > +PASS airplayButton._image.complete is false This shouldn't change, this test is currently broken and expected to be per TestExpectations. > LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:-29 > -FAIL airplayButton._image.complete should be false. Was true. This shouldn't change, this test is currently broken and expected to be per TestExpectations. > LayoutTests/media/modern-media-controls/airplay-button/airplay-button-expected.txt:49 > +PASS airplayButton.element.getAttribute('aria-label') is "AirPlay" That's correct though. Comment on attachment 305090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305090&action=review >>> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6 >>> + "Enter Picture-in-Picture": "Enter Picture-in-picture", >> >> Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix. > > No hyphenation, should be "Enter Picture in Picture". I think "Enter" is needed at least for full screen because there is a an Exit burrow in the controls when it is in full screen mode. >> Source/WebCore/Modules/modern-media-controls/js-files:2 >> +main.js > > Why was this needed? the main.js provided the UIString method, I moved it up to the top because now that we are using UIString to pull the loc strings, the main.js becomes necessary before all other files, the "modern-media-controls-localized-strings.js" was meant to be a symlink, but it seems like it is pulled in as a new file, but I took a second pass, the loc file is not needed. I will remove in next patch. Comment on attachment 305090 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305090&action=review >>>> Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:6 >>>> + "Enter Picture-in-Picture": "Enter Picture-in-picture", >>> >>> Reduce the verbosity of the labels where possible. These should just be "Full screen" and "Picture-in-picture." No need for the "Enter" prefix. >> >> No hyphenation, should be "Enter Picture in Picture". > > I think "Enter" is needed at least for full screen because there is a an Exit burrow in the controls when it is in full screen mode. Exit button* Created attachment 305223 [details]
Patch
Comment on attachment 305223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305223&action=review > Source/WebCore/English.lproj/modern-media-controls-localized-strings.js:28 > + "Volume Mute": "Volume Mute", volume mute sounds strange to me. should this just be "Mute" Created attachment 305239 [details]
Patch
Comment on attachment 305239 [details]
Patch
Looks good! While this could land as-is, I think we might want to refactor imageForIconNameAndLayoutTraits() into imageForIconAndLayoutTraits() and pass the icon and directly and let that function look up its name instead of explicitly passing it around.
Comment on attachment 305239 [details] Patch Clearing flags on attachment: 305239 Committed r214400: <http://trac.webkit.org/changeset/214400> All reviewed patches have been landed. Closing bug. |