RESOLVED FIXED 146916
Input buttons rendered at the wrong size when pinching-to-zoom
https://bugs.webkit.org/show_bug.cgi?id=146916
Summary Input buttons rendered at the wrong size when pinching-to-zoom
Wenson Hsieh
Reported 2015-07-13 14:34:06 PDT
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.
Attachments
Patch (9.42 KB, patch)
2015-07-13 15:47 PDT, Wenson Hsieh
no flags
Patch (10.66 KB, patch)
2015-07-14 08:35 PDT, Wenson Hsieh
no flags
Patch (10.05 KB, patch)
2015-07-14 09:21 PDT, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 2015-07-13 14:34:58 PDT
Wenson Hsieh
Comment 2 2015-07-13 15:47:42 PDT
Simon Fraser (smfr)
Comment 3 2015-07-13 15:49:52 PDT
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 4 2015-07-13 15:54:09 PDT
Comment on attachment 256736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256736&action=review > Source/WebCore/ChangeLog:1 > +2015-07-13 Wenson Hsieh <whsieh@berkeley.edu> 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
Comment 5 2015-07-14 08:35:39 PDT
Simon Fraser (smfr)
Comment 6 2015-07-14 08:47:05 PDT
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 7 2015-07-14 09:16:43 PDT
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
Comment 8 2015-07-14 09:21:17 PDT
Tim Horton
Comment 9 2015-07-14 11:39:20 PDT
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 10 2015-07-14 12:09:25 PDT
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 11 2015-07-14 12:12:34 PDT
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 12 2015-07-14 12:25:34 PDT
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 13 2015-07-14 14:40:46 PDT
Comment on attachment 256772 [details] Patch Clearing flags on attachment: 256772 Committed r186817: <http://trac.webkit.org/changeset/186817>
WebKit Commit Bot
Comment 14 2015-07-14 14:40:51 PDT
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 15 2015-07-15 07:11:46 PDT
Follow-up build fix in Bug 146947: Committed r186825: <http://trac.webkit.org/changeset/186825>
Note You need to log in before you can comment on or make changes to this bug.