WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
80027
[chromium] Increase size of Combo Box Options for touch and high DPI devices
https://bugs.webkit.org/show_bug.cgi?id=80027
Summary
[chromium] Increase size of Combo Box Options for touch and high DPI devices
Tim Dresser
Reported
2012-03-01 07:30:50 PST
Combo box popups are currently unscaled by the DefaultDeviceScaleFactor, making them too small on high DPI devices. The <option> elements also need additional padding when touch is enabled, to ensure the hitboxes are large enough to touch.
Attachments
Patch
(12.23 KB, patch)
2012-03-01 08:31 PST
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(12.24 KB, patch)
2012-03-07 06:37 PST
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(12.20 KB, patch)
2012-03-09 13:46 PST
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(12.20 KB, patch)
2012-03-09 14:04 PST
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(12.19 KB, patch)
2012-03-09 14:11 PST
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(14.36 KB, patch)
2012-03-14 10:54 PDT
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Patch
(14.38 KB, patch)
2012-03-21 06:34 PDT
,
Tim Dresser
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Tim Dresser
Comment 1
2012-03-01 08:31:08 PST
Created
attachment 129710
[details]
Patch
Darin Fisher (:fishd, Google)
Comment 2
2012-03-06 14:40:45 PST
Comment on
attachment 129710
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=129710&action=review
> Source/WebCore/platform/chromium/PopupListBox.cpp:49 > +#include "Settings.h"
do you really need this include?
> Source/WebCore/platform/chromium/PopupListBox.cpp:357 > + const int& scale = m_settings.defaultDeviceScaleFactor;
why make this a reference? it seems like making a copy of the int should be sufficient unless you really need the indirection.
> Source/WebCore/platform/chromium/PopupListBox.cpp:397 > + const int& scale = m_settings.defaultDeviceScaleFactor;
ditto
> Source/WebCore/platform/chromium/PopupListBox.cpp:401 > + // Height and y will both be evenly divisible by scale.
should you perhaps assert this? it seems like rounding errors if possible would be annoying.
> Source/WebCore/platform/chromium/PopupListBox.cpp:632 > + const int& scale = m_settings.defaultDeviceScaleFactor;
ditto
> Source/WebCore/platform/chromium/PopupListBox.cpp:634 > + if (WebCore::RuntimeEnabledFeatures::touchEnabled())
you should not need the "WebCore::" prefix
> Source/WebCore/rendering/RenderMenuList.cpp:312 > + const int& scale = document()->page()->settings()->defaultDeviceScaleFactor();
no reference
> Source/WebKit/chromium/src/WebViewImpl.cpp:637 > + // That tap triggered a select popup which is the same as the one that
so we will see a flickering of the select popup? is there some way to mitigate / avoid that?
Tim Dresser
Comment 3
2012-03-07 06:37:50 PST
Created
attachment 130614
[details]
Patch
Tim Dresser
Comment 4
2012-03-07 06:39:11 PST
(In reply to
comment #2
)
> (From update of
attachment 129710
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=129710&action=review
> > > Source/WebCore/platform/chromium/PopupListBox.cpp:49 > > +#include "Settings.h" > > do you really need this include? >
Nope, removed.
> > Source/WebCore/platform/chromium/PopupListBox.cpp:357 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > why make this a reference? it seems like making a copy of the int should be sufficient unless you really need the indirection. >
Done.
> > Source/WebCore/platform/chromium/PopupListBox.cpp:397 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > ditto > > > Source/WebCore/platform/chromium/PopupListBox.cpp:401 > > + // Height and y will both be evenly divisible by scale. > > should you perhaps assert this? it seems like rounding errors if possible would be annoying. >
Done.
> > Source/WebCore/platform/chromium/PopupListBox.cpp:632 > > + const int& scale = m_settings.defaultDeviceScaleFactor; > > ditto > > > Source/WebCore/platform/chromium/PopupListBox.cpp:634 > > + if (WebCore::RuntimeEnabledFeatures::touchEnabled()) > > you should not need the "WebCore::" prefix >
Done.
> > Source/WebCore/rendering/RenderMenuList.cpp:312 > > + const int& scale = document()->page()->settings()->defaultDeviceScaleFactor(); > > no reference > > > Source/WebKit/chromium/src/WebViewImpl.cpp:637 > > + // That tap triggered a select popup which is the same as the one that > > so we will see a flickering of the select popup? is there some way to mitigate / avoid that?
This code is parallel to Source/WebCore/rendering/RenderMenuList.cpp:516. The popup is never shown, as it is hidden before a render occurs.
Darin Fisher (:fishd, Google)
Comment 5
2012-03-09 11:49:09 PST
Comment on
attachment 130614
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=130614&action=review
Switch the 'const int scale' to just 'int scale', and then R=me
> Source/WebCore/platform/chromium/PopupListBox.cpp:356 > + const int scale = m_settings.defaultDeviceScaleFactor;
nit: no need for 'const' in front of int here. it is unnecessary verbosity. we usually only write 'const int' when giving a name to an int literal.
Tim Dresser
Comment 6
2012-03-09 13:46:16 PST
Created
attachment 131093
[details]
Patch
Tim Dresser
Comment 7
2012-03-09 14:04:42 PST
Created
attachment 131103
[details]
Patch
Tim Dresser
Comment 8
2012-03-09 14:11:45 PST
Created
attachment 131104
[details]
Patch
WebKit Review Bot
Comment 9
2012-03-09 18:40:41 PST
Comment on
attachment 131104
[details]
Patch Clearing flags on attachment: 131104 Committed
r110359
: <
http://trac.webkit.org/changeset/110359
>
Tim Dresser
Comment 10
2012-03-14 10:54:08 PDT
Created
attachment 131882
[details]
Patch
Tim Dresser
Comment 11
2012-03-14 10:57:03 PDT
(In reply to
comment #10
)
> Created an attachment (id=131882) [details] > Patch
The most recent patch ensures that RuntimeEnabledFeatures::touch_enabled() is false for the duration of SelectPopupMenuTest. This fixes the unit test failure in SelectPopupMenuTest.ClickItem.
WebKit Review Bot
Comment 12
2012-03-20 13:24:49 PDT
Comment on
attachment 131882
[details]
Patch Rejecting
attachment 131882
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: g file Source/WebCore/rendering/RenderMenuList.cpp patching file Source/WebKit/chromium/src/WebViewImpl.cpp Hunk #1 FAILED at 602. Hunk #2 succeeded at 2753 (offset 62 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej patching file Source/WebKit/chromium/tests/PopupMenuTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Darin Fish..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output:
http://queues.webkit.org/results/12000453
Tim Dresser
Comment 13
2012-03-21 06:34:31 PDT
Created
attachment 133034
[details]
Patch
WebKit Review Bot
Comment 14
2012-03-21 07:20:05 PDT
Comment on
attachment 133034
[details]
Patch Clearing flags on attachment: 133034 Committed
r111539
: <
http://trac.webkit.org/changeset/111539
>
WebKit Review Bot
Comment 15
2012-03-21 07:20:10 PDT
All reviewed patches have been landed. Closing bug.
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