<rdar://problem/30153323>
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.
https://bugs.webkit.org/show_bug.cgi?id=170047
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.