I've confirmed this issue in chromium port. If zoom level is less than 100%, the background is rendered in blue color, though it should be rendered in while color. chromium issue tracker: http://code.google.com/p/chromium/issues/detail?id=113711
Created attachment 135984 [details] Repro
Created attachment 135986 [details] Repro
The root cause of this bug is intrinsic height (or width) of background image become 0 if zoom level < 1, and it is wrongly considered as something like 'unspecified'.
Created attachment 136006 [details] Patch
Oh, when I tried to add zimmerman in the cc list, he was already added.
Is this the issue that breaks the background rendering on http://americanmccarver.com, http://www.foofighters.com/us/tour, http://wiki.ffxiclopedia.org/wiki/When_Wills_Collide when zoomed?
(In reply to comment #3) > The root cause of this bug is intrinsic height (or width) of background image become 0 if zoom level < 1, and it is wrongly considered as something like 'unspecified'. Recently the Image::size() code was unified again for SVGImages and non-SVGImages, we should use the same zoom level clamping as regular images now for SVG images. I just tried your repro and realized that this isn't about SVG images at all. I'll check the background-image code, it's likely a bug in RenderBoxModelObject calculateFillTileSize() & friends. (In reply to comment #6) > Is this the issue that breaks the background rendering on http://americanmccarver.com, http://www.foofighters.com/us/tour, http://wiki.ffxiclopedia.org/wiki/When_Wills_Collide when zoomed? I tried zooming those with trunk and saw no issues?
Comment on attachment 136006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136006&action=review Heh, I totally missed that you already had a patch for this, nice job! I'd give r+, though still one suggestion that you might want to follow: > Source/WebCore/rendering/RenderBoxModelObject.cpp:984 > + int resolvedWidth = intrinsicWidth.isFixed() ? static_cast<int>(intrinsicWidth.value() * style()->effectiveZoom()) : 0; > + int resolvedHeight = intrinsicHeight.isFixed() ? static_cast<int>(intrinsicHeight.value() * style()->effectiveZoom()) : 0; We might want to reuse the code in CachedImage::imageSizeForRenderer() that makes sure an intrinsic size greater than 1 can never shrink to 0 because of zoom. // Don't let images that have a width/height >= 1 shrink below 1 when zoomed. bool hasWidth = imageSize.width() > 0; bool hasHeight = imageSize.height() > 0; int width = imageSize.width() * (m_image->hasRelativeWidth() ? 1.0f : multiplier); int height = imageSize.height() * (m_image->hasRelativeHeight() ? 1.0f : multiplier); if (hasWidth) width = max(1, width); if (hasHeight) height = max(1, height); return IntSize(width, height); We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think?
> We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think? Zimmerman, sorry for late response. It's a great idea. I'll work fot it soon.
(In reply to comment #9) > > We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think? > > Zimmerman, sorry for late response. It's a great idea. I'll work fot it soon. BTW, where should we put it?
(In reply to comment #10) > (In reply to comment #9) > > > We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think? > > > > Zimmerman, sorry for late response. It's a great idea. I'll work fot it soon. > > BTW, where should we put it? IntSize sounds good.
(In reply to comment #9) > > We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think? > > Zimmerman, sorry for late response. It's a great idea. I'll work fot it soon. Ah, I tried refactoring it, but it will make a patch a bit messy. How about doing another patch? What do you think?
(In reply to comment #12) > (In reply to comment #9) > > > We could generalize this into a method usable for both CachedImage and RenderBoxModelObject - what do you think? > > > > Zimmerman, sorry for late response. It's a great idea. I'll work fot it soon. > > Ah, I tried refactoring it, but it will make a patch a bit messy. How about doing another patch? What do you think? I have no problem if it gets more complex, I'd like to review it anyways :-) Just try it...
Created attachment 136377 [details] Patch
Comment on attachment 136377 [details] Patch Attachment 136377 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12381333 New failing tests: svg/as-background-image/animated-svg-as-background.html css2.1/20110323/background-intrinsic-007.htm css2.1/20110323/background-intrinsic-004.htm fast/backgrounds/animated-svg-as-mask.html css2.1/20110323/background-intrinsic-008.htm css2.1/20110323/background-intrinsic-005.htm svg/zoom/page/zoom-svg-as-background-with-relative-size-and-viewBox.html css2.1/20110323/background-intrinsic-003.htm
Created attachment 136462 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 136377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136377&action=review Patch looks better! r- because of the cr-linux EWS failures, you should investigate into them first. This code is a bit tricky :-) > Source/WebCore/ChangeLog:21 > + (WebCore::IntSize::scaleWithMinimumSize): How about 'scaleAndClampToMinimumSize' ? Sounds more descriptive IMHO. > Source/WebCore/loader/cache/CachedImage.cpp:268 > + int minWidth = imageSize.width() > 0 ? 1 : 0; > + int minHeight = imageSize.height() > 0 ? 1 : 0; I'd rather early exit and return IntSize() if either m_imageSize.width() or m_imageSize.height() is 0. if (imageSize.isEmpty()) return IntSize(); float widthScale = m_image->hasRelativeWidth() ? 1.0 : multiplier; ...
Created attachment 136647 [details] Patch
(In reply to comment #17) > (From update of attachment 136377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136377&action=review > > Patch looks better! r- because of the cr-linux EWS failures, you should investigate into them first. This code is a bit tricky :-) > > > Source/WebCore/ChangeLog:21 > > + (WebCore::IntSize::scaleWithMinimumSize): > > How about 'scaleAndClampToMinimumSize' ? Sounds more descriptive IMHO. > > > Source/WebCore/loader/cache/CachedImage.cpp:268 > > + int minWidth = imageSize.width() > 0 ? 1 : 0; > > + int minHeight = imageSize.height() > 0 ? 1 : 0; > > I'd rather early exit and return IntSize() if either m_imageSize.width() or m_imageSize.height() is 0. > if (imageSize.isEmpty()) > return IntSize(); > float widthScale = m_image->hasRelativeWidth() ? 1.0 : multiplier; > ... (In reply to comment #17) > (From update of attachment 136377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136377&action=review > > Patch looks better! r- because of the cr-linux EWS failures, you should investigate into them first. This code is a bit tricky :-) > > > Source/WebCore/ChangeLog:21 > > + (WebCore::IntSize::scaleWithMinimumSize): > > How about 'scaleAndClampToMinimumSize' ? Sounds more descriptive IMHO. > > > Source/WebCore/loader/cache/CachedImage.cpp:268 > > + int minWidth = imageSize.width() > 0 ? 1 : 0; > > + int minHeight = imageSize.height() > 0 ? 1 : 0; > > I'd rather early exit and return IntSize() if either m_imageSize.width() or m_imageSize.height() is 0. > if (imageSize.isEmpty()) > return IntSize(); > float widthScale = m_image->hasRelativeWidth() ? 1.0 : multiplier; > ... Oh, sorry. The submitted patch didn't reflect your comment. Since I don't think IntSize::scaleWithMinimumSize makes my code clean, I changed its functionality a bit... What do you think of my latest patch?
> Oh, sorry. The submitted patch didn't reflect your comment. It's just because I wrote a patch before your comment. Thanks!
Comment on attachment 136647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136647&action=review I still have a nitpick :-) > Source/WebCore/platform/graphics/IntSize.h:90 > + void scaleNonEmpty(float widthScale, float heightScale) I still dislike the name 'scaleNonEmpty', it's not obvious on first sight what it does. Its hard to come up with a better name, so I propose following: Add bool* widthIsEmpty = 0, bool* heightIsEmpty = 0, to our standard scale() function. If the passed-in booleans are non-zero, assign *widthIsEmpty = m_width <= 0, etc. Then add a new function "void clampToMinimumSize(bool widthIsEmpty, bool heightIsEmpty, IntSize minimumSize = IntSize(1, 1)". We can now use it as follows from imageSizeForRenderer: float widthScale = ..; float heightScale = ...; bool widthIsEmpty = false; bool heightIsEmpty = false; imageSize.scale(widthScale, heightScale, &widthIsEmpty, &heightIsEmpty); imageSize.clampToMinimumSize(widthIsEmpty, heightIsEmpty); What do you think of this? Its probably cleaner this way, no?
Hmm... Yeah, I also do not like scaleNonEmpty... But I dislike scale() having widthIsEmpty and heightIsEmpty... I think it's too much. But clampToMinimumSize() is a good idea, so I would like to adopt it for my patch.
Created attachment 136810 [details] Patch
Comment on attachment 136810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136810&action=review That approach is much better, r=me! Please correct one thing before landing: > Source/WebCore/platform/graphics/IntSize.h:92 > + this->scale(scale, scale); s/this->//
(In reply to comment #24) > (From update of attachment 136810 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136810&action=review > > That approach is much better, r=me! Please correct one thing before landing: > > > Source/WebCore/platform/graphics/IntSize.h:92 > > + this->scale(scale, scale); > > s/this->// Actually name conflict will happen, so we cannot remove 'this->'
Comment on attachment 136810 [details] Patch Clearing flags on attachment: 136810 Committed r114095: <http://trac.webkit.org/changeset/114095>
All reviewed patches have been landed. Closing bug.