Bug 146916 – Input buttons rendered at the wrong size when pinching-to-zoom

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

When zooming in with the trackpad, the button image remains at a constant height to prevent stretching upon zoom. While this was originally intended, it has been agreed that a better behavior would be to scale the button up as the user zooms.

[Wenson Hsieh]

<rdar://problem/9342801>

[Wenson Hsieh]

Created attachment 256736 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 256736 [details]
Patch

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

> Source/WebCore/platform/mac/ThemeMac.mm:573
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::createCompatibleBuffer(inflatedRect.size(), deviceScaleFactor, ColorSpaceDeviceRGB, context, false);
> +    LocalCurrentGraphicsContext localContext(imageBuffer->context());

I think we should only use this intermedia buffer if the page scale is != 1.

[Wenson Hsieh]

Comment on attachment 256736 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Wenson Hsieh  <[e-mail]>

Hm...I really have to figure out why it still defaults to this email

>> Source/WebCore/platform/mac/ThemeMac.mm:573
>> +    LocalCurrentGraphicsContext localContext(imageBuffer->context());
> 
> I think we should only use this intermedia buffer if the page scale is != 1.

Good idea! I'll do that.

[Wenson Hsieh]

Created attachment 256770 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 256770 [details]
Patch

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

> Source/WebCore/platform/mac/ThemeMac.mm:574
> +    std::unique_ptr<ImageBuffer> imageBuffer = nullptr;

No need to initialize with nullptr.

> Source/WebCore/platform/mac/ThemeMac.mm:603
> +            std::unique_ptr<ImageBuffer> focusRectImageBuffer = ImageBuffer::createCompatibleBuffer(inflatedRect.size() + 2 * FloatSize(focusWidth, focusWidth), deviceScaleFactor, ColorSpaceDeviceRGB, context, false);

Can we just draw the buttonCell and the focus ring into the same buffer? Just need to make a larger buffer up front.

[Wenson Hsieh]

Comment on attachment 256770 [details]
Patch

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

>> Source/WebCore/platform/mac/ThemeMac.mm:574
>> +    std::unique_ptr<ImageBuffer> imageBuffer = nullptr;
> 
> No need to initialize with nullptr.

Got it. Fixed!

>> Source/WebCore/platform/mac/ThemeMac.mm:603
>> +            std::unique_ptr<ImageBuffer> focusRectImageBuffer = ImageBuffer::createCompatibleBuffer(inflatedRect.size() + 2 * FloatSize(focusWidth, focusWidth), deviceScaleFactor, ColorSpaceDeviceRGB, context, false);
> 
> Can we just draw the buttonCell and the focus ring into the same buffer? Just need to make a larger buffer up front.

Sounds good. Done!

[Wenson Hsieh]

Created attachment 256772 [details]
Patch

[Tim Horton]

Comment on attachment 256772 [details]
Patch

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

> Source/WebCore/platform/mac/ThemeMac.mm:602
> +    if (states & ControlStates::FocusState) {

At some point, we should probably not paint the focus ring into the ImageBuffer, and instead paint it straight into the page but inflated to the scaled size of the button, so that it is the right thickness. Maybe leave a FIXME?

[Dean Jackson]

Comment on attachment 256772 [details]
Patch

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

> Source/WebCore/platform/mac/ThemeMac.mm:610
> +        if (shouldUseImageBuffer) {
> +            drawCellFocusRing(buttonCell, CGRectMake(focusThickness, focusThickness, inflatedRect.width(), inflatedRect.height()), view);
> +            context->drawImageBuffer(imageBuffer.get(), ColorSpaceDeviceRGB, inflatedRect.location() - FloatSize(focusThickness, focusThickness));
> +        } else
> +            drawCellFocusRing(buttonCell, inflatedRect, view);
> +    }
> +    
> +    controlStates->setNeedsRepaint(false);

While we disabled animating the focus ring during the last cycle, I think we should at least keep this part of the logic in place. i.e. that drawCellFocusRing returns a boolean that we use to set needsRepaint.

[Dean Jackson]

Comment on attachment 256772 [details]
Patch

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

> Source/WebCore/platform/mac/ThemeMac.mm:540
> +static void paintButton(ControlPart part, ControlStates* controlStates, GraphicsContext* context, const FloatRect& zoomedRect, float zoomFactor, ScrollView* scrollView, float deviceScaleFactor, float pageScaleFactor)
>  {

Now that we have the scale factors, we should use them to pick the most appropriate control size. i.e. if we have a small control and are really zoomed in, it doesn't make sense to draw a small bitmap in a huge area, when we can get the bigger bitmap by asking for a larger control.

[Dean Jackson]

Comment on attachment 256772 [details]
Patch

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

>> Source/WebCore/platform/mac/ThemeMac.mm:540
>>  {
> 
> Now that we have the scale factors, we should use them to pick the most appropriate control size. i.e. if we have a small control and are really zoomed in, it doesn't make sense to draw a small bitmap in a huge area, when we can get the bigger bitmap by asking for a larger control.

After some discussion on IRC, we'll move this to a followup patch (maybe next cycle). The issue is that we need to know the metrics of all the controls in order to draw the bigger control at the size of the smaller controls.

[WebKit Commit Bot]

Comment on attachment 256772 [details]
Patch

Clearing flags on attachment: 256772

Committed r186817: <http://trac.webkit.org/changeset/186817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Follow-up build fix in Bug 146947:

Committed r186825: <http://trac.webkit.org/changeset/186825>