Bug 38829 - The HTML5 video element in Safari does not respect "visibility:hidden" CSS property
Summary: The HTML5 video element in Safari does not respect "visibility:hidden" CSS pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Minor
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 75682
  Show dependency treegraph
 
Reported: 2010-05-09 19:04 PDT by Silvia Pfeiffer
Modified: 2019-05-02 16:24 PDT (History)
8 users (show)

See Also:


Attachments
safari bug with visibility:hidden on <video> element (88.74 KB, image/png)
2010-05-09 19:04 PDT, Silvia Pfeiffer
no flags Details
example video for bug test (1.57 MB, video/mp4)
2010-05-10 17:38 PDT, Silvia Pfeiffer
no flags Details
test tarball (1.56 MB, application/x-gzip)
2010-05-12 17:53 PDT, Silvia Pfeiffer
no flags Details
Patch (54.79 KB, patch)
2011-10-27 14:44 PDT, Simon Fraser (smfr)
jamesr: review+
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Silvia Pfeiffer 2010-05-09 19:04:48 PDT
Created attachment 55517 [details]
safari bug with visibility:hidden on <video> element

Just created a Web page example like this:

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>visibility bug</title>
    <style type="text/css">
    video {
      visibility: hidden;
    }
    </style>
  </head>
  <body>
    <p>A paragraph before the video element.</p>
    <video controls>
        <source src="HelloWorld.mp4" type="video/mp4">
        <source src="HelloWorld.ogv" type="video/ogg">
    </video>
    <p>A paragraph after the video element.</p>
  </body>
</html>

Safari displays the first frame of the video even though the video has been set to hidden (see attached screen shot). All other browsers including Google Chrome make the video element invisible.
Comment 1 Mark Rowe (bdash) 2010-05-10 11:52:44 PDT
Which build did you see this with?  I cannot reproduce it with a tip of tree build of WebKit.
Comment 2 Alexey Proskuryakov 2010-05-10 14:41:20 PDT
I also cannot reproduce this.
Comment 3 Silvia Pfeiffer 2010-05-10 16:13:49 PDT
It was Safari Version 4.0.5 (6531.22.7)
Comment 4 Alexey Proskuryakov 2010-05-10 16:23:00 PDT
Does this happen for you with a nightly build from <http://nightly.webkit.org>?
Comment 5 Silvia Pfeiffer 2010-05-10 17:34:07 PDT
Just installed Version 4.0.5 (6531.22.7, r59046) and it still happens. Attaching the video file I am using.
Comment 6 Silvia Pfeiffer 2010-05-10 17:38:39 PDT
Created attachment 55627 [details]
example video for bug test

attaching video with which I see the bug
Comment 7 Eric Carlson 2010-05-12 09:53:06 PDT
I can't reproduce with your test video either. Maybe it has something to do with the seconds <source> element? Can you try it after moving the ogg file to another directory?
Comment 8 Silvia Pfeiffer 2010-05-12 17:53:37 PDT
Created attachment 55916 [details]
test tarball

my directory with test
Comment 9 Silvia Pfeiffer 2010-05-12 17:55:29 PDT
I've restarted safari, I've restarted webkit and I still have the problem. I could restart the computer - will try tomorrow if possible. I don't have XiphQT installed and I removed the HelloWorld.png poster from the directory, too (see new attachment). It's still there. :(
Comment 10 Alexey Proskuryakov 2010-05-12 18:04:59 PDT
I can reproduce this with Safari/WebKit 4.0.5 and with r57789 on Mac OS X 10.6.3.
Comment 11 Silvia Pfeiffer 2010-05-12 18:20:02 PDT
(In reply to comment #10)
> I can reproduce this with Safari/WebKit 4.0.5 and with r57789 on Mac OS X 10.6.3.

Glad you can! I thought I was going mental! :)
Comment 12 Simon Fraser (smfr) 2010-05-12 20:32:53 PDT
Compositing layers in general don't support visibility:hidden (bug 29984 is related) and this is one example.
Comment 13 Silvia Pfeiffer 2010-05-12 23:04:22 PDT
(In reply to comment #12)
> Compositing layers in general don't support visibility:hidden (bug 29984 is related) and this is one example.

Video is flow content and not a compositing layer. Also, it works in Google Chrome (as well as Opera and Firefox, which are not really as relevant, but show that it's the right thing to do). So, my guess is it's a bug with the video implementation in Safari only.
Comment 14 Eric Carlson 2010-05-13 06:55:25 PDT
It is a bug with any element that uses a compositing layer (ie. is hardware composited). Simon has a work in progress patch attached to 29984
Comment 15 Simon Fraser (smfr) 2010-05-13 07:26:34 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Compositing layers in general don't support visibility:hidden (bug 29984 is related) and this is one example.
> 
> Video is flow content and not a compositing layer.

We give RenderVideo a RenderLayer so we can hardware-accelerate video playback.
Comment 16 Silvia Pfeiffer 2010-05-13 08:46:45 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Compositing layers in general don't support visibility:hidden (bug 29984 is related) and this is one example.
> > 
> > Video is flow content and not a compositing layer.
> 
> We give RenderVideo a RenderLayer so we can hardware-accelerate video playback.

OK. Sorry, I don't know anything about the internals of Webkit - thought "compositing" referred to the HTML type of element.
Comment 17 Simon Fraser (smfr) 2011-10-27 14:44:35 PDT
Created attachment 112757 [details]
Patch
Comment 18 James Robinson 2011-10-27 15:18:45 PDT
Comment on attachment 112757 [details]
Patch

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

I'd prefer it if we could manage the logic for transitioning between contentsVisible=false -> contentsVisible=true in shared code instead of making each GraphicsLayer implementation deal with it, since we already have API for setContentsNeedsDisplay() and it seems that every implementation will end up doing effectively that on that transition.  Otherwise this looks good to me.

R=me although I can't vouch for all of the GraphicsLayerCA.cpp changes.

> Source/WebCore/ChangeLog:8
> +        Make compositiong and CSS visibility play nicely together.

compositiong -> compositing

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:968
> +    if (m_uncommittedChanges & ContentsVisibilityChanged)
> +        updateContentsVisibility();

this is y'alls code but the order in which things are checked here is strange to me.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1207
> +        if (LayerMap* layerCloneMap = m_layerClones.get()) {
> +            LayerMap::const_iterator end = layerCloneMap->end();
> +            for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it)
> +                it->second->setContents(0);
> +        }

I have to admit I have no idea what this is, and maybe someone more familiar with GraphicsLayerCA should check it over.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:342
> +        ContentsVisibilityChanged = 1 << 24

nit: can you add a trailing comma here so the next patch doesn't cause a diff to two lines?

> Source/WebCore/rendering/RenderLayer.cpp:4047
>      // Overflow layers are just painted by their enclosing layers, so they don't get put in zorder lists.
> -    if ((m_hasVisibleContent || (m_hasVisibleDescendant && isStackingContext())) && !isNormalFlowOnly() && !renderer()->isRenderFlowThread()) {
> +    bool includeHiddenLayer = includeHiddenLayers || (m_hasVisibleContent || (m_hasVisibleDescendant && isStackingContext()));
> +    if (includeHiddenLayer && !isNormalFlowOnly() && !renderer()->isRenderFlowThread()) {

The comment does not seem to match the code very well any more. Could you update it?

> LayoutTests/compositing/visibility/visibility-image-layers-dynamic.html:13
> +/*      background-color: blue;*/

Did you mean to check this in commented out?

> LayoutTests/compositing/visibility/visibility-image-layers.html:13
> +/*      background-color: blue;*/

same here

> LayoutTests/compositing/visibility/visibility-image-layers.html:38
> +    .composited {
> +      -webkit-transform: translateZ(1px);

a lot of things like this are useful in pretty much every test in here, is it worth making a stylesheet with utility classes like this to use by reference instead of copy-paste all over the place?
Comment 19 Simon Fraser (smfr) 2011-10-27 17:36:35 PDT
(In reply to comment #18)
> (From update of attachment 112757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112757&action=review
> 
> I'd prefer it if we could manage the logic for transitioning between contentsVisible=false -> contentsVisible=true in shared code instead of making each GraphicsLayer implementation deal with it, since we already have API for setContentsNeedsDisplay() and it seems that every implementation will end up doing effectively that on that transition.

setContentsNeedsDisplay() is about painting, and doesn't cover GraphicsLayers with image or video content, which is why it's separate.

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1207
> > +        if (LayerMap* layerCloneMap = m_layerClones.get()) {
> > +            LayerMap::const_iterator end = layerCloneMap->end();
> > +            for (LayerMap::const_iterator it = layerCloneMap->begin(); it != end; ++it)
> > +                it->second->setContents(0);
> > +        }
> 
> I have to admit I have no idea what this is, and maybe someone more familiar with GraphicsLayerCA should check it over.

This is for reflections.

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:342
> > +        ContentsVisibilityChanged = 1 << 24
> 
> nit: can you add a trailing comma here so the next patch doesn't cause a diff to two lines?

Will do. Hoping no compilers complain.

> > Source/WebCore/rendering/RenderLayer.cpp:4047
> >      // Overflow layers are just painted by their enclosing layers, so they don't get put in zorder lists.
> > -    if ((m_hasVisibleContent || (m_hasVisibleDescendant && isStackingContext())) && !isNormalFlowOnly() && !renderer()->isRenderFlowThread()) {
> > +    bool includeHiddenLayer = includeHiddenLayers || (m_hasVisibleContent || (m_hasVisibleDescendant && isStackingContext()));
> > +    if (includeHiddenLayer && !isNormalFlowOnly() && !renderer()->isRenderFlowThread()) {
> 
> The comment does not seem to match the code very well any more. Could you update it?

Yep.

> > LayoutTests/compositing/visibility/visibility-image-layers.html:38
> > +    .composited {
> > +      -webkit-transform: translateZ(1px);
> 
> a lot of things like this are useful in pretty much every test in here, is it worth making a stylesheet with utility classes like this to use by reference instead of copy-paste all over the place?

A good thing for a later patch.
Comment 20 Gustavo Noronha (kov) 2011-10-27 22:52:50 PDT
Comment on attachment 112757 [details]
Patch

Attachment 112757 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10238103
Comment 21 Simon Fraser (smfr) 2011-10-28 10:46:57 PDT
http://trac.webkit.org/changeset/98735
Comment 22 Philippe Normand 2011-10-28 11:59:12 PDT
(In reply to comment #21)
> http://trac.webkit.org/changeset/98735

Why landing this patch knowing (thanks EWS) it broke the GTK build? (and win-cairo, it seems)
Comment 23 Simon Fraser (smfr) 2011-10-28 12:34:44 PDT
qt-ews said "Unable to clean working directory" so I assumed that was a bot problem.