WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(10.66 KB, patch)
2015-07-14 08:35 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(10.05 KB, patch)
2015-07-14 09:21 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2015-07-13 14:34:58 PDT
<
rdar://problem/9342801
>
Wenson Hsieh
Comment 2
2015-07-13 15:47:42 PDT
Created
attachment 256736
[details]
Patch
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
Created
attachment 256770
[details]
Patch
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
Created
attachment 256772
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug