1272012 - Up/Down key on a combobox with a closed dropdown menu should open it on OSX

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: test tweaks

I'll file a follow-up bug on enabling the few a11y tests I've disabled on OSX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b996953f0b32

Comment 3

[Neil Deakin]

Comment on attachment 8752329 [details] [diff] [review]
fix

>+  bool dropDownMenuOnUpDown =
>+#ifdef XP_MACOSX
>+    IsInDropDownMode() && !mComboboxFrame->IsDroppedDown();
>+#else
>+    keyEvent->IsAlt();
>+#endif
>+  if (dropDownMenuOnUpDown &&
>+      (keyEvent->keyCode == NS_VK_UP || keyEvent->keyCode == NS_VK_DOWN)) {
>+    DropDownToggleKey(aKeyEvent);
>+    if (keyEvent->DefaultPrevented()) {
>+      return NS_OK;
>+    }
>+  }

Does your comment earlier in this method relate to the extra DefaultPrevented check here?

As an aside, Mac also opens dropdowns on a modified space key.

Comment 4

[Mats Palmgren (inactive)]

(In reply to Neil Deakin from comment #3)
> Does your comment earlier in this method relate to the extra
> DefaultPrevented check here?

I intended it as an early return for the case we do open the menu -
DropDownToggleKey calls PreventDefault() here:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#2067

But you're right, it could have already been PreventDefault'ed
before that.  Given my earlier code comment there, that's probably
OK though.

> As an aside, Mac also opens dropdowns on a modified space key.

Good point, let's fix that while we're here.

My quick testing shows that:
OSX: spacebar *toggles* the menu, but not if Alt, Ctrl or Cmd is pressed.
IE11 on Windows 7, and native GTK apps behaves the same: spacebar *opens*
the menu but doesn't close it (with/without any key modifiers).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f6864653aaf

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix, v2

Added support for Space key.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e24bac891d7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e25db36b720

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c804afcc865
https://hg.mozilla.org/integration/mozilla-inbound/rev/467b0d255265

Comment 8

[Mats Palmgren (inactive)]

Filed bug 1275513 to re-enable the a11y tests I disabled on OSX in this bug.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in test_bug615833.html on OS X 10.10 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52a8bf703126
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c66fb81be68

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5e25db36b720b695da94b2bc1373ea369408e404
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=28685051&repo=mozilla-inbound

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=28650912&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19753db8aa95
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd37cbc7e543

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c2a7a3f9ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4592d84a9e

Comment 13

[Mats Palmgren (inactive)]

Hmm, yeah that test was added after I did my Try runs.
I totally blame Neil ;-)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9c2a7a3f9ca
https://hg.mozilla.org/mozilla-central/rev/bd4592d84a9e