Bug 149111

Created attachment 261112 [details] Patch

Can you please tell me which diagnostics have you made so that you reach the conclusion that this is the best patch? It is weird because we shouldn't need to check the opacity as precisely, we handle the opacity thru CSS with the classes we add to the element.

(In reply to comment #2) > Can you please tell me which diagnostics have you made so that you reach the > conclusion that this is the best patch? > > It is weird because we shouldn't need to check the opacity as precisely, we > handle the opacity thru CSS with the classes we add to the element. As I described the situation in the ChangeLog, We changed not to update time if controllAreHidden is true in https://bugs.webkit.org/show_bug.cgi?id=137773. However this affects on controls behavior very much. Let guess this scenario. (move cursor on playbutton) -> (click play button) -> (hideControl timer fired by resetHideControlsTimer after 4 secs) -> (now controlsAreHidden is true since panel has neither -webkit-media-controls-panel.show nor -webkit-media-controls-panel.hidden) -> (playing along with visible controls due to -webkit-media-controls-panel:hover which sets opacity to 1, but time is not updated because of controlsAreHidden == 1) So the problem here is controlsAreHidden does not actually mean "hidden" and I check the opacity is really 0. I don't want to argue this approach is the best. Any better idea is welcome. =)

This fixes audio controls broken as well. Try http://www.quackit.com/html_5/tags/html_audio_tag.cfm

(In reply to comment #3) > As I described the situation in the ChangeLog, We changed not to update time > if controllAreHidden is true in > https://bugs.webkit.org/show_bug.cgi?id=137773. > However this affects on controls behavior very much. Let guess this scenario. > (move cursor on playbutton) -> (click play button) -> (hideControl timer > fired by resetHideControlsTimer after 4 secs) -> (now controlsAreHidden is > true since panel has neither -webkit-media-controls-panel.show nor > -webkit-media-controls-panel.hidden) -> (playing along with visible controls > due to -webkit-media-controls-panel:hover which sets opacity to 1, but time > is not updated because of controlsAreHidden == 1) > So the problem here is controlsAreHidden does not actually mean "hidden" and > I check the opacity is really 0. I understand now, good catch. You're right, another check needs to be done, though I don't think checking for opacity is the best way to go, but checking for the :hover property as :hover can change opacity, display or whatever we want from CSS. IMHO, we are safer with checking for :hover.

Created attachment 261311 [details] Patch

(In reply to comment #5) > (In reply to comment #3) > > As I described the situation in the ChangeLog, We changed not to update time > > if controllAreHidden is true in > > https://bugs.webkit.org/show_bug.cgi?id=137773. > > However this affects on controls behavior very much. Let guess this scenario. > > (move cursor on playbutton) -> (click play button) -> (hideControl timer > > fired by resetHideControlsTimer after 4 secs) -> (now controlsAreHidden is > > true since panel has neither -webkit-media-controls-panel.show nor > > -webkit-media-controls-panel.hidden) -> (playing along with visible controls > > due to -webkit-media-controls-panel:hover which sets opacity to 1, but time > > is not updated because of controlsAreHidden == 1) > > So the problem here is controlsAreHidden does not actually mean "hidden" and > > I check the opacity is really 0. > > I understand now, good catch. You're right, another check needs to be done, > though I don't think checking for opacity is the best way to go, but > checking for the :hover property as :hover can change opacity, display or > whatever we want from CSS. IMHO, we are safer with checking for :hover. Otherwise we can use querySelector to check :hover existence. However, I'm not sure if that way is better than checking opacity directly in performance aspect..

(In reply to comment #7) > Otherwise we can use querySelector to check :hover existence. However, I'm > not sure if that way is better than checking opacity directly in performance > aspect.. LGTM. I don't think, unless you correct me, that performance is going to be a problem.

Comment on attachment 261311 [details] Patch Thanks!

Comment on attachment 261311 [details] Patch Clearing flags on attachment: 261311 Committed r190053: <http://trac.webkit.org/changeset/190053>

All reviewed patches have been landed. Closing bug.