Bug 84570 – audio controls have a 1px surplus outline

• Status: CLOSED
• Resolution: FIXED
• Component: Media

[Silvia Pfeiffer]

The audio controls have a 1px grey outline that is screwing with my video controls update for Chromium, see attached image mediacontrols.jpg .

It is created by WebKit, since the Safari controls have the same issue, see attached image audio_controls_shadow_display_none.png. This image was created by loading a html page with only an audio element into Safari and setting the shadow dom that creates the audio controls to "display:none".

I've confirmed that if I just return from "RenderImage::paintReplaced" in src/third_party/WebKit/Source/WebCore/rendering/RenderImage.cpp , I can remove that outline.

Thus, my theory is that the audio element, being a stripped-down video element, might try to render a poster, which does not exist and is therefore rendering the 1px grey and empty outline of an image.

Will need to further debug, so registering so I don't forget.

[Silvia Pfeiffer]

Created attachment 138299 [details]
1px grey outline on audio control for new chrome controls

[Silvia Pfeiffer]

Created attachment 138300 [details]
audio controls with shadow dom set to display none in Safari

[Eric Carlson]

(In reply to comment #0)
> 
> I've confirmed that if I just return from "RenderImage::paintReplaced" in src/third_party/WebKit/Source/WebCore/rendering/RenderImage.cpp , I can remove that outline.
> 
> Thus, my theory is that the audio element, being a stripped-down video element, might try to render a poster, which does not exist and is therefore rendering the 1px grey and empty outline of an image.
> 
This theory is incorrect. HTMLAudioElement has a RenderMedia when it has controls, HTMLVideoElement has a RenderVideo, and RenderMedia doesn't know anything at all about the poster. 

Have you tried adding "border: initial;" to [video|audio]::-webkit-media-controls-panel?

[Silvia Pfeiffer]

(In reply to comment #3)
>
> Have you tried adding "border: initial;" to [video|audio]::-webkit-media-controls-panel?

I've tried that, but it's not the CSS on any of the shadow DOM elements. In fact, if you completely remove the shadow dom with "display:none", the border is still there.

[Silvia Pfeiffer]

Created attachment 139129 [details]
Patch

[Silvia Pfeiffer]

Turns out it's only a function that was inherited from the RenderImage class that was missing for RenderMedia , but exists for RenderVideo. Has indeed nothing to do with posters. :-)

[Eric Carlson]

Comment on attachment 139129 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        * media/audio-controls-no-image-outline.html: Added.

It looks like you forgot to "svn add" the new test results.

> LayoutTests/media/audio-controls-no-image-outline.html:3
> +    This tests that audio controls do not fade out when the audio is playing.

Is this comment correct?

> LayoutTests/media/audio-controls-no-image-outline.html:13
> +<p id="result">
> +    FAIL: Test did not run.
> +</p>
> +<audio id="audio" controls autoplay onplaying="playing()" src="content/silence.wav"></audio><br>
> +<script>
> +    if (window.layoutTestController) {
> +        layoutTestController.waitUntilDone();
> +        layoutTestController.dumpAsText();
> +    }

Nit: Is there any reason to not include video-test.js (as the other media tests do) and use its helpers instead of rolling your own?

> LayoutTests/media/audio-controls-no-image-outline.html:21
> +        setTimeout(function() {
> +            var controls = internals.shadowRoot(document.getElementById("audio")).firstChild.firstChild;
> +            var opacity = getComputedStyle(controls).opacity;
> +            document.getElementById("result").innerText = opacity < 1 ? "FAIL" : "PASS";
> +            layoutTestController.notifyDone();
> +        }, 250)

How does this test your changes?

If it does test this fix, is the setTimeout really necessary? We have been trying to get rid of all timeouts in media tests because they have been the source of much flakiness, so I would really prefer to see this done another way.

[Silvia Pfeiffer]

(In reply to comment #7)
> (From update of attachment 139129 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139129&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        * media/audio-controls-no-image-outline.html: Added.
> 
> It looks like you forgot to "svn add" the new test results.

Oops. As it turned out, I didn't need a new test and didn't actually want this in this patch. Other tests are covering this change already. Will remove.


> > LayoutTests/media/audio-controls-no-image-outline.html:3
> > +    This tests that audio controls do not fade out when the audio is playing.
> 
> Is this comment correct?

Will remove this file.


> > LayoutTests/media/audio-controls-no-image-outline.html:13
> > +<p id="result">
> > +    FAIL: Test did not run.
> > +</p>
> > +<audio id="audio" controls autoplay onplaying="playing()" src="content/silence.wav"></audio><br>
> > +<script>
> > +    if (window.layoutTestController) {
> > +        layoutTestController.waitUntilDone();
> > +        layoutTestController.dumpAsText();
> > +    }
> 
> Nit: Is there any reason to not include video-test.js (as the other media tests do) and use its helpers instead of rolling your own?

Will remove this file.

 
> > LayoutTests/media/audio-controls-no-image-outline.html:21
> > +        setTimeout(function() {
> > +            var controls = internals.shadowRoot(document.getElementById("audio")).firstChild.firstChild;
> > +            var opacity = getComputedStyle(controls).opacity;
> > +            document.getElementById("result").innerText = opacity < 1 ? "FAIL" : "PASS";
> > +            layoutTestController.notifyDone();
> > +        }, 250)
> 
> How does this test your changes?
> 
> If it does test this fix, is the setTimeout really necessary? We have been trying to get rid of all timeouts in media tests because they have been the source of much flakiness, so I would really prefer to see this done another way.

Will remove this file.

[Silvia Pfeiffer]

Created attachment 139267 [details]
Patch

Remove surplus test

[WebKit Review Bot]

Comment on attachment 139267 [details]
Patch

Attachment 139267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12553516

New failing tests:
media/audio-repaint.html
media/audio-controls-rendering.html
media/media-controls-clone.html
media/controls-layout-direction.html

[WebKit Review Bot]

Created attachment 139345 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Carlson]

Comment on attachment 139267 [details]
Patch

This change looks fine to me, but it will definitely break the build because you don't have new test results for every port that needs them. I don't think you should use the commit queue to land it because you need to be on hand when it lands to fix the breakage.

[Eric Carlson]

Comment on attachment 139267 [details]
Patch

setting cq+ even though we know this will break cr-linux, Silvia will update results once it lands.

[WebKit Review Bot]

Comment on attachment 139267 [details]
Patch

Rejecting attachment 139267 [details] from commit-queue.

New failing tests:
media/audio-repaint.html
media/audio-controls-rendering.html
media/media-controls-clone.html
media/controls-layout-direction.html
Full output: http://queues.webkit.org/results/12586412

[WebKit Review Bot]

Created attachment 139539 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Silvia Pfeiffer]

Created attachment 139615 [details]
Patch

Added layout tests for Chromium

[WebKit Review Bot]

Comment on attachment 139615 [details]
Patch

Attachment 139615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12591548

New failing tests:
media/audio-repaint.html
media/controls-after-reload.html
media/audio-controls-rendering.html
media/media-controls-clone.html
media/controls-layout-direction.html

[WebKit Review Bot]

Created attachment 139620 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Silvia Pfeiffer]

Created attachment 139623 [details]
Patch

Fix cr-linux tests

[Eric Carlson]

Comment on attachment 139623 [details]
Patch

Many of these images look identical, do you really need five different versions of each? For example, could you replace chromium-linux/media/audio-repaint-expected.png, chromium-mac-leopard/media/audio-repaint-expected.png, chromium-mac-snowleopard/media/audio-repaint-expected.png, chromium-mac/media/audio-repaint-expected.png, and chromium-win/media/audio-repaint-expected.png with chromium/media/audio-repaint-expected.png?

[Silvia Pfeiffer]

(In reply to comment #20)
> (From update of attachment 139623 [details])
> Many of these images look identical, do you really need five different versions of each? For example, could you replace chromium-linux/media/audio-repaint-expected.png, chromium-mac-leopard/media/audio-repaint-expected.png, chromium-mac-snowleopard/media/audio-repaint-expected.png, chromium-mac/media/audio-repaint-expected.png, and chromium-win/media/audio-repaint-expected.png with chromium/media/audio-repaint-expected.png?

The linux and mac ones only look identical - they actually have different fonts and colors.

[WebKit Review Bot]

Comment on attachment 139623 [details]
Patch

Clearing flags on attachment: 139623

Committed r115749: <http://trac.webkit.org/changeset/115749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.