Issue 368023003: Popup open and hide using Alt+KeyDown

Issue 368023003: Popup open and hide using Alt+KeyDown (Closed)

Created:
6 years, 5 months ago by Habib Virji

Modified:
6 years, 5 months ago

Reviewers:
keishi, tkent

CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Popup open and hide using Alt+KeyDown HTMLSelect to open popup when key combination alt+keydown. It does not select any element, just opens or hide popup. Once popup is open, default handler of the popup handles key event. For that reason hide popup is handled in PopupListBox. This behavior is compatible with firefox behavior. R=tkent, keishi BUG=383818 TEST=On Alt+keydown open popup and check if visible. On again press of Alt+keydown, close popup. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177834

Patch Set 1 #

Total comments: 10

Patch Set 2 : #

Total comments: 6

Patch Set 3 : Updated as per code review comments #

Patch Set 4 : Fixed mac tests #

Total comments: 9

Patch Set 5 : Structuring of code #

Total comments: 1

Patch Set 6 : Another patch for mac test fix #

Total comments: 1

Patch Set 7 : Review comments #

Patch Set 8 : "Build fix" #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+189 lines, -139 lines)			Patch
A	LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html	View	1 2 3	1 chunk	+66 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard-expected.txt	View	1 2	1 chunk	+26 lines, -0 lines	0 comments	Download
A	LayoutTests/platform/mac/fast/forms/select/menulist-popup-open-hide-using-keyboard-expected.txt	View	1 2 3	1 chunk	+26 lines, -0 lines	0 comments	Download
M	Source/core/core.gypi	View	1	1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/html/HTMLSelectElement.h	View	1 2 3 4	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/html/HTMLSelectElement.cpp	View	1 2 3 4 5 6	4 chunks	+56 lines, -74 lines	0 comments	Download
D	Source/core/html/HTMLSelectElementWin.cpp	View	1	1 chunk	+0 lines, -61 lines	0 comments	Download
M	Source/core/rendering/RenderTheme.h	View	1 2	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderThemeChromiumDefault.h	View	1 2	1 chunk	+3 lines, -1 line	0 comments	Download
M	Source/core/rendering/RenderThemeChromiumMac.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/PopupListBox.cpp	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 28 (0 generated)

Expand Messages | Collapse Messages

keishi

https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html File LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html (right): https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html#newcode5 LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:5: <body> nit:extra space https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html#newcode14 LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:14: description('Test for opening select ...

6 years, 5 months ago (2014-07-03 07:13:38 UTC) #1

Habib Virji

Thanks keishi, will do the changes. Can you please clarify below quest as alt+down key ...

6 years, 5 months ago (2014-07-03 08:17:34 UTC) #2

keishi

https://codereview.chromium.org/368023003/diff/1/Source/core/html/HTMLSelectElement.cpp File Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/368023003/diff/1/Source/core/html/HTMLSelectElement.cpp#newcode1187 Source/core/html/HTMLSelectElement.cpp:1187: if (keyIdentifier == "Down" && event->isKeyboardEvent() && toKeyboardEvent(event)->altKey()) { ...

6 years, 5 months ago (2014-07-03 08:38:47 UTC) #3

Habib Virji

- RenderTheme and RenderThemeChromiumDefault includes a new function for setting alt key only for linux ...

6 years, 5 months ago (2014-07-03 17:24:04 UTC) #4

- RenderTheme and RenderThemeChromiumDefault includes a new function for setting
alt key only for linux and windows.
    
Introduced new function handlePopupOpenKeyboardEvent
- Combined various popup menu handlers in one function.
- downUpArrowKey  and spaceKey are specifically for mac. As popsMenuByArrowKey
is set only for mac.
- spaceOrEnterKey, altDownUp, f4 are for opening popup on linux and windows.
- Retained selectedIndex from HTMLSelectElementWin as it makes sense to set
selectedIndex if available.
- Removed platformKeyboardEvent as it was specific for MAC.
(popsMenuByArrowKey() is only set for mac)
- Removed HTMLSelectElementWin.cpp as now handlePopupOpenKeyboardEvent handles
for both Windows and Linux.
    
- Introduced  further tests to accumulate behavior of keyboard shortcut to open
popup.
- PopupListBox includes up arrow key to close the popup menu.

https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/selec...
File LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html
(right):

https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/selec...
LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:5:
<body>
On 2014/07/03 07:13:38, keishi wrote:
> nit:extra space

Done.

https://codereview.chromium.org/368023003/diff/1/LayoutTests/fast/forms/selec...
LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:14:
description('Test for opening select popup using alt+KeyDown and closing by
pressing alt+keyDown again</i>.');
On 2014/07/03 07:13:38, keishi wrote:
> down arrow is better. keydown is also an event name.

Done.

https://codereview.chromium.org/368023003/diff/1/Source/core/html/HTMLSelectE...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/368023003/diff/1/Source/core/html/HTMLSelectE...
Source/core/html/HTMLSelectElement.cpp:1187: if (keyIdentifier == "Down" &&
event->isKeyboardEvent() && toKeyboardEvent(event)->altKey()) {
On 2014/07/03 08:38:47, keishi wrote:
> On 2014/07/03 08:17:34, Habib Virji wrote:
> > On 2014/07/03 07:13:38, keishi wrote:
> > > We already open the popup on alt+down/up. It is implemented in
> > > platformHandleKeydownEvent in HTMLSelectElementWin.cpp.
> > > We don't want this shortcut on Mac so this is unnecessary.
> > > We just need to close the popup on alt+down/up.
> > 
> > @keishi: What about linux? It does not work for linux, alt+down. Should
ifdef
> > flag be added to make it work in Linux?
> 
> I think we want this shortcut for Windows and Linux.
> We can't share code if we use file names to specify behavior for each
> platform(e.g. *Win.cpp and *Linux.cpp)
> I think we should use RenderTheme. Please refer to
> RenderTheme::shouldOpenPickerWithF4Key

Done.

https://codereview.chromium.org/368023003/diff/1/Source/web/PopupListBox.cpp
File Source/web/PopupListBox.cpp (right):

https://codereview.chromium.org/368023003/diff/1/Source/web/PopupListBox.cpp#...
Source/web/PopupListBox.cpp:246: if (event.altKey() && event.keyIdentifier() ==
"Down")
On 2014/07/03 07:13:38, keishi wrote:
> We want to support alt+up too.

Done.

keishi

https://codereview.chromium.org/368023003/diff/20001/LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html File LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html (right): https://codereview.chromium.org/368023003/diff/20001/LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html#newcode22 LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:22: shouldBeTrue('internals.isSelectPopupVisible(popup)'); You can pass true as the second argument ...

6 years, 5 months ago (2014-07-04 01:33:50 UTC) #5

Habib Virji

Thanks keishi. Updated as suggested, sorry for delay took time to figure out why it ...

6 years, 5 months ago (2014-07-08 12:21:32 UTC) #6

Thanks keishi. Updated as suggested, sorry for delay took time to figure out why
it was failing in Mac... 

Updated code to handle all tests scenario
    
- Updated to pass spatial handling tests. Check before hand to avoid any
crashes.
- Test updated correctly for mac tests.line 1187 has early return condition
which was used in previous code to provide early return for mac scenarios. 
- Corrected enter key is handled in mac and does form submission. Previous
popsMenuByArrowKey was used to handle all mac  scenario, including space and
enter. Enter key has different handling   in mac, thus po
- Implemented code review comments.

https://codereview.chromium.org/368023003/diff/20001/LayoutTests/fast/forms/s...
File LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html
(right):

https://codereview.chromium.org/368023003/diff/20001/LayoutTests/fast/forms/s...
LayoutTests/fast/forms/select/menulist-popup-open-hide-using-keyboard.html:22:
shouldBeTrue('internals.isSelectPopupVisible(popup)');
On 2014/07/04 01:33:50, keishi wrote:
> You can pass true as the second argument to shouldBeTrue so that there won't
be
> a lot of FAILs inside the Mac expectation.

@keishi, wanted to show through how some keys work for mac and fails for
windows/linux and vice-versa. Like for linux down key will fail and that's
captured in the tests. Adding true might make these tests not covered. Please
suggest, have updated tests and tested on mac.

https://codereview.chromium.org/368023003/diff/20001/Source/core/html/HTMLSel...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/368023003/diff/20001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1126: bool spaceKey = (event->type() ==
EventTypeNames::keypress) &&  renderTheme.popsMenuByArrowKeys() && keyCode == '
';
On 2014/07/04 01:33:50, keishi wrote:
> Using RenderTheme.popsMenuByArrowKeys here seems wrong. Maybe you can split
> popsMenuBySpaceOrReturn to popsMenuBySpace and popsMenuByReturn

Done.

https://codereview.chromium.org/368023003/diff/20001/Source/core/html/HTMLSel...
Source/core/html/HTMLSelectElement.cpp:1132: if (downUpKey || spaceKey ||
spaceOrEnterKey || altDownUpKey || f4Key) {
On 2014/07/04 01:33:50, keishi wrote:
> nit: I think this is kind of confusing because spaceKey may be false even when
> space is pressed.
> Maybe we could have a single shouldOpenPopup boolean and do |=.
> 
> bool shouldOpenPopup = false;
> if (event->type() == EventTypeNames::keypress)
>   shouldOpenPopup |= renderTheme.popsMenuByArrowKeys() && keyCode == ' ';
>   ...
> } else if (event->type() == EventTypeNames::keydown)
>   ...
> }
> if (shouldOpenPopup) {
>   // open popup
> } else {
>   // Maybe you could event handle form submission and spatial navigation
> selecting in here too.
> }

Done.

keishi

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSelectElement.cpp File Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSelectElement.cpp#newcode1117 Source/core/html/HTMLSelectElement.cpp:1117: bool HTMLSelectElement::handlePopupOpenKeyboardEvent(Event* event) HTMLSelectElement::handlePopupOpenKeyboardEvent has stuff not related to ...

6 years, 5 months ago (2014-07-09 03:38:21 UTC) #7

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1117: bool
HTMLSelectElement::handlePopupOpenKeyboardEvent(Event* event)
HTMLSelectElement::handlePopupOpenKeyboardEvent has stuff not related to opening
the popup and as you mention in the comments the early return stuff is tricky.

Maybe the code will be easier to follow if most of the structure was kept in
HTMLSelectElement::menuListDefaultEventHandler.

For example:

// Return true if 
bool HTMLSelectElement::handlePopupOpenKeyboardEvent(KeyboardEvent* event)
{
    focus();
    // Calling focus() may cause us to lose our renderer. Return true so
    // that our caller doesn't process the event further, but don't set
    // the event as handled.
    if (!renderer() || !renderer()->isMenuList() || isDisabledFormControl())
        return true;
    // Save the selection so it can be compared to the new selection
    // when dispatching change events during selectOption, which
    // gets called from RenderMenuList::valueChanged, which gets called
    // after the user makes a selection from the menu.
    saveLastSelection();
    if (RenderMenuList* menuList = toRenderMenuList(renderer()))
        menuList->showPopup();
    int index = selectedIndex();
    ASSERT(index >= 0);
    ASSERT_WITH_SECURITY_IMPLICATION(index <
static_cast<int>(listItems().size()));
    setSelectedIndex(index);
    event->setDefaultHandled();
    return false;
}

bool HTMLSelectElement::shouldOpenPopupForKeyDownEvent(KeyboardEvent* event)
{
    const String& keyIdentifier = event->keyIdentifier();
    RenderTheme& renderTheme = RenderTheme::theme();
    int keyCode = event->keyCode();
    if (isSpatialNavigationEnabled(document().frame()))
        return false;
    return (renderTheme.popsMenuByArrowKeys() &&  (keyIdentifier == "Down" ||
keyIdentifier == "Up"))
        || (renderTheme.popsMenuByAltDownUpOrF4Key() && (keyIdentifier == "Down"
|| keyIdentifier == "Up") && (event->altKey() || event->altGraphKey()))
        || (renderTheme.popsMenuByAltDownUpOrF4Key() && (!event->altKey() &&
!event->ctrlKey() && keyIdentifier == "F4"));
}

bool HTMLSelectElement::shouldOpenPopupForKeyPressEvent(KeyboardEvent* event)
{
    const String& keyIdentifier = event->keyIdentifier();
    RenderTheme& renderTheme = RenderTheme::theme();
    int keyCode = event->keyCode();
    return ((renderTheme.popsMenuBySpaceKey() && keyCode == ' ')
        || (!renderTheme.popsMenuByArrowKeys() &&
renderTheme.popsMenuByReturnKey() && keyCode == '\r'));
}

void HTMLSelectElement::menuListDefaultEventHandler(Event* event)
{
    ...
    if (event->type() == EventTypeNames::keydown) {
        if (!renderer() || !event->isKeyboardEvent())
            return;

        KeyboardEvent* keyEvent = toKeyboardEvent(event);
        if (shouldOpenPopupForKeyDownEvent(keyEvent)) {
            handlePopupOpenKeyboardEvent(keyEvent);
            return;
        }
        ...
    }
    if (event->type() == EventTypeNames::keypress) {
        if (!renderer() || !event->isKeyboardEvent())
            return;

        int keyCode = toKeyboardEvent(event)->keyCode();
        if (keyCode == ' ' && isSpatialNavigationEnabled(document().frame())) {
            // Use space to toggle arrow key handling for selection change or
spatial navigation.
            m_activeSelectionState = !m_activeSelectionState;
            event->setDefaultHandled();
            return;
        }
        
        KeyboardEvent* keyEvent = toKeyboardEvent(event);
        if (shouldOpenPopupForKeyPressEvent(keyEvent)) {
            handlePopupOpenKeyboardEvent(keyEvent);
            return;
        }
    }
    ...
}

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1123: int keyCode =
toKeyboardEvent(event)->keyCode();
keyEvent

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1135: ||
(!renderTheme.popsMenuByArrowKeys() && renderTheme.popsMenuByReturnKey() &&
keyCode == '\r')); // popsMenuByArrowKeys() is true only for mac
popsMenuByReturnKey() should be false on mac so you don't need to check
popsMenuByArrowKeys

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1146: // down/up+alt & f4 works for linux
and windows
I don't think we need this comment because we aren't doing anything special.
This is written in RenderTheme.

https://codereview.chromium.org/368023003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderThemeChromiumMac.h (right):

https://codereview.chromium.org/368023003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderThemeChromiumMac.h:81: virtual bool
popsMenuByReturnKey() const OVERRIDE FINAL { return true; }
This should be false. On mac the popup should be opened for space but not for
return key.

Habib Virji

@keishi: One query regarding popsMenuByReturnKey. In current code-> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/HTMLSelectElement.cpp&l=1228 This line gets true only in ...

6 years, 5 months ago (2014-07-09 09:12:34 UTC) #8

keishi

On 2014/07/09 09:12:34, Habib Virji wrote: > @keishi: One query regarding popsMenuByReturnKey. > > In ...

6 years, 5 months ago (2014-07-09 09:35:10 UTC) #9

Habib Virji

Thanks keishi, updated as suggested.. https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSelectElement.cpp File Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSelectElement.cpp#newcode1117 Source/core/html/HTMLSelectElement.cpp:1117: bool HTMLSelectElement::handlePopupOpenKeyboardEvent(Event* event) On ...

6 years, 5 months ago (2014-07-09 09:49:39 UTC) #10

Thanks keishi, updated as suggested..

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1117: bool
HTMLSelectElement::handlePopupOpenKeyboardEvent(Event* event)
On 2014/07/09 03:38:21, keishi wrote:
> HTMLSelectElement::handlePopupOpenKeyboardEvent has stuff not related to
opening
> the popup and as you mention in the comments the early return stuff is tricky.
> 
> Maybe the code will be easier to follow if most of the structure was kept in
> HTMLSelectElement::menuListDefaultEventHandler.
> 
> For example:
> 
> // Return true if 
> bool HTMLSelectElement::handlePopupOpenKeyboardEvent(KeyboardEvent* event)
> {
>     focus();
>     // Calling focus() may cause us to lose our renderer. Return true so
>     // that our caller doesn't process the event further, but don't set
>     // the event as handled.
>     if (!renderer() || !renderer()->isMenuList() || isDisabledFormControl())
>         return true;
>     // Save the selection so it can be compared to the new selection
>     // when dispatching change events during selectOption, which
>     // gets called from RenderMenuList::valueChanged, which gets called
>     // after the user makes a selection from the menu.
>     saveLastSelection();
>     if (RenderMenuList* menuList = toRenderMenuList(renderer()))
>         menuList->showPopup();
>     int index = selectedIndex();
>     ASSERT(index >= 0);
>     ASSERT_WITH_SECURITY_IMPLICATION(index <
> static_cast<int>(listItems().size()));
>     setSelectedIndex(index);
>     event->setDefaultHandled();
>     return false;
> }
> 
> bool HTMLSelectElement::shouldOpenPopupForKeyDownEvent(KeyboardEvent* event)
> {
>     const String& keyIdentifier = event->keyIdentifier();
>     RenderTheme& renderTheme = RenderTheme::theme();
>     int keyCode = event->keyCode();
>     if (isSpatialNavigationEnabled(document().frame()))
>         return false;
>     return (renderTheme.popsMenuByArrowKeys() &&  (keyIdentifier == "Down" ||
> keyIdentifier == "Up"))
>         || (renderTheme.popsMenuByAltDownUpOrF4Key() && (keyIdentifier ==
"Down"
> || keyIdentifier == "Up") && (event->altKey() || event->altGraphKey()))
>         || (renderTheme.popsMenuByAltDownUpOrF4Key() && (!event->altKey() &&
> !event->ctrlKey() && keyIdentifier == "F4"));
> }
> 
> bool HTMLSelectElement::shouldOpenPopupForKeyPressEvent(KeyboardEvent* event)
> {
>     const String& keyIdentifier = event->keyIdentifier();
>     RenderTheme& renderTheme = RenderTheme::theme();
>     int keyCode = event->keyCode();
>     return ((renderTheme.popsMenuBySpaceKey() && keyCode == ' ')
>         || (!renderTheme.popsMenuByArrowKeys() &&
> renderTheme.popsMenuByReturnKey() && keyCode == '\r'));
> }
> 
> void HTMLSelectElement::menuListDefaultEventHandler(Event* event)
> {
>     ...
>     if (event->type() == EventTypeNames::keydown) {
>         if (!renderer() || !event->isKeyboardEvent())
>             return;
> 
>         KeyboardEvent* keyEvent = toKeyboardEvent(event);
>         if (shouldOpenPopupForKeyDownEvent(keyEvent)) {
>             handlePopupOpenKeyboardEvent(keyEvent);
>             return;
>         }
>         ...
>     }
>     if (event->type() == EventTypeNames::keypress) {
>         if (!renderer() || !event->isKeyboardEvent())
>             return;
> 
>         int keyCode = toKeyboardEvent(event)->keyCode();
>         if (keyCode == ' ' && isSpatialNavigationEnabled(document().frame()))
{
>             // Use space to toggle arrow key handling for selection change or
> spatial navigation.
>             m_activeSelectionState = !m_activeSelectionState;
>             event->setDefaultHandled();
>             return;
>         }
>         
>         KeyboardEvent* keyEvent = toKeyboardEvent(event);
>         if (shouldOpenPopupForKeyPressEvent(keyEvent)) {
>             handlePopupOpenKeyboardEvent(keyEvent);
>             return;
>         }
>     }
>     ...
> }
> 

Done.

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1123: int keyCode =
toKeyboardEvent(event)->keyCode();
On 2014/07/09 03:38:21, keishi wrote:
> keyEvent

Done.

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1135: ||
(!renderTheme.popsMenuByArrowKeys() && renderTheme.popsMenuByReturnKey() &&
keyCode == '\r')); // popsMenuByArrowKeys() is true only for mac
On 2014/07/09 03:38:21, keishi wrote:
> popsMenuByReturnKey() should be false on mac so you don't need to check
> popsMenuByArrowKeys

Done.

https://codereview.chromium.org/368023003/diff/100001/Source/core/html/HTMLSe...
Source/core/html/HTMLSelectElement.cpp:1146: // down/up+alt & f4 works for linux
and windows
On 2014/07/09 03:38:21, keishi wrote:
> I don't think we need this comment because we aren't doing anything special.
> This is written in RenderTheme.

Done.

keishi

Looks like select-popup-pagekeys.html is failing. I'm gussing PageUp/Down shouldn't work on mac.

6 years, 5 months ago (2014-07-09 12:31:26 UTC) #11

Habib Virji

On 2014/07/09 12:31:26, keishi wrote: > Looks like select-popup-pagekeys.html is failing. I'm gussing PageUp/Down > ...

6 years, 5 months ago (2014-07-09 12:43:19 UTC) #12

keishi

LGTM https://codereview.chromium.org/368023003/diff/160001/Source/core/html/HTMLSelectElement.cpp File Source/core/html/HTMLSelectElement.cpp (right): https://codereview.chromium.org/368023003/diff/160001/Source/core/html/HTMLSelectElement.cpp#newcode1182 Source/core/html/HTMLSelectElement.cpp:1182: // Early return as for mac below handling ...

6 years, 5 months ago (2014-07-10 02:24:11 UTC) #13

tkent

web/ lgtm https://codereview.chromium.org/368023003/diff/120001/Source/web/PopupListBox.cpp File Source/web/PopupListBox.cpp (right): https://codereview.chromium.org/368023003/diff/120001/Source/web/PopupListBox.cpp#newcode247 Source/web/PopupListBox.cpp:247: hidePopup(); Probably we should do |return| after ...

6 years, 5 months ago (2014-07-10 02:26:55 UTC) #14

Habib Virji

On 2014/07/10 02:26:55, tkent wrote: > web/ lgtm > > https://codereview.chromium.org/368023003/diff/120001/Source/web/PopupListBox.cpp > File Source/web/PopupListBox.cpp (right): ...

6 years, 5 months ago (2014-07-10 08:15:16 UTC) #15