Bug 29126 - Play/pause button in <video> controls shows the wrong image
Summary: Play/pause button in <video> controls shows the wrong image
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-10 08:44 PDT by Adam Roben (:aroben)
Modified: 2009-09-10 09:14 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (1.98 KB, patch)
2009-09-10 08:44 PDT, Adam Roben (:aroben)
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2009-09-10 08:44:07 PDT
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!
Comment 1 Eric Carlson 2009-09-10 08:53:32 PDT
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.
Comment 2 Adam Roben (:aroben) 2009-09-10 08:57:36 PDT
Committed r48255: <http://trac.webkit.org/changeset/48255>
Comment 3 Adam Roben (:aroben) 2009-09-10 09:04:16 PDT
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?
Comment 4 Eric Carlson 2009-09-10 09:14:06 PDT
> 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 :-)