Bug 29126 – Play/pause button in <video> controls shows the wrong image

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Roben (:aroben)]

Created attachment 39351 [details]
Patch v1

The play/pause button in <video> controls currently shows the "play" image when the video is playing, and the "pause" image when the video is paused. But in most media players (and <video> controls on Mac), the button image reflects what the button will do, not the current state of the video.

To reproduce:

1. Go to http://movies.apple.com/datapub/us/podcasts/apple_keynotes/sept2009_event.m4v

When the movie starts playing, the play/pause button shows a "play" image. If you [A[A[B[ou click it, it will show a "pause" button. This is backwards!

[Eric Carlson]

Comment on attachment 39351 [details]
Patch v1

> +        No test possible.

The existing pixel tests will check this as a side effect - once you update the results ;-)


> -                bool currentlyPlaying = btn->displayType() == MediaPlayButton;
> -                paintThemePart(currentlyPlaying ? SafariTheme::MediaPauseButtonPart : SafariTheme::MediaPlayButtonPart, paintInfo.context->platformContext(), r, NSRegularControlSize, determineState(o));
> +                bool canPlay = btn->displayType() == MediaPlayButton;
> +                paintThemePart(canPlay ? SafariTheme::MediaPlayButtonPart : SafariTheme::MediaPauseButtonPart, paintInfo.context->platformContext(), r, NSRegularControlSize, determineState(o));

"canPlay" can be slightly misleading because the movie may not have enough media buffered to begin playback immediately, but I may be too pedantic.

r=me if you update the pixel test results at some point.

[Adam Roben (:aroben)]

Committed r48255: <http://trac.webkit.org/changeset/48255>

[Adam Roben (:aroben)]

Shoot, I missed that you had made comments! Sorry about that.

(In reply to comment #1)
> (From update of attachment 39351 [details])
> > +        No test possible.
> 
> The existing pixel tests will check this as a side effect - once you update the
> results ;-)

We don't run pixel tests on Windows, nor do we have any expected results for pixel tests on Windows. Since this is a Windows-specific change, I don't think there's anything I can do here. Am I missing something?

> > -                bool currentlyPlaying = btn->displayType() == MediaPlayButton;
> > -                paintThemePart(currentlyPlaying ? SafariTheme::MediaPauseButtonPart : SafariTheme::MediaPlayButtonPart, paintInfo.context->platformContext(), r, NSRegularControlSize, determineState(o));
> > +                bool canPlay = btn->displayType() == MediaPlayButton;
> > +                paintThemePart(canPlay ? SafariTheme::MediaPlayButtonPart : SafariTheme::MediaPauseButtonPart, paintInfo.context->platformContext(), r, NSRegularControlSize, determineState(o));
> 
> "canPlay" can be slightly misleading because the movie may not have enough
> media buffered to begin playback immediately, but I may be too pedantic.

I took the "canPlay" terminology from MediaControlPlayButtonElement::updateDisplayType <http://trac.webkit.org/browser/trunk/WebCore/rendering/MediaControlElements.cpp?rev=47774#L473>. Is the terminology inaccurate there, too?

[Eric Carlson]

> We don't run pixel tests on Windows, nor do we have any expected results for
> pixel tests on Windows. Since this is a Windows-specific change, I don't think
> there's anything I can do here. Am I missing something?

Oops, I guess the only thing you (we) are missing is pixel tests on Windows :-)