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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 736883002: Implement <select> Popup Menu using PagePopup (Closed)

Created:
6 years, 1 month ago by keishi
Modified:
5 years, 10 months ago
Reviewers:
tkent
CC:
blink-reviews, tyoshino+watch_chromium.org, blink-reviews-rendering, zoltan1, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, gavinp+loader_chromium.org, jchaffraix+rendering, Nate Chapin
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Implement <select> Popup Menu using PagePopup PopupMenuImpl provides the content for the page popup. ListPicker is the JavaScript implementation for the popup menu. BUG=346582 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189304 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189581

Patch Set 1 : #

Total comments: 4

Patch Set 2 : #

Patch Set 3 : #

Total comments: 26

Patch Set 4 : #

Total comments: 16

Patch Set 5 : #

Total comments: 20

Patch Set 6 : #

Patch Set 7 : rebased #

Patch Set 8 : #

Patch Set 9 : Rebased #

Patch Set 10 : Fixed return value for fontWeightToString #

Patch Set 11 : #

Patch Set 12 : fixed update-widget-positions-on-nested-frames-and-scrollers #

Patch Set 13 : fixed flaky test #

Patch Set 14 : #

Patch Set 15 : Fixed tests for mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1334 lines, -31 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 11 13 14 3 chunks +17 lines, -2 lines 0 comments Download
M LayoutTests/compositing/overflow/resources/update-widget-positions-on-nested-frames-and-scrollers-inner-frame.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +9 lines, -8 lines 0 comments Download
M LayoutTests/fast/forms/resources/picker-common.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +48 lines, -18 lines 0 comments Download
M LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html View 1 2 3 4 4 chunks +13 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-appearance.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +27 lines, -0 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-empty.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -6 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-empty-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-appearance-many.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +29 lines, -0 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-many-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-appearance-single-option.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +33 lines, -0 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-single-option-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-appearance-styled.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +40 lines, -0 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-styled-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-appearance-transform.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +23 lines, -0 lines 0 comments Download
A + LayoutTests/fast/forms/select/popup-menu-appearance-transform-expected.txt View 1 2 3 4 13 14 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-ax.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-key-operations.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +135 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-mouse-operations.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +45 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/select/popup-menu-position.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +184 lines, -0 lines 0 comments Download
A ManualTests/forms/list-picker.html View 1 1 chunk +225 lines, -0 lines 0 comments Download
M Source/core/css/CSSFontSelector.h View 2 chunks +4 lines, -2 lines 0 comments Download
M Source/core/dom/StyleEngine.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/dom/StyleEngine.cpp View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/html/HTMLOptionElement.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/html/HTMLOptionElement.cpp View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/html/HTMLSelectElement.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
A + Source/core/html/forms/PopupMenuClient.h View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/page/PagePopup.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/page/PagePopupClient.h View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderMenuList.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderMenuList.cpp View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -0 lines 0 comments Download
M Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 13 14 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/ChromeClientImpl.cpp View 1 2 3 4 5 6 7 8 13 14 2 chunks +4 lines, -0 lines 0 comments Download
M Source/web/ColorChooserPopupUIController.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/DateTimeChooserImpl.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/ExternalPopupMenuTest.cpp View 1 2 3 chunks +9 lines, -0 lines 0 comments Download
A Source/web/PopupMenuImpl.h View 1 2 3 4 1 chunk +64 lines, -0 lines 0 comments Download
A Source/web/PopupMenuImpl.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +309 lines, -0 lines 0 comments Download
M Source/web/PopupMenuTest.cpp View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M Source/web/WebPagePopupImpl.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/WebPagePopupImpl.cpp View 1 2 3 4 5 6 7 8 3 chunks +19 lines, -0 lines 0 comments Download
M Source/web/web.gypi View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 79 (43 generated)
keishi
I still need to add tooltip support to PagePopup but could you start taking a ...
6 years ago (2014-12-12 10:41:39 UTC) #2
tkent
General comments: - Please remove Patch Set 1-17 from this codereview. - We should have ...
6 years ago (2014-12-15 02:07:00 UTC) #3
tkent
https://codereview.chromium.org/736883002/diff/340001/Source/platform/PopupMenuClient.h File Source/platform/PopupMenuClient.h (right): https://codereview.chromium.org/736883002/diff/340001/Source/platform/PopupMenuClient.h#newcode58 Source/platform/PopupMenuClient.h:58: virtual Element& ownerElement() const = 0; Layering violation. platform/* ...
6 years ago (2014-12-15 02:10:52 UTC) #21
keishi
Added tooltip support. > General comments: > - Please remove Patch Set 1-17 from this ...
6 years ago (2014-12-15 08:29:42 UTC) #22
tkent
https://codereview.chromium.org/736883002/diff/380001/Source/core/html/HTMLOptionElement.cpp File Source/core/html/HTMLOptionElement.cpp (right): https://codereview.chromium.org/736883002/diff/380001/Source/core/html/HTMLOptionElement.cpp#newcode177 Source/core/html/HTMLOptionElement.cpp:177: HTMLSelectElement* selectElement = ownerSelectElement(); if (HTMLSelectElement* selectElement = ownerSelectElement()) ...
6 years ago (2014-12-15 09:26:05 UTC) #23
keishi
https://codereview.chromium.org/736883002/diff/380001/Source/core/html/HTMLOptionElement.cpp File Source/core/html/HTMLOptionElement.cpp (right): https://codereview.chromium.org/736883002/diff/380001/Source/core/html/HTMLOptionElement.cpp#newcode177 Source/core/html/HTMLOptionElement.cpp:177: HTMLSelectElement* selectElement = ownerSelectElement(); On 2014/12/15 09:26:04, unavailable until ...
6 years ago (2014-12-16 03:53:25 UTC) #24
tkent
https://codereview.chromium.org/736883002/diff/400001/Source/core/dom/StyleEngine.h File Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/736883002/diff/400001/Source/core/dom/StyleEngine.h#newcode155 Source/core/dom/StyleEngine.h:155: void setFontSelector(PassRefPtrWillBeRawPtr<CSSFontSelector> fontSelector) { m_fontSelector = fontSelector; } nit: ...
6 years ago (2014-12-16 05:42:39 UTC) #25
tkent
LayoutTests/fast/forms/select/resources/popup-menu-common.js is empty. I'd like to look at PNG results of the appearance tests.
6 years ago (2014-12-16 05:45:30 UTC) #26
keishi
Looks like in the current implementation, when any key is pressed while the popup is ...
6 years ago (2014-12-16 12:21:03 UTC) #28
tkent
lgtm. Does Mac Content Shell use no external popup menu? the PNG results look like ...
6 years ago (2014-12-17 01:06:21 UTC) #29
keishi
Mac uses external popup even for content shell. I forced it on. The focus ring ...
6 years ago (2014-12-22 05:03:54 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/440001
6 years ago (2014-12-22 05:04:29 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: android_blink_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_rel/builds/23170) android_chromium_gn_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_compile_rel/builds/21105) blink_presubmit ...
6 years ago (2014-12-22 05:08:19 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/480001
5 years, 11 months ago (2015-01-12 10:49:29 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/41755)
5 years, 11 months ago (2015-01-12 11:10:09 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/500001
5 years, 11 months ago (2015-01-12 15:15:26 UTC) #40
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/builds/32384)
5 years, 11 months ago (2015-01-12 15:39:23 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/500001
5 years, 11 months ago (2015-01-13 05:55:42 UTC) #44
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/builds/32451)
5 years, 11 months ago (2015-01-13 06:26:15 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/520001
5 years, 10 months ago (2015-01-29 17:25:06 UTC) #48
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/48355)
5 years, 10 months ago (2015-01-29 17:47:48 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/540001
5 years, 10 months ago (2015-02-01 02:41:22 UTC) #52
commit-bot: I haz the power
Committed patchset #10 (id:540001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189304
5 years, 10 months ago (2015-02-01 03:59:42 UTC) #53
vsevik
This patch breaks layout test and was reverted, see https://codereview.chromium.org/892083003/
5 years, 10 months ago (2015-02-02 10:58:01 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/620001
5 years, 10 months ago (2015-02-03 04:53:14 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/48934)
5 years, 10 months ago (2015-02-03 07:18:03 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/620001
5 years, 10 months ago (2015-02-03 13:42:06 UTC) #60
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41034)
5 years, 10 months ago (2015-02-03 16:08:00 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/620001
5 years, 10 months ago (2015-02-04 08:23:24 UTC) #64
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41291)
5 years, 10 months ago (2015-02-04 10:46:20 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/620001
5 years, 10 months ago (2015-02-04 11:10:10 UTC) #68
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41345)
5 years, 10 months ago (2015-02-04 13:39:40 UTC) #70
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/620001
5 years, 10 months ago (2015-02-04 18:20:26 UTC) #72
commit-bot: I haz the power
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/41417)
5 years, 10 months ago (2015-02-04 20:55:22 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/736883002/640001
5 years, 10 months ago (2015-02-05 16:35:47 UTC) #78
commit-bot: I haz the power
5 years, 10 months ago (2015-02-05 19:05:28 UTC) #79
Message was sent while issue was closed. 
Committed patchset #15 (id:640001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189581

Powered by Google App Engine
This is Rietveld 408576698