[GTK] Improve JavaScript multimedia controls
Created attachment 224635 [details] Patch
Created attachment 224639 [details] Patch
Created attachment 224643 [details] Patch I expect James review, but Mario, could you please also confirm that the a11y rebaselines are correct?
Comment on attachment 224643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224643&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:70 > + this.controls.remainingTime.classList.add(this.ClassNames.hidden); My preference would be to use the hidden attribute here (combined with the CSS change below) instead of a hidden class. Is there a reason this needs to be a classname? this.controls.remainingTime.hidden = true; > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 > + if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) > + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); if (this.controls.currentTime.hidden) this.controls.remainingTime.hidden = false; (You probably don't need the conditional here in either case, unless you were planning to use el.classList.toggle()) > Source/WebCore/css/mediaControlsGtk.css:32 > +.hidden { > + display: none !important; > +} Selector change based on above use of hidden attr would be: *[hidden] { or just [hidden] { > Source/WebCore/css/mediaControlsGtk.css:117 > - outline: none; > + outline: black solid 0px; These are effectively identical unless you back these up with a :focus state, such as… audio::-webkit-media-controls-mute-button:focus, video::-webkit-media-controls-mute-button:focus { outline: black dotted 1px; } > Source/WebCore/css/mediaControlsGtk.css:130 > - outline: none; > + outline: black solid 0px; ditto for the rest > Source/WebCore/css/mediaControlsGtk.css:-180 > -audio::-webkit-media-controls-current-time-display, video::-webkit-media-controls-current-time-display { > - display: block; > -} > - > -audio::-webkit-media-controls-time-remaining-display, > -video::-webkit-media-controls-time-remaining-display { > - display: none; > -} > - > -audio::-webkit-media-controls-time-remaining-display.show, > -video::-webkit-media-controls-time-remaining-display.show { > - display: block; > -} > - > -audio::-webkit-media-controls-current-time-display.hidden, > -video::-webkit-media-controls-current-time-display.hidden, > -audio::-webkit-media-controls-time-remaining-display.hidden, > -video::-webkit-media-controls-time-remaining-display.hidden { > - display: none; > -} > - I love seeing one simple rule replace all this redundancy. ;-) > Source/WebCore/css/mediaControlsGtk.css:194 > audio::-webkit-media-controls-volume-slider-container.hidden, > video::-webkit-media-controls-volume-slider-container.hidden { > + display: inherit !important; > height: 0; > } I think it'd be better to have the script *not* set the hidden attr or class on this element rather than use !important as an override here.
(In reply to comment #4) > (From update of attachment 224643 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224643&action=review > > > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:70 > > + this.controls.remainingTime.classList.add(this.ClassNames.hidden); > > My preference would be to use the hidden attribute here (combined with the CSS change below) instead of a hidden class. Is there a reason this needs to be a classname? > > this.controls.remainingTime.hidden = true; Nope, I have never used that but I'll give it a try. > > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 > > + if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) > > + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); > > if (this.controls.currentTime.hidden) > this.controls.remainingTime.hidden = false; > > (You probably don't need the conditional here in either case, unless you were planning to use el.classList.toggle()) I don't follow this. > > Source/WebCore/css/mediaControlsGtk.css:32 > > +.hidden { > > + display: none !important; > > +} > > Selector change based on above use of hidden attr would be: > > *[hidden] { > or just > [hidden] { > > > Source/WebCore/css/mediaControlsGtk.css:117 > > - outline: none; > > + outline: black solid 0px; > > These are effectively identical unless you back these up with a :focus state, such as… > > audio::-webkit-media-controls-mute-button:focus, video::-webkit-media-controls-mute-button:focus { > outline: black dotted 1px; > } What I tried to do is not showing any mark of focus, which I was commanded to remove by our designers, without disturbing accessibility. I don't know if you can come up with any other solution because I guess adding the black dotted 1px for :focus will show something. Another backup idea would be avoiding painting the line from C++ by telling WebCore that the part handles the focus ring and then not painting it. > > Source/WebCore/css/mediaControlsGtk.css:194 > > audio::-webkit-media-controls-volume-slider-container.hidden, > > video::-webkit-media-controls-volume-slider-container.hidden { > > + display: inherit !important; > > height: 0; > > } > > I think it'd be better to have the script *not* set the hidden attr or class on this element rather than use !important as an override here. I guess you mean manually handling the height to 0 instead of setting it hidden as we do for the captions menu?
(In reply to comment #5) > > My preference would be to use the hidden attribute here (combined with the CSS change below) instead of a hidden class. Is there a reason this needs to be a classname? > > > > this.controls.remainingTime.hidden = true; > > Nope, I have never used that but I'll give it a try. I began to have a look but I realized that Apple controls, which we have as base, use it as class name, so changing this would mean that we couldn't use a big part of the common code. In order to change it and keep the common code, it would require that we changed Apple, iOS and GTK+ controls at the same time, which I would gladly do, but I think it would be better to do it in a different bug.
Comment on attachment 224643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224643&action=review >>> Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:70 >>> + this.controls.remainingTime.classList.add(this.ClassNames.hidden); >> >> My preference would be to use the hidden attribute here (combined with the CSS change below) instead of a hidden class. Is there a reason this needs to be a classname? >> >> this.controls.remainingTime.hidden = true; > > Nope, I have never used that but I'll give it a try. You wrote: > I began to have a look but I realized that Apple controls, which we have as base, use it as class name, so changing this would mean that we couldn't use a big part of the common code. In order to change it and keep the common code, it would require that we changed Apple, iOS and GTK+ controls at the same time, which I would gladly do, but I think it would be better to do it in a different bug. That's fair. Leave it for now. >>> Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 >>> + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); >> >> if (this.controls.currentTime.hidden) >> this.controls.remainingTime.hidden = false; >> >> (You probably don't need the conditional here in either case, unless you were planning to use el.classList.toggle()) > > I don't follow this. You're inside an else block here that means you always want to show the remaining time element. So you can just call el.classList.remove(). WebKit will do the implicit conditional here, which is checking to see if that classname exists before removing it. The only reason you would need to use a conditional is if you were using el.classList.toggle(), which removes the classname if it exists, and adds it if it doesn't, so you can change these lines: if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) this.controls.remainingTime.classList.remove(this.ClassNames.hidden); to just this: this.controls.remainingTime.classList.remove(this.ClassNames.hidden); Which, internally, is equivalent to this: if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) this.controls.remainingTime.classList.toggle(this.ClassNames.hidden); // only toggle the classname if it's already contained >>> Source/WebCore/css/mediaControlsGtk.css:32 >>> +} >> >> Selector change based on above use of hidden attr would be: >> >> *[hidden] { >> or just >> [hidden] { > > What I tried to do is not showing any mark of focus, which I was commanded to remove by our designers, without disturbing accessibility. I don't know if you can come up with any other solution because I guess adding the black dotted 1px for :focus will show something. Another backup idea would be avoiding painting the line from C++ by telling WebCore that the part handles the focus ring and then not painting it. While a visible keyboard focus does not negatively affect fully blind screen reader users, you should make your designers aware that hiding a visible focus state does disturb accessibility for sighted keyboard users. Not everyone is able to use a mouse, and therefore hiding the visible state of keyboard makes these controls inoperable to a class of users. If the designers don't like the default focus style, they should design a new focus style for you to implement, or you can piggyback on the :active (mousedown) styles like the proposed patch in bug 127260. >>> Source/WebCore/css/mediaControlsGtk.css:194 >>> } >> >> I think it'd be better to have the script *not* set the hidden attr or class on this element rather than use !important as an override here. > > I guess you mean manually handling the height to 0 instead of setting it hidden as we do for the captions menu? No, I mean here you've stated that the volume slider should inherit the display value of its parent element, regardless of whether it has a "hidden" class. It's an anti-pattern to display something that is marked as hidden, so instead of overriding the "hidden" class definition for this element to mean something other than "hidden", you should add a conditional to the script that adds this classname, and only add it if the element really should be hidden.
(In reply to comment #7) > > What I tried to do is not showing any mark of focus, which I was commanded to remove by our designers, without disturbing accessibility. I don't know if you can come up with any other solution because I guess adding the black dotted 1px for :focus will show something. Another backup idea would be avoiding painting the line from C++ by telling WebCore that the part handles the focus ring and then not painting it. > > While a visible keyboard focus does not negatively affect fully blind screen reader users, you should make your designers aware that hiding a visible focus state does disturb accessibility for sighted keyboard users. Not everyone is able to use a mouse, and therefore hiding the visible state of keyboard makes these controls inoperable to a class of users. If the designers don't like the default focus style, they should design a new focus style for you to implement, or you can piggyback on the :active (mousedown) styles like the proposed patch in bug 127260. I am adding Jon here to get some feedback from him. Jon, as you can read, James recommends to keep a style for the focus because it is needed for accessibility reasons. Any idea of which outline style can we use to keep a reasonable trade-off? Maybe you can have a look at http://www.w3schools.com/cssref/pr_outline.asp and play a bit to get a reasonable solution that you like? > >> I think it'd be better to have the script *not* set the hidden attr or class on this element rather than use !important as an override here. > > > > I guess you mean manually handling the height to 0 instead of setting it hidden as we do for the captions menu? > > No, I mean here you've stated that the volume slider should inherit the display value of its parent element, regardless of whether it has a "hidden" class. It's an anti-pattern to display something that is marked as hidden, so instead of overriding the "hidden" class definition for this element to mean something other than "hidden", you should add a conditional to the script that adds this classname, and only add it if the element really should be hidden. The problem here is that almost all elements should have display none when marked as hidden. There are some of them like the captions menu and the volume slider that have animations and changing its display to none would cut the animation. In this case, hiding the element is done through animating the height instead of switching the display, but as you asked me to make a general case for the hidden elements I could only override the display to get the animation seen. At this point I only see the following possibilities: 1) Keeping the override and admitting that antipattern. 2) Defining another class, that could be called "animated" for example and make something like: .hidden.animated { display: block !important; } 3) Going back to the different hidden rules with different displays instead of having a general ".hidden" 4) Animating the bar directly with the height without setting the hidden class.
Gentle reminder for comment 8. Any answer?
Only replying to the only question directed at me regarding the focus indicator. Please correct me if I'm wrong. I'm well aware of the use of the focus indicator. If I'm understanding the question correctly, what I would recommend is doing what we attempt to do in GTK+. That is, show the focus / keyboard navigation indicator (aka focus rectangle) when keyboard navigation is invoked. Otherwise, do not show it. In GTK+ the intention (modulo bugs) is for it to be shown when using tab navigation of controls but not before. This allows us not to have to make a poor trade of being ugly by default, or hurting a11y. In order to be clear about what we are discussing can you please attach a screenshot of the focus indicator that is proposed? Thanks.
(In reply to comment #10) > Only replying to the only question directed at me regarding the focus indicator. Please correct me if I'm wrong. > > I'm well aware of the use of the focus indicator. If I'm understanding the question correctly, what I would recommend is doing what we attempt to do in GTK+. That is, show the focus / keyboard navigation indicator (aka focus rectangle) when keyboard navigation is invoked. Otherwise, do not show it. In GTK+ the intention (modulo bugs) is for it to be shown when using tab navigation of controls but not before. This allows us not to have to make a poor trade of being ugly by default, or hurting a11y. In order to be clear about what we are discussing can you please attach a screenshot of the focus indicator that is proposed? Thanks. I could avoid painting the rectangle from c++ if that's what you think it's best. Anyway, if we had to paint something it is my question question to you what shape to choose among the different combinations that the css standard offers us. I think the default is 1 pixel dotted line.
(In reply to comment #8) > I am adding Jon here to get some feedback from him. Jon, as you can read, James recommends to keep a style for the focus because it is needed for accessibility reasons. Any idea of which outline style can we use to keep a reasonable trade-off? Maybe you can have a look at http://www.w3schools.com/cssref/pr_outline.asp and play a bit to get a reasonable solution that you like? Clarifying again that you don't need to use one of the default outline styles if you consider them ugly. a visible focus style can be anything. For example, most native menuing systems use the same style for mouseover and focus behavior, and neither has an outline. Likewise the Apple video controls use a small radial gradient (looks like a white "glow" behind the button) on the :active (mousedown) state, and the related bug's patch includes the :focus state there. Neither use "ugly" outlines. > The problem here is that almost all elements should have display none when marked as hidden. There are some of them like the captions menu and the volume slider that have animations and changing its display to none would cut the animation. Then I think you should get a callback from the webkitTransitionEnd event, and only set the hidden property once the animation is finished. You can use a separate animation class if needed.
Created attachment 225101 [details] focus/active state of Apple controls
(In reply to comment #10) > This allows us not to have to make a poor trade of being ugly by default, or hurting a11y. In order to be clear about what we are discussing can you please attach a screenshot of the focus indicator that is proposed? Added screen shot of the subtle style :active/:focus style on the pause button (Apple styled controls) that does not use the CSS outline property.
Created attachment 225167 [details] Patch
Comment on attachment 225167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225167&action=review > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:516 > - return false; > + return true; It looks like this is actually fixing a bug. Does it change the test output? > LayoutTests/ChangeLog:9 > + * platform/gtk/TestExpectations: Added video-zoom-controls as > + image only. Why is this test failing now?
(In reply to comment #16) > (From update of attachment 225167 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225167&action=review > > > Source/WebCore/platform/gtk/RenderThemeGtk.cpp:516 > > - return false; > > + return true; > > It looks like this is actually fixing a bug. Does it change the test output? No, because we weren't painting any background before. We are not changing the output. > > LayoutTests/ChangeLog:9 > > + * platform/gtk/TestExpectations: Added video-zoom-controls as > > + image only. > > Why is this test failing now? The problem because that test has an image failure is previous to this patch but I preferred to flag it.
Comment on attachment 225167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225167&action=review >>> LayoutTests/ChangeLog:9 >>> + image only. >> >> Why is this test failing now? > > The problem because that test has an image failure is previous to this patch but I preferred to flag it. I don't think it's correct to flag it in this patch then.
Created attachment 225247 [details] Patch
(In reply to comment #12) > > I am adding Jon here to get some feedback from him. Jon, as you can read, James recommends to keep a style for the focus because it is needed for accessibility reasons. Any idea of which outline style can we use to keep a reasonable trade-off? Maybe you can have a look at http://www.w3schools.com/cssref/pr_outline.asp and play a bit to get a reasonable solution that you like? > > Clarifying again that you don't need to use one of the default outline styles if you consider them ugly. a visible focus style can be anything. For example, most native menuing systems use the same style for mouseover and focus behavior, and neither has an outline. Likewise the Apple video controls use a small radial gradient (looks like a white "glow" behind the button) on the :active (mousedown) state, and the related bug's patch includes the :focus state there. Neither use "ugly" outlines. I added a very subtle gradient as mark for :active and :focus components. > > The problem here is that almost all elements should have display none when marked as hidden. There are some of them like the captions menu and the volume slider that have animations and changing its display to none would cut the animation. > > Then I think you should get a callback from the webkitTransitionEnd event, and only set the hidden property once the animation is finished. You can use a separate animation class if needed. I changed the class. (In reply to comment #18) > I don't think it's correct to flag it in this patch then. Removed.
Comment on attachment 225247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225247&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 > - this.controls.remainingTime.classList.add(this.ClassNames.show); > + if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) > + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); No need for this conditional. You can just directly call classList.remove() it like you directly called classList.add() above. this.controls.remainingTime.classList.remove(this.ClassNames.hidden); The rest of the CSS/JS parts look good to me. Thanks.
(In reply to comment #21) > (From update of attachment 225247 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225247&action=review > > > Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 > > - this.controls.remainingTime.classList.add(this.ClassNames.show); > > + if (this.controls.currentTime.classList.contains(this.ClassNames.hidden)) > > + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); > > No need for this conditional. You can just directly call classList.remove() it like you directly called classList.add() above. > > this.controls.remainingTime.classList.remove(this.ClassNames.hidden); > > The rest of the CSS/JS parts look good to me. Thanks. Maybe I am confused, but I don't know if you realized that the contains and the remove are called on different objects. What I am trying to do is not showing the remaining time if the current is currently shown.
Created attachment 225269 [details] Screenshot of the gradient Jon, I attach a screenshot so that you can see the subtle gradient around the play button.
(In reply to comment #23) > Created an attachment (id=225269) [details] > Screenshot of the gradient > > Jon, I attach a screenshot so that you can see the subtle gradient around the play button. I mean that's what I did for :focus and :active.
Comment on attachment 225247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225247&action=review >>> Source/WebCore/Modules/mediacontrols/mediaControlsGtk.js:75 >>> + this.controls.remainingTime.classList.remove(this.ClassNames.hidden); >> >> No need for this conditional. You can just directly call classList.remove() it like you directly called classList.add() above. >> >> this.controls.remainingTime.classList.remove(this.ClassNames.hidden); >> >> The rest of the CSS/JS parts look good to me. Thanks. > > Maybe I am confused, but I don't know if you realized that the contains and the remove are called on different objects. What I am trying to do is not showing the remaining time if the current is currently shown. You're right. I missed that detail. Please disregard that comment.
Comment on attachment 225247 [details] Patch r=me, based on James's comments.
Comment on attachment 225247 [details] Patch Clearing flags on attachment: 225247 Committed r164782: <http://trac.webkit.org/changeset/164782>
All reviewed patches have been landed. Closing bug.