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.
<rdar://problem/9342801>
Created attachment 256736 [details] Patch
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.
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.
Created attachment 256770 [details] Patch
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.
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!
Created attachment 256772 [details] Patch
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?
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.
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.
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.
Comment on attachment 256772 [details] Patch Clearing flags on attachment: 256772 Committed r186817: <http://trac.webkit.org/changeset/186817>
All reviewed patches have been landed. Closing bug.
Follow-up build fix in Bug 146947: Committed r186825: <http://trac.webkit.org/changeset/186825>