MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/87d03c40c52a557902464d6351b543c8e7991c93
Cr-Commit-Position: refs/heads/master@{#405374}

[dpapad, 2016-07-12 18:29:12]

Description was changed from

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
==========

to

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-07-12 18:30:41]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-07-12 20:14:49]

dpapad@chromium.org changed reviewers:
+ michaelpg@chromium.org

[dpapad, 2016-07-12 20:14:50]

FYI, I attempted to add a simple test for the keyboard behavior in
dropdown_menu_tests.js, without success. Polymer is doing something fancy (did
not entirely figure out yet), that causes MockInteractions.pressEnter() to not
trigger the necessary event callbacks, even though manually pressing enter
works. From my debugging, the manual "Enter" key press is at some point
converted to a MouseEvent, but could not figure out where does that happen. I'll
try to come up with a minimal repro case, but I don't think this should hold up
the regression fix.

[michaelpg, 2016-07-12 22:38:03]

On 2016/07/12 20:14:50, dpapad wrote:
> FYI, I attempted to add a simple test for the keyboard behavior in
> dropdown_menu_tests.js, without success. Polymer is doing something fancy (did
> not entirely figure out yet), that causes MockInteractions.pressEnter() to not
> trigger the necessary event callbacks, even though manually pressing enter
> works. From my debugging, the manual "Enter" key press is at some point
> converted to a MouseEvent, but could not figure out where does that happen.
I'll
> try to come up with a minimal repro case, but I don't think this should hold
up
> the regression fix.

You may be running into the same problem I did, where pressEnter happens
asynchronously. See my workaround here:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/device_p...

[dpapad, 2016-07-12 22:46:24]

On 2016/07/12 at 22:38:03, michaelpg wrote:
> On 2016/07/12 20:14:50, dpapad wrote:
> > FYI, I attempted to add a simple test for the keyboard behavior in
> > dropdown_menu_tests.js, without success. Polymer is doing something fancy
(did
> > not entirely figure out yet), that causes MockInteractions.pressEnter() to
not
> > trigger the necessary event callbacks, even though manually pressing enter
> > works. From my debugging, the manual "Enter" key press is at some point
> > converted to a MouseEvent, but could not figure out where does that happen.
I'll
> > try to come up with a minimal repro case, but I don't think this should hold
up
> > the regression fix.
> 
> You may be running into the same problem I did, where pressEnter happens
> asynchronously. See my workaround here:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/device_p...

Still does not seem to work with Polymer.Base.async(). See this minimal repro
https://jsfiddle.net/j4rfjasf/4/. Any help is appreciated.

[stevenjb, 2016-07-12 22:46:56]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-07-12 22:46:57]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
Should we change to button for all paper-dropdown-menu cases? Maybe not in this
CL, but it would be good to use a consistent pattern.

Also, my understanding is that <div> is considerably cheaper than <button>, I
assume there isn't an easy way to decorate the <div> to get the behavior we
want?

[dpapad, 2016-07-12 22:56:09]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
On 2016/07/12 at 22:46:57, stevenjb wrote:
> Should we change to button for all paper-dropdown-menu cases? Maybe not in
this CL, but it would be good to use a consistent pattern.
> 
> Also, my understanding is that <div> is considerably cheaper than <button>, I
assume there isn't an easy way to decorate the <div> to get the behavior we
want?

I agree that we should change it to something that works for all
paper-dropdown-menus, I am just addressing settings-dropdown-menu only in this
CL.

Regarding performance, my measurements showed that both <button> and <div> are
much faster than <paper-item>. I did not observe a noticeable difference in
performance between <div> and <button>.

[Dan Beam, 2016-07-12 23:00:26]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-07-12 23:00:27]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
80 col wrap

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:43: border: none;
nit: -webkit-appearance: none;

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:46: font-size:
inherit;
do we need to change this to font: inherit?  i remember something funky about
font-face not working well with <button>s by default

[Dan Beam, 2016-07-12 23:09:07]

Description was changed from

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-07-12 23:09:07]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[dpapad, 2016-07-12 23:18:02]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
On 2016/07/12 at 23:00:27, Dan Beam wrote:
> 80 col wrap

Done.

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:43: border: none;
On 2016/07/12 at 23:00:27, Dan Beam wrote:
> nit: -webkit-appearance: none;

Does not seem to work equivalently for me, see http://imgur.com/4rctYdV.

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:46: font-size:
inherit;
On 2016/07/12 at 23:00:27, Dan Beam wrote:
> do we need to change this to font: inherit?  i remember something funky about
font-face not working well with <button>s by default

Done. Changing 'font-size' to 'font' indeed has an effect on the applied font,
from Arial to "Roboto, Ubuntu.."

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(left):

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:20: lands
in paper-item-shared-styles.html-->
Removed this TODO, since based on Michael's latest update at
https://github.com/PolymerElements/paper-item/issues/85#issuecomment-232207944,
it is unclear when/if this PR will land in Polymer.

[Dan Beam, 2016-07-12 23:27:26]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_shared_css.html:43: border: none;
On 2016/07/12 23:18:02, dpapad wrote:
> On 2016/07/12 at 23:00:27, Dan Beam wrote:
> > nit: -webkit-appearance: none;
> 
> Does not seem to work equivalently for me, see http://imgur.com/4rctYdV.

we use it in widgets.css and otherwise a -webkit-appearance: push-button; is
probably applied.  maybe it's just doing a background and your rule is
overriding.  it's probably fine.

[michaelpg, 2016-07-12 23:42:32]

On 2016/07/12 22:46:24, dpapad wrote:
> On 2016/07/12 at 22:38:03, michaelpg wrote:
> > On 2016/07/12 20:14:50, dpapad wrote:
> > > FYI, I attempted to add a simple test for the keyboard behavior in
> > > dropdown_menu_tests.js, without success. Polymer is doing something fancy
> (did
> > > not entirely figure out yet), that causes MockInteractions.pressEnter() to
> not
> > > trigger the necessary event callbacks, even though manually pressing enter
> > > works. From my debugging, the manual "Enter" key press is at some point
> > > converted to a MouseEvent, but could not figure out where does that
happen.
> I'll
> > > try to come up with a minimal repro case, but I don't think this should
hold
> up
> > > the regression fix.
> > 
> > You may be running into the same problem I did, where pressEnter happens
> > asynchronously. See my workaround here:
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/device_p...
> 
> Still does not seem to work with Polymer.Base.async(). See this minimal repro
> https://jsfiddle.net/j4rfjasf/4/. Any help is appreciated.

Ah, your problem is that the item isn't being selected at all. The async only
becomes relevant once you want to check the result.

Can you find a test in the Polymer elements where they simulate pressing Enter
on a menu item? (Unfortunately there are like ten different repos where that
might be.) If not, pick an element that *should* include that test and ask the
owner why it won't work. Doesn't seem like an issue with the way we're using the
elements, thanks to your minimal repro case.

[michaelpg, 2016-07-12 23:44:22]

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
wrap

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:41: align-items:
center;
Have you checked if there's anything else in paper-item-shared-styles.html that
should be copied here? There's a lot of styles packed in there.

[dpapad, 2016-07-13 01:01:07]

After a lot of investigation, I ended up filing a
https://github.com/PolymerElements/paper-dropdown-menu/issues/175 about not
being able to test the "Enter" key functionality, hoping someone can give some
insight.

@michaelpg: Per your suggestion, I looked around several repositories and found
https://github.com/PolymerElements/iron-behaviors/blob/master/test/active-sta...
that uses pressEnter() (did not help me as much as I hoped though). I also found
a bug you had filed
(https://github.com/PolymerElements/iron-behaviors/issues/54), but in my case it
seems that the codepath that succeeds (when I actually use the keyboard) is not
hitting the _asyncClick() codepath anyway.

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_dropdown_menu.html
(right):

https://codereview.chromium.org/2143733002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_dropdown_menu.html:21:
<button class="dropdown-item" role="option" data-value$="[[item.value]]">
On 2016/07/12 at 23:44:22, michaelpg (slow) wrote:
> wrap

Already done.

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2143733002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_shared_css.html:41: align-items:
center;
On 2016/07/12 at 23:44:22, michaelpg (slow) wrote:
> Have you checked if there's anything else in paper-item-shared-styles.html
that should be copied here? There's a lot of styles packed in there.

I checked. The only thing that I see missing is the a dropdown-item[disabled]
style. But I also can't find any such cases in our UI. We tend to not show such
options at all, instead of showing them but have them disabled.

[Dan Beam, 2016-07-13 01:36:26]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-07-13 01:36:26]

lgtm

[michaelpg, 2016-07-13 17:56:18]

lgtm

[dpapad, 2016-07-13 20:33:46]

@stevenjb: Ping.

[stevenjb, 2016-07-13 23:42:48]

On 2016/07/13 20:33:46, dpapad wrote:
> @stevenjb: Ping.

Sorry, my comment was just a drive-by; by default retvield makes one a reviewer
if one comments. lgtm.

[dpapad, 2016-07-14 00:04:09]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-07-14 00:05:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 00:57:24]

Description was changed from

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-14 00:57:25]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-14 00:57:32]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 00:59:36]

Description was changed from

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: settings-dropdown-menu, restore "Enter" key behavior.

BUG=626989
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/87d03c40c52a557902464d6351b543c8e7991c93
Cr-Commit-Position: refs/heads/master@{#405374}
==========

[commit-bot: I haz the power, 2016-07-14 00:59:38]

Patchset 2 (id:??) landed as
https://crrev.com/87d03c40c52a557902464d6351b543c8e7991c93
Cr-Commit-Position: refs/heads/master@{#405374}