Bug 38829

Summary:

The HTML5 video element in Safari does not respect "visibility:hidden" CSS property

Product:

WebKit

Reporter:

Silvia Pfeiffer <silviapfeiffer1>

Component:

Media

Assignee:

Simon Fraser (smfr) <simon.fraser>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

ap, eric.carlson, gustavo, jamesr, pnormand, sharonfrankklin, simon.fraser, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.6

Bug Depends on:

Bug Blocks:

75682

Attachments:

Description	Flags
safari bug with visibility:hidden on <video> element	none
example video for bug test	none
test tarball	none
Patch	jamesr: review+, gustavo: commit-queue-

Silvia Pfeiffer

Reported 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.

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+ gustavo: commit-queue-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Mark Rowe (bdash)

Comment 1 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.

Alexey Proskuryakov

Comment 2 2010-05-10 14:41:20 PDT

I also cannot reproduce this.

Silvia Pfeiffer

Comment 3 2010-05-10 16:13:49 PDT

It was Safari Version 4.0.5 (6531.22.7)

Alexey Proskuryakov

Comment 4 2010-05-10 16:23:00 PDT

Does this happen for you with a nightly build from <http://nightly.webkit.org>?

Silvia Pfeiffer

Comment 5 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.

Silvia Pfeiffer

Comment 6 2010-05-10 17:38:39 PDT

Created attachment 55627 [details] example video for bug test attaching video with which I see the bug

Eric Carlson

Comment 7 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?

Silvia Pfeiffer

Comment 8 2010-05-12 17:53:37 PDT

Created attachment 55916 [details] test tarball my directory with test

Silvia Pfeiffer

Comment 9 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. :(

Alexey Proskuryakov

Comment 10 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.

Silvia Pfeiffer

Comment 11 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! :)

Simon Fraser (smfr)

Comment 12 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.

Silvia Pfeiffer

Comment 13 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.

Eric Carlson

Comment 14 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

Simon Fraser (smfr)

Comment 15 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.

Silvia Pfeiffer

Comment 16 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.

Simon Fraser (smfr)

Comment 17 2011-10-27 14:44:35 PDT

Created attachment 112757 [details] Patch

James Robinson

Comment 18 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?

Simon Fraser (smfr)

Comment 19 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.

Gustavo Noronha (kov)

Comment 20 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

Simon Fraser (smfr)

Comment 21 2011-10-28 10:46:57 PDT

http://trac.webkit.org/changeset/98735

Philippe Normand

Comment 22 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)

Simon Fraser (smfr)

Comment 23 2011-10-28 12:34:44 PDT

qt-ews said "Unable to clean working directory" so I assumed that was a bot problem.

Note You need to log in before you can comment on or make changes to this bug.