Background width (or height) is wrong if width (or height) * zoom < 1

• Component: CSS
• Status: RESOLVED FIXED

[Shinya Kawanaka]

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

[Shinya Kawanaka]

Created attachment 135984 [details]
Repro

[Shinya Kawanaka]

Created attachment 135986 [details]
Repro

[Shinya Kawanaka]

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

[Shinya Kawanaka]

Created attachment 136006 [details]
Patch

[Shinya Kawanaka]

Oh, when I tried to add zimmerman in the cc list, he was already added.

[Simon Fraser (smfr)]

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?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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?

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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

[Nikolas Zimmermann]

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

[Shinya Kawanaka]

Created attachment 136377 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nikolas Zimmermann]

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

[Shinya Kawanaka]

Created attachment 136647 [details]
Patch

[Shinya Kawanaka]

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

[Shinya Kawanaka]

> Oh, sorry. The submitted patch didn't reflect your comment. 

It's just because I wrote a patch before your comment. Thanks!

[Nikolas Zimmermann]

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?

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

Created attachment 136810 [details]
Patch

[Nikolas Zimmermann]

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->//

[Shinya Kawanaka]

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

[WebKit Review Bot]

Comment on attachment 136810 [details]
Patch

Clearing flags on attachment: 136810

Committed r114095: <http://trac.webkit.org/changeset/114095>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.