Bug 173135 – [Win] Play button on movie trailers page is cropped with device scale factor 1.25.

• Component: WebCore Misc.
• Status: NEW

[Per Arne Vollan]

1) Visit trailers.apple.com.
2) Choose one of the available trailers.
3) Observe how the play button is cropped when the device scale factor is 1.25.

[Per Arne Vollan]

Created attachment 312393 [details]
Image showing cropped play button.

[Per Arne Vollan]

Created attachment 312395 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #2)
> Created attachment 312395 [details]
> Patch

This patch fixes this specific bug, but I don't think it is the correct fix, so input is highly appreciated :)

[Per Arne Vollan]

<rdar://problem/32547940>

[Brent Fulgham]

Comment on attachment 312395 [details]
Patch

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

> Source/WebCore/platform/graphics/LayoutRect.h:229
> +    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), FloatPoint(roundToDevicePixel(rect.maxX(), pixelSnappingFactor), roundToDevicePixel(rect.maxY(), pixelSnappingFactor)));

We need Zalan to look at this!

[alan]

Comment on attachment 312395 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Instead of using the function snapSizeToDevicePixel to compute the bottom, right corder of the
> +        snapped rectangle, we can use the function roundToDevicePixel.

In the ChangeLog entry you should explain the "why" and not the "what".

> Source/WebCore/ChangeLog:12
> +        Covered by existing tests. 

It is clearly a behavior change and unless you unksip some test, I don't think the 'covered by existing tests' is correct. Please add a test case for this progression.

>> Source/WebCore/platform/graphics/LayoutRect.h:229
>> -    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), snapSizeToDevicePixel(rect.size(), rect.location(), pixelSnappingFactor));
>> +    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), FloatPoint(roundToDevicePixel(rect.maxX(), pixelSnappingFactor), roundToDevicePixel(rect.maxY(), pixelSnappingFactor)));
> 
> We need Zalan to look at this!

I need to do a bit more digging to figure out if it is correct. in the meantime, for the reasons above -> r-.

[Per Arne Vollan]

(In reply to zalan from comment #6)
> Comment on attachment 312395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312395&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Instead of using the function snapSizeToDevicePixel to compute the bottom, right corder of the
> > +        snapped rectangle, we can use the function roundToDevicePixel.
> 
> In the ChangeLog entry you should explain the "why" and not the "what".
> 
> > Source/WebCore/ChangeLog:12
> > +        Covered by existing tests. 
> 
> It is clearly a behavior change and unless you unksip some test, I don't
> think the 'covered by existing tests' is correct. Please add a test case for
> this progression.
> 
> >> Source/WebCore/platform/graphics/LayoutRect.h:229
> >> -    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), snapSizeToDevicePixel(rect.size(), rect.location(), pixelSnappingFactor));
> >> +    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), FloatPoint(roundToDevicePixel(rect.maxX(), pixelSnappingFactor), roundToDevicePixel(rect.maxY(), pixelSnappingFactor)));
> > 
> > We need Zalan to look at this!
> 
> I need to do a bit more digging to figure out if it is correct. in the
> meantime, for the reasons above -> r-.

Thanks for reviewing! I forgot to mention that this happens on Windows, although it might be a problem on macOS/iOS as well if the scale factor is 1.25x.

[alan]

(In reply to Per Arne Vollan from comment #7)
> (In reply to zalan from comment #6)
> > Comment on attachment 312395 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=312395&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        Instead of using the function snapSizeToDevicePixel to compute the bottom, right corder of the
> > > +        snapped rectangle, we can use the function roundToDevicePixel.
> > 
> > In the ChangeLog entry you should explain the "why" and not the "what".
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        Covered by existing tests. 
> > 
> > It is clearly a behavior change and unless you unksip some test, I don't
> > think the 'covered by existing tests' is correct. Please add a test case for
> > this progression.
> > 
> > >> Source/WebCore/platform/graphics/LayoutRect.h:229
> > >> -    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), snapSizeToDevicePixel(rect.size(), rect.location(), pixelSnappingFactor));
> > >> +    return FloatRect(FloatPoint(roundToDevicePixel(rect.x(), pixelSnappingFactor), roundToDevicePixel(rect.y(), pixelSnappingFactor)), FloatPoint(roundToDevicePixel(rect.maxX(), pixelSnappingFactor), roundToDevicePixel(rect.maxY(), pixelSnappingFactor)));
> > > 
> > > We need Zalan to look at this!
> > 
> > I need to do a bit more digging to figure out if it is correct. in the
> > meantime, for the reasons above -> r-.
> 
> Thanks for reviewing! I forgot to mention that this happens on Windows,
> although it might be a problem on macOS/iOS as well if the scale factor is
> 1.25x.
We surely have issues with non-integral zoom factors on all platforms.