Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(501)

Issue 2560733002: SELECT element: Fix a bug that intrinsic width is too narrow in less-than-100% zoom level. (Closed)

Created:
4 years ago by tkent
Modified:
4 years ago
Reviewers:
keishi
CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, mac-reviews_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

SELECT element: Fix a bug that intrinsic width is too narrow in less-than-100% zoom level. This is a regression by crbug.com/432795. Scrollbar thickness is fixed regardless of zoom level. So popup width was too narrow in less-than-100% zoom level. With this CL, popupInternalPaddingEnd() returns zoomed value as ever if zoom level is 100%+, and returns a value based on actual scrollbar thickness otherwise. Also, popupIntenalPaddingEnd() respects to the actual scrollbar thickness instead of returning fixed '18' pixel. The default scrollbar thickness on Windows is 17. popupInternalPaddingEnd() returns 1 + <scrollbar thickness>. We need to update ThemePainterDefault::setupMenuListArrow() so that it can support variable width of scrollbars. Summary of behavior changes: All platforms except Mac: menulist box is wider in less-than-100% zoom level. Windows Aura theme: menulist box is not changed in 100%+ zoom level. Non-Windows Aura theme and Mock theme: menulist box is narrower by 2px because scrollbar thickness is 15px. BUG=667236 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0151427d8ecd6ff854cffb3b741673ab043f7bd0 Cr-Commit-Position: refs/heads/master@{#437864}

Patch Set 1 #

Patch Set 2 : PartScrollbarVerticalTrack -> PartScrollbarDownArrow to fix Android crash #

Unified diffs Side-by-side diffs Delta from patch set Stats (+214 lines, -89 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 chunks +104 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/accessibility/disabled-controls-not-focusable.html View 1 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/canvas/fallback-content.html View 1 3 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/forms/form-radio-img-node-list.html View 3 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/forms/form-radio-img-node-list-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/forms/select/menulist-appearance-basic.html View 1 chunk +3 lines, -1 line 0 comments Download
D third_party/WebKit/LayoutTests/platform/linux/fast/forms/form-radio-img-node-list-expected.txt View 1 chunk +0 lines, -23 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/forms/form-radio-img-node-list-expected.txt View 1 chunk +0 lines, -23 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/forms/select/menulist-appearance-basic-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/forms/select/menulist-appearance-basic-expected.txt View 1 chunk +7 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/forms/select/menulist-appearance-rtl-expected.png View Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/forms/select/menulist-appearance-rtl-expected.txt View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMenuList.cpp View 2 chunks +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTheme.h View 2 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutThemeDefault.h View 1 chunk +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutThemeDefault.cpp View 1 5 chunks +34 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutThemeMac.h View 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutThemeMac.mm View 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/ThemePainterDefault.h View 2 chunks +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/ThemePainterDefault.cpp View 4 chunks +26 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/Scrollbar.h View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scroll/ScrollbarTheme.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 63 (51 generated)
tkent
keishi@, would you review this please?
4 years ago (2016-12-08 04:00:55 UTC) #21
keishi
LGTM
4 years ago (2016-12-09 03:39:35 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2560733002/40001
4 years ago (2016-12-09 05:14:10 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/195703)
4 years ago (2016-12-09 07:06:40 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2560733002/40001
4 years ago (2016-12-09 07:07:39 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/195765)
4 years ago (2016-12-09 08:35:22 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2560733002/40001
4 years ago (2016-12-09 08:36:12 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2560733002/100001
4 years ago (2016-12-12 10:18:13 UTC) #54
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/197009)
4 years ago (2016-12-12 12:20:28 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2560733002/100001
4 years ago (2016-12-12 12:29:37 UTC) #58
commit-bot: I haz the power
Committed patchset #2 (id:100001)
4 years ago (2016-12-12 14:08:47 UTC) #61
commit-bot: I haz the power
4 years ago (2016-12-12 15:11:59 UTC) #63
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/0151427d8ecd6ff854cffb3b741673ab043f7bd0
Cr-Commit-Position: refs/heads/master@{#437864}

Powered by Google App Engine
This is Rietveld 408576698