Bug 84570 - audio controls have a 1px surplus outline
Summary: audio controls have a 1px surplus outline
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Silvia Pfeiffer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-22 23:55 PDT by Silvia Pfeiffer
Modified: 2012-05-09 16:49 PDT (History)
6 users (show)

See Also:


Attachments
1px grey outline on audio control for new chrome controls (10.91 KB, image/jpeg)
2012-04-22 23:57 PDT, Silvia Pfeiffer
no flags Details
audio controls with shadow dom set to display none in Safari (1.73 KB, image/png)
2012-04-22 23:58 PDT, Silvia Pfeiffer
no flags Details
Patch (105.32 KB, patch)
2012-04-26 20:55 PDT, Silvia Pfeiffer
no flags Details | Formatted Diff | Diff
Patch (104.11 KB, patch)
2012-04-27 13:58 PDT, Silvia Pfeiffer
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.20 MB, application/zip)
2012-04-28 00:22 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cq-01 (5.73 MB, application/zip)
2012-04-30 16:04 PDT, WebKit Review Bot
no flags Details
Patch (748.76 KB, patch)
2012-05-01 05:46 PDT, Silvia Pfeiffer
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (6.18 MB, application/zip)
2012-05-01 06:21 PDT, WebKit Review Bot
no flags Details
Patch (661.04 KB, patch)
2012-05-01 07:23 PDT, Silvia Pfeiffer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Silvia Pfeiffer 2012-04-22 23:55:56 PDT
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.
Comment 1 Silvia Pfeiffer 2012-04-22 23:57:42 PDT
Created attachment 138299 [details]
1px grey outline on audio control for new chrome controls
Comment 2 Silvia Pfeiffer 2012-04-22 23:58:23 PDT
Created attachment 138300 [details]
audio controls with shadow dom set to display none in Safari
Comment 3 Eric Carlson 2012-04-23 10:46:18 PDT
(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?
Comment 4 Silvia Pfeiffer 2012-04-23 15:51:59 PDT
(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.
Comment 5 Silvia Pfeiffer 2012-04-26 20:55:09 PDT
Created attachment 139129 [details]
Patch
Comment 6 Silvia Pfeiffer 2012-04-26 21:07:03 PDT
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 7 Eric Carlson 2012-04-27 11:04:30 PDT
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.
Comment 8 Silvia Pfeiffer 2012-04-27 13:10:15 PDT
(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.
Comment 9 Silvia Pfeiffer 2012-04-27 13:58:10 PDT
Created attachment 139267 [details]
Patch

Remove surplus test
Comment 10 WebKit Review Bot 2012-04-28 00:22:00 PDT
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
Comment 11 WebKit Review Bot 2012-04-28 00:22:05 PDT
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 12 Eric Carlson 2012-04-30 08:31:26 PDT
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 13 Eric Carlson 2012-04-30 14:19:58 PDT
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 14 WebKit Review Bot 2012-04-30 16:04:44 PDT
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
Comment 15 WebKit Review Bot 2012-04-30 16:04:50 PDT
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
Comment 16 Silvia Pfeiffer 2012-05-01 05:46:29 PDT
Created attachment 139615 [details]
Patch

Added layout tests for Chromium
Comment 17 WebKit Review Bot 2012-05-01 06:21:07 PDT
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
Comment 18 WebKit Review Bot 2012-05-01 06:21:13 PDT
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
Comment 19 Silvia Pfeiffer 2012-05-01 07:23:24 PDT
Created attachment 139623 [details]
Patch

Fix cr-linux tests
Comment 20 Eric Carlson 2012-05-01 08:50:18 PDT
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?
Comment 21 Silvia Pfeiffer 2012-05-01 13:40:17 PDT
(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 22 WebKit Review Bot 2012-05-01 14:25:26 PDT
Comment on attachment 139623 [details]
Patch

Clearing flags on attachment: 139623

Committed r115749: <http://trac.webkit.org/changeset/115749>
Comment 23 WebKit Review Bot 2012-05-01 14:25:59 PDT
All reviewed patches have been landed.  Closing bug.