Bug 38829

Which build did you see this with? I cannot reproduce it with a tip of tree build of WebKit.

I also cannot reproduce this.

It was Safari Version 4.0.5 (6531.22.7)

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

Just installed Version 4.0.5 (6531.22.7, r59046) and it still happens. Attaching the video file I am using.

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

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?

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

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

I can reproduce this with Safari/WebKit 4.0.5 and with r57789 on Mac OS X 10.6.3.

(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! :)

Compositing layers in general don't support visibility:hidden (bug 29984 is related) and this is one example.

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

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

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

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

Created attachment 112757 [details] Patch

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?

(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 on attachment 112757 [details] Patch Attachment 112757 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10238103

(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)

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