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.
Created attachment 138299 [details] 1px grey outline on audio control for new chrome controls
Created attachment 138300 [details] audio controls with shadow dom set to display none in Safari
(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?
(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.
Created attachment 139129 [details] Patch
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. :-)
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.
(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.
Created attachment 139267 [details] Patch Remove surplus test
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
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
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.
Comment on attachment 139267 [details] Patch setting cq+ even though we know this will break cr-linux, Silvia will update results once it lands.
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
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
Created attachment 139615 [details] Patch Added layout tests for Chromium
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
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
Created attachment 139623 [details] Patch Fix cr-linux tests
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?
(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.
Comment on attachment 139623 [details] Patch Clearing flags on attachment: 139623 Committed r115749: <http://trac.webkit.org/changeset/115749>
All reviewed patches have been landed. Closing bug.