Bug 87774 – REGRESSION (r105515): reflection masks are truncated at zoom levels < 1

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

r105515 broke reflections when using text zoom to zoom out to a level < 1. The reflection mask is truncated.

[Simon Fraser (smfr)]

Created attachment 144612 [details]
Testcase

[Simon Fraser (smfr)]

<rdar://problem/11387506>

[Simon Fraser (smfr)]

Niko: ping

[Beth Dakin]

It seems like RenderBoxModelObject::calculateImageIntrinsicDimensions() does not always scale by the effectiveZoom so it doesn't make sense to always divide the zoom out. That's what happening in this test case. 

That is what is happening for the attached reduction. We return right away without scaling the dimensions:

IntSize RenderBoxModelObject::calculateImageIntrinsicDimensions(StyleImage* image, const IntSize& positioningAreaSize) const
{
    // A generated image without a fixed size, will always return the container size as intrinsic size.
    if (image->isGeneratedImage() && image->usesImageContainerSize())
        return IntSize(positioningAreaSize.width(), positioningAreaSize.height());

…

But there are other cases that don't scale too.

[Beth Dakin]

Created attachment 145361 [details]
Patch

Here's one way to fix this.

[Tim Horton]

Comment on attachment 145361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145361&action=review

> Source/WebCore/ChangeLog:17
> +        * rendering/RenderBoxModelObject.cpp:

Additional newline here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1034
> +    IntSize imageIntrinsicSize = calculateImageIntrinsicDimensions(image, positioningAreaSize, true);

Maybe an enum instead of an incomprehensible bool argument?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1251
> -    int imageWidth = imageSize.width() / style->effectiveZoom();
> -    int imageHeight = imageSize.height() / style->effectiveZoom();
> +    int imageWidth = imageSize.width();
> +    int imageHeight = imageSize.height();

Have you quadruple checked that this doesn't break the apple.com top bar again (a la https://bugs.webkit.org/show_bug.cgi?id=76249)?

> LayoutTests/ChangeLog:5
> +        -and corresponding-

I don't know that the -and corresponding- is necessary, but OK :D

[Simon Fraser (smfr)]

Comment on attachment 145361 [details]
Patch

r+ but I agree with Tim!

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 145361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145361&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        * rendering/RenderBoxModelObject.cpp:
> 
> Additional newline here.
> 

Added.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1034
> > +    IntSize imageIntrinsicSize = calculateImageIntrinsicDimensions(image, positioningAreaSize, true);
> 
> Maybe an enum instead of an incomprehensible bool argument?
> 

I knew someone was going to suggest this. And I only didn't do it that way from the start because I suck so much at naming things. How about:

enum ShouldScaleByEffectiveZoom { ShouldScale, ShouldNotScale }

Thoughts? Suggestions?

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1251
> > -    int imageWidth = imageSize.width() / style->effectiveZoom();
> > -    int imageHeight = imageSize.height() / style->effectiveZoom();
> > +    int imageWidth = imageSize.width();
> > +    int imageHeight = imageSize.height();
> 
> Have you quadruple checked that this doesn't break the apple.com top bar again (a la https://bugs.webkit.org/show_bug.cgi?id=76249)?
> 

I have! I ran lots of layout tests and pixel tests too.

> > LayoutTests/ChangeLog:5
> > +        -and corresponding-
> 
> I don't know that the -and corresponding- is necessary, but OK :D

It's my thang! Just ask mitzpettel. :-P

[Beth Dakin]

Fixed: http://trac.webkit.org/changeset/119274

[Nikolas Zimmermann]

(In reply to comment #3)
> Niko: ping
Sorry for the late answer - I was busy with internal work this week.
Beth, thanks a bunch for looking into this!

[Beth Dakin]

(In reply to comment #10)
> (In reply to comment #3)
> > Niko: ping
> Sorry for the late answer - I was busy with internal work this week.
> Beth, thanks a bunch for looking into this!

No worries, I understand. 

And, you're welcome! :-)