Using alt and down key to open datalist using keyboard

Using alt and down key to open datalist using keyboard

On pressing alty+down key, datalist is open and no stepDown is generated.

R=tkent@chromium.org, isherman@chromium.org, keishi1@chromium.org
BUG=229073
TEST= On down+alt key combination does not generate stepDown.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170673

[Habib Virji, 2014-04-01 16:31:55]

Using alt+down arrow key in input type number to get the datalist. Invokes
chrome since it already checks for down arrow key it works fine. 

Have added test to capture pressing alt+down to open datalist open but since it
runs in content_shell was not able to capture that in test. Though the datalist
box do appear with alt+key in chrome. Saw other tests using number datalist are
similar they seem to be not showing datalist box. Please suggest if possible
somehow to show datalist option in content_shell and capture suggestion box
appearance.

[tkent, 2014-04-02 00:41:31]

+keishi

[keishi, 2014-04-02 08:38:01]

The autofill popup where datalist suggestions are shown is opened in the
chromium side so it is not observable from layout tests.

I think all you can do is add a test to number-keyoperation.html that checks
that down+alt does trigger step down.

https://codereview.chromium.org/218683010/diff/1/Source/core/html/forms/TextF...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/218683010/diff/1/Source/core/html/forms/TextF...
Source/core/html/forms/TextFieldInputType.cpp:215:
chrome->client().handleKeyboardEventOnTextField(element(), *event);
I think if we return early without calling event->setDefaultHandled(),
handleKeyboardEventOnTextField will be called from
NumberInputType::handleKeydownEvent.

[Habib Virji, 2014-04-02 08:48:03]

"I think all you can do is add a test to number-keyoperation.html that checks
that down+alt does trigger step down."

--> Do you mean does *not* trigger stepdown?

[keishi, 2014-04-02 08:55:06]

On 2014/04/02 08:48:03, Habib Virji wrote:
> "I think all you can do is add a test to number-keyoperation.html that checks
> that down+alt does trigger step down."
> 
> --> Do you mean does *not* trigger stepdown?

sorry, yes

[Habib Virji, 2014-04-02 09:28:32]

On 2014/04/02 08:38:01, keishi1 wrote:
> The autofill popup where datalist suggestions are shown is opened in the
> chromium side so it is not observable from layout tests.
> 
> I think all you can do is add a test to number-keyoperation.html that checks
> that down+alt does trigger step down.
> 
>
https://codereview.chromium.org/218683010/diff/1/Source/core/html/forms/TextF...
> File Source/core/html/forms/TextFieldInputType.cpp (right):
> 
>
https://codereview.chromium.org/218683010/diff/1/Source/core/html/forms/TextF...
> Source/core/html/forms/TextFieldInputType.cpp:215:
> chrome->client().handleKeyboardEventOnTextField(element(), *event);
> I think if we return early without calling event->setDefaultHandled(),
> handleKeyboardEventOnTextField will be called from
> NumberInputType::handleKeydownEvent.

Thanks keishi for review. Updated as suggested. Please suggest if its okay.

[keishi, 2014-04-02 11:15:38]

https://codereview.chromium.org/218683010/diff/20001/LayoutTests/fast/forms/n...
File LayoutTests/fast/forms/number/number-appearance-keyboard.html (right):

https://codereview.chromium.org/218683010/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-appearance-keyboard.html:17:
eventSender.keyDown('downArrow', ['altKey']);
I don't think this needs to be a separate file. Also the file name doesn't make
sense, we aren't testing appearance.
You can add the down+alt case to number-keyoperation.html

https://codereview.chromium.org/218683010/diff/20001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/218683010/diff/20001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:214: if (Chrome* chrome =
this->chrome())
I meant something like this. I tried it and it seems to be working.

    if (key == "Up")
        spinButtonStepUp();
    else if (key == "Down" && !event->altKey())
        spinButtonStepDown();
    else
        return;

[Habib Virji, 2014-04-02 12:27:26]

Thanks. Done as suggested.

https://codereview.chromium.org/218683010/diff/20001/LayoutTests/fast/forms/n...
File LayoutTests/fast/forms/number/number-appearance-keyboard.html (right):

https://codereview.chromium.org/218683010/diff/20001/LayoutTests/fast/forms/n...
LayoutTests/fast/forms/number/number-appearance-keyboard.html:17:
eventSender.keyDown('downArrow', ['altKey']);
On 2014/04/02 11:15:38, keishi1 wrote:
> I don't think this needs to be a separate file. Also the file name doesn't
make
> sense, we aren't testing appearance.
> You can add the down+alt case to number-keyoperation.html

Done.

https://codereview.chromium.org/218683010/diff/20001/Source/core/html/forms/T...
File Source/core/html/forms/TextFieldInputType.cpp (right):

https://codereview.chromium.org/218683010/diff/20001/Source/core/html/forms/T...
Source/core/html/forms/TextFieldInputType.cpp:214: if (Chrome* chrome =
this->chrome())
On 2014/04/02 11:15:38, keishi1 wrote:
> I meant something like this. I tried it and it seems to be working.
> 
>     if (key == "Up")
>         spinButtonStepUp();
>     else if (key == "Down" && !event->altKey())
>         spinButtonStepDown();
>     else
>         return;

Done.

[keishi, 2014-04-02 12:36:11]

lgtm

[Habib Virji, 2014-04-02 16:41:01]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-04-02 16:41:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/218683010/40001

[commit-bot: I haz the power, 2014-04-02 17:44:23]

Change committed as 170673