89384 2012-06-18 14:34:52 -0700 Looks like wrong position of isSVGForeignObject checks in RenderObject::container 2012-06-19 10:55:49 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 inferno inferno eric fmalita hyatt mitz pdr schenney simon.fraser webkit.review.bot zimmermann oldest_to_newest 651736 0 inferno 2012-06-18 14:34:52 -0700 RenderObject* RenderObject::container(const RenderBoxModelObject* repaintContainer, bool* repaintContainerSkipped) const { ................... EPosition pos = m_style->position(); if (pos == FixedPosition) { // container() can be called on an object that is not in the // tree yet. We don't call view() since it will assert if it // can't get back to the canvas. Instead we just walk as high up // as we can. If we're in the tree, we'll get the root. If we // aren't we'll get the root of our little subtree (most likely // we'll just return 0). // FIXME: The definition of view() has changed to not crawl up the render tree. It might // be safe now to use it. while (o && o->parent() && !(o->hasTransform() && o->isRenderBlock())) { if (repaintContainerSkipped && o == repaintContainer) *repaintContainerSkipped = true; #if ENABLE(SVG) // foreignObject is the containing block for its contents. if (o->isSVGForeignObject()) break; #endif o = o->parent(); } } else if (pos == AbsolutePosition) { // Same goes here. We technically just want our containing block, but // we may not have one if we're part of an uninstalled subtree. We'll // climb as high as we can though. while (o && o->style()->position() == StaticPosition && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) { if (repaintContainerSkipped && o == repaintContainer) *repaintContainerSkipped = true; #if ENABLE(SVG) if (o->isSVGForeignObject()) // foreignObject is the containing block for contents inside it break; --------------------------- Shouldn't 'if (o->isSVGForeignObject()' be before the 'if (repaintContainerSkipped && o == repaintContainer)' check for both fixed positioned objects, absolute positioned objects. Can someone in the SVG team take a look and fix this. 651827 1 schenney 2012-06-18 15:52:01 -0700 It definitely seems wrong. We are not skipping the object if it's SVG - we're breaking out. So we shouldn't be setting the flag as skipped. Go ahead and fix, or ask pdr or fmalita. I'm not bug fixing while traveling. 651857 2 148200 inferno 2012-06-18 16:19:35 -0700 Created attachment 148200 Patch 651880 3 148200 krit 2012-06-18 16:56:11 -0700 Comment on attachment 148200 Patch Is it possible to test it with a DRT test? 651900 4 inferno 2012-06-18 17:20:02 -0700 (In reply to comment #3) > (From update of attachment 148200 [details]) > Is it possible to test it with a DRT test? I don't know the painting pipeline well enough to write a failing test. 652498 5 simon.fraser 2012-06-19 08:50:52 -0700 Patch seems reasonable; we might never hit this on real content. repaintContainerSkipped kicks in when something like opacity creates a compositing layer between an abspos div and its container ancestor: <div style="position: relative"> <div style="opacity: 0.5"> <div style="-webkit-transform: translateZ(0);"> <!-- kick the next div into compositing --> <div style="position: absolute;"> For isSVGForeignObject to kick in, you need to be inside an svg <foreignObject> and to have the repaintContainer in the SVG. Since svg doesn't do compositing, I'm not sure this is possible. 652534 6 inferno 2012-06-19 09:47:21 -0700 (In reply to comment #5) > Patch seems reasonable; we might never hit this on real content. > > repaintContainerSkipped kicks in when something like opacity creates a compositing layer between an abspos div and its container ancestor: > > <div style="position: relative"> > <div style="opacity: 0.5"> > <div style="-webkit-transform: translateZ(0);"> <!-- kick the next div into compositing --> > <div style="position: absolute;"> > > For isSVGForeignObject to kick in, you need to be inside an svg <foreignObject> and to have the repaintContainer in the SVG. Since svg doesn't do compositing, I'm not sure this is possible. Thanks Simon. So, looks like there is not way to test this. Can you please review this so that atleast we don't regress in the future. 652602 7 148200 webkit.review.bot 2012-06-19 10:55:41 -0700 Comment on attachment 148200 Patch Clearing flags on attachment: 148200 Committed r120732: <http://trac.webkit.org/changeset/120732> 652603 8 webkit.review.bot 2012-06-19 10:55:49 -0700 All reviewed patches have been landed. Closing bug. 148200 2012-06-18 16:19:35 -0700 2012-06-19 10:55:41 -0700 Patch bug-89384-20120618161934.patch text/plain 2694 inferno U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwNjI5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjNlYTZlMzYzNWRiODQ4 ZmU2YTZhMTUxOWJiN2YxNjg4ZTNhZDk1My4uZmJkNTRjZTNkMGNjOTcwMGMzZGFhYmM2OTNjMDM3 N2RmMzdiM2QzNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA2LTE4ICBBYmhp c2hlayBBcnlhICA8aW5mZXJub0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgV3JvbmcgcmVwYWlu dENvbnRhaW5lclNraXBwZWQgaW4gUmVuZGVyT2JqZWN0Ojpjb250YWluZXIoKQorICAgICAgICB3 aGVuIHBvc2l0aW9uZWQgb2JqZWN0cyBhcmUgZW5jbG9zZWQgaW4gYSA8Zm9yZWlnbk9iamVjdD4u CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04OTM4NAor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyB0 ZXN0cy4gVGhlIGJ1ZyB3YXMgZm91bmQgaW4gYSBjb2RlIGF1ZGl0LgorCisgICAgICAgICogcmVu ZGVyaW5nL1JlbmRlck9iamVjdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJPYmplY3Q6 OmNvbnRhaW5lcik6CisKIDIwMTItMDYtMTggIE5pY28gV2ViZXIgIDx0aGFraXNAY2hyb21pdW0u b3JnPgogCiAgICAgICAgIFtjaHJvbWl1bS9tYWNdIFVuYnJlYWsgc21vb3RoIHNjcm9sbGluZy4K ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuY3BwIGIv U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAKaW5kZXggNzgwZWI4YzBl ODU0YWQwYzEzMDc4MzNiYzQwYWQ1NGUxNDFkYzllZS4uZjQzNGJiMWI1OTg5NmNhMTRmOTQ4MTBm YWEzOTkyYzE1MzA1YWU0NSAxMDA3NTUKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlck9iamVjdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVj dC5jcHAKQEAgLTIyMTYsMTMgKzIyMTYsMTQgQEAgUmVuZGVyT2JqZWN0KiBSZW5kZXJPYmplY3Q6 OmNvbnRhaW5lcihjb25zdCBSZW5kZXJCb3hNb2RlbE9iamVjdCogcmVwYWludENvbnRhaW4KICAg ICAgICAgLy8gRklYTUU6IFRoZSBkZWZpbml0aW9uIG9mIHZpZXcoKSBoYXMgY2hhbmdlZCB0byBu b3QgY3Jhd2wgdXAgdGhlIHJlbmRlciB0cmVlLiAgSXQgbWlnaHQKICAgICAgICAgLy8gYmUgc2Fm ZSBub3cgdG8gdXNlIGl0LgogICAgICAgICB3aGlsZSAobyAmJiBvLT5wYXJlbnQoKSAmJiAhKG8t Pmhhc1RyYW5zZm9ybSgpICYmIG8tPmlzUmVuZGVyQmxvY2soKSkpIHsKLSAgICAgICAgICAgIGlm IChyZXBhaW50Q29udGFpbmVyU2tpcHBlZCAmJiBvID09IHJlcGFpbnRDb250YWluZXIpCi0gICAg ICAgICAgICAgICAgKnJlcGFpbnRDb250YWluZXJTa2lwcGVkID0gdHJ1ZTsKICNpZiBFTkFCTEUo U1ZHKQogICAgICAgICAgICAgLy8gZm9yZWlnbk9iamVjdCBpcyB0aGUgY29udGFpbmluZyBibG9j ayBmb3IgaXRzIGNvbnRlbnRzLgogICAgICAgICAgICAgaWYgKG8tPmlzU1ZHRm9yZWlnbk9iamVj dCgpKQogICAgICAgICAgICAgICAgIGJyZWFrOwogI2VuZGlmCisgICAgICAgICAgICBpZiAocmVw YWludENvbnRhaW5lclNraXBwZWQgJiYgbyA9PSByZXBhaW50Q29udGFpbmVyKQorICAgICAgICAg ICAgICAgICpyZXBhaW50Q29udGFpbmVyU2tpcHBlZCA9IHRydWU7CisKICAgICAgICAgICAgIG8g PSBvLT5wYXJlbnQoKTsKICAgICAgICAgfQogICAgIH0gZWxzZSBpZiAocG9zID09IEFic29sdXRl UG9zaXRpb24pIHsKQEAgLTIyMzAsMTIgKzIyMzEsMTMgQEAgUmVuZGVyT2JqZWN0KiBSZW5kZXJP YmplY3Q6OmNvbnRhaW5lcihjb25zdCBSZW5kZXJCb3hNb2RlbE9iamVjdCogcmVwYWludENvbnRh aW4KICAgICAgICAgLy8gd2UgbWF5IG5vdCBoYXZlIG9uZSBpZiB3ZSdyZSBwYXJ0IG9mIGFuIHVu aW5zdGFsbGVkIHN1YnRyZWUuICBXZSdsbAogICAgICAgICAvLyBjbGltYiBhcyBoaWdoIGFzIHdl IGNhbiB0aG91Z2guCiAgICAgICAgIHdoaWxlIChvICYmIG8tPnN0eWxlKCktPnBvc2l0aW9uKCkg PT0gU3RhdGljUG9zaXRpb24gJiYgIW8tPmlzUmVuZGVyVmlldygpICYmICEoby0+aGFzVHJhbnNm b3JtKCkgJiYgby0+aXNSZW5kZXJCbG9jaygpKSkgewotICAgICAgICAgICAgaWYgKHJlcGFpbnRD b250YWluZXJTa2lwcGVkICYmIG8gPT0gcmVwYWludENvbnRhaW5lcikKLSAgICAgICAgICAgICAg ICAqcmVwYWludENvbnRhaW5lclNraXBwZWQgPSB0cnVlOwogI2lmIEVOQUJMRShTVkcpCiAgICAg ICAgICAgICBpZiAoby0+aXNTVkdGb3JlaWduT2JqZWN0KCkpIC8vIGZvcmVpZ25PYmplY3QgaXMg dGhlIGNvbnRhaW5pbmcgYmxvY2sgZm9yIGNvbnRlbnRzIGluc2lkZSBpdAogICAgICAgICAgICAg ICAgIGJyZWFrOwogI2VuZGlmCisgICAgICAgICAgICBpZiAocmVwYWludENvbnRhaW5lclNraXBw ZWQgJiYgbyA9PSByZXBhaW50Q29udGFpbmVyKQorICAgICAgICAgICAgICAgICpyZXBhaW50Q29u dGFpbmVyU2tpcHBlZCA9IHRydWU7CisKICAgICAgICAgICAgIG8gPSBvLT5wYXJlbnQoKTsKICAg ICAgICAgfQogICAgIH0K