Bug 82476 – Extra display logic for the media control panel element

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

In order to have the media control panel element toggle the display:none property when the fadeout
transition ends, we need to ensure that the property isn't removed when the default controls should not be displayed.

[Victor Carbune]

Created attachment 136017 [details]
Fixed display bug and removed timer

[Victor Carbune]

Comment on attachment 136017 [details]
Fixed display bug and removed timer

It seems that this method doesn't actually work, because dispatchEvent is called only when a listener exists. I got tricked by the fact that the test passes (because it has waitForEvent). The only way to do this is using a timer - I will add a simpler patch just adding the extra variable.

[Victor Carbune]

Created attachment 136291 [details]
Simple patch

[Victor Carbune]

Created attachment 136293 [details]
Updated test expected output

[Daniel Bates]

Comment on attachment 136293 [details]
Updated test expected output

View in context: https://bugs.webkit.org/attachment.cgi?id=136293&action=review

I'm r-ing this patch because it doesn't have a properly formatted change log entry. In particular, it's missing a "Reviewed by" line in the WebCore/ChangeLog file.

> Source/WebCore/ChangeLog:7
> +        This patch fixes a bug which caused the controls to be displayed
> +        when they should remain hidden. Added an extra variable to the
> +        panel elements which properly keeps the state of the panel (visible or not).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=82476

This change log entry doesn't conform to the format we use for change log entries. In particular, it's missing a bug title and a "Reviewed by" line. The top portion of a change log entry has the following lines (in order): data/author's full name/author's email address, bug title, bug URL, Reviewed by line, and change description. See <http://www.webkit.org/coding/contributing.html#changelogs> for more details, which references an example entry. Another example of a change log entry that conforms to the aforementioned format is <http://trac.webkit.org/changeset/103692/trunk/Source/WebCore/ChangeLog>.

> LayoutTests/ChangeLog:8
> +        Added test to ensure that controls are not displayed when
> +        the controls attribute is not set.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=82476
> +
> +        Reviewed by NOBODY (OOPS!).

Similarly, this change log entry doesn't conform to the format we use for a change log entry.

[Victor Carbune]

Created attachment 136458 [details]
Fixed ChangeLog entries

[Victor Carbune]

(In reply to comment #5)
> (From update of attachment 136293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136293&action=review
> 
> I'm r-ing this patch because it doesn't have a properly formatted change log entry. In particular, it's missing a "Reviewed by" line in the WebCore/ChangeLog file.

I have changed both ChangeLogs, I wasn't aware how strict the rules are. Thank you for pointing this out.

Thanks for taking the time to review this.

[Eric Carlson]

Comment on attachment 136458 [details]
Fixed ChangeLog entries

View in context: https://bugs.webkit.org/attachment.cgi?id=136458&action=review

> LayoutTests/media/video-controls-toggling.html:11
> +        var fadeoutTime = 1000;

This is a very long pause for a layout test. The controls fade actually out in 0.3 seconds, can we make this somewhat smaller without making the test flakey? 

Could it work to set the timeout to 0.3 and have continueTest() set another timeout (only one time) if the panel is still visible?

[Victor Carbune]

Created attachment 136489 [details]
Updated test.

[Victor Carbune]

(In reply to comment #8)
> (From update of attachment 136458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136458&action=review
> 
> > LayoutTests/media/video-controls-toggling.html:11
> > +        var fadeoutTime = 1000;
> 
> This is a very long pause for a layout test. The controls fade actually out in 0.3 seconds, can we make this somewhat smaller without making the test flakey? 
> 
> Could it work to set the timeout to 0.3 and have continueTest() set another timeout (only one time) if the panel is still visible?

Your solution works good on my machine and I hope it won't be flaky (unfortunately I can't think of any other way of testing this, besides using setTimeout)

[Eric Carlson]

Comment on attachment 136489 [details]
Updated test.

View in context: https://bugs.webkit.org/attachment.cgi?id=136489&action=review

> LayoutTests/media/video-controls-toggling.html:58
> +            if (panel.style['display'] != 'none') {
> +                setTimeout(continueTest, fadeoutTime);
> +                return;
> +            }

The only problem with this is that it will set the timer again and again until the test is killed, so it might be better to only do this once. On the other hand, the test will timeout eventually so this might be OK. Your call.

[Victor Carbune]

(In reply to comment #11)
> (From update of attachment 136489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136489&action=review
> 
> > LayoutTests/media/video-controls-toggling.html:58
> > +            if (panel.style['display'] != 'none') {
> > +                setTimeout(continueTest, fadeoutTime);
> > +                return;
> > +            }
> 
> The only problem with this is that it will set the timer again and again until the test is killed, so it might be better to only do this once. On the other hand, the test will timeout eventually so this might be OK. Your call.

I think that rather than dealing with flaky tests because of slightly different observed fadeout times on various platforms, we are better off with a timeout, meaning that the test really is failing.

[Eric Carlson]

Victor, please mark this cq? if it is ready to land.

[Victor Carbune]

(In reply to comment #13)
> Victor, please mark this cq? if it is ready to land.
This patch enables toggling display:none on the controls and depends on the volume slider rendering fix before it can be marked with cq.

[Victor Carbune]

Created attachment 138540 [details]
Rebased

[WebKit Review Bot]

Comment on attachment 138540 [details]
Rebased

Clearing flags on attachment: 138540

Committed r115125: <http://trac.webkit.org/changeset/115125>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.