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.
Created attachment 129710 [details] Patch
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?
Created attachment 130614 [details] Patch
(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.
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.
Created attachment 131093 [details] Patch
Created attachment 131103 [details] Patch
Created attachment 131104 [details] Patch
Comment on attachment 131104 [details] Patch Clearing flags on attachment: 131104 Committed r110359: <http://trac.webkit.org/changeset/110359>
Created attachment 131882 [details] Patch
(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.
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
Created attachment 133034 [details] Patch
Comment on attachment 133034 [details] Patch Clearing flags on attachment: 133034 Committed r111539: <http://trac.webkit.org/changeset/111539>
All reviewed patches have been landed. Closing bug.