1237713 - fix test browser_tabkeynavigation.js

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Tracy Walker [:tracy]]

this is a browser chrome test in /browser/base/content/test/general/

Basically just needs the optional param for window in each synthesizeKey event to be removed so the browser can just default to the tab that is in focus.

patch is currently finishing up in try.

Comment 1

[Tracy Walker [:tracy]]

Attachment: tabkeynavigation.patch

passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=840b324acae4

However, I didn't update this to modern test code.  The more I got into that process the more broken the test case became. Jim, if you want me to modernize it, maybe we can walk through it in my next 1-1.

Otherwise requesting review.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8705780 [details] [diff] [review]
tabkeynavigation.patch

lgtm

Comment 3

[Tracy Walker [:tracy]]

Let's get this enabled as-is, then I'll work on it modernization.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c06ff2c95163

Comment 5

[Phil Ringnalda (:philor)]

Turns out not to have been enough to only run e10s browser-chrome on try, since a couple of those todo_is() tests fail on non-e10s OS X, https://treeherder.mozilla.org/logviewer.html#?job_id=6532247&repo=fx-team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/1764c03305e6

Comment 6

[Tracy Walker [:tracy]]

The fix works fine everywhere except Mac non-e10s.  

As comment in the test case says,
       
// XXX Currently, Command + "{" and "}" don't work if keydown event is
//     consumed because following keypress event isn't fired.

is still affecting non-e10s in this Mac only section of the test code.

Is there a way to skip-if (os == Mac && non-e10s)?

Comment 7

[Tracy Walker [:tracy]]

ah skip-if (os == "mac" && !e10s) 

(I won't be able to figure out why those key events aren't fired under this configuration. The tab switching shortcuts work as expected manually in non-e10s window.)

Anyway we should not hold up enabling this, wherever possible, because of a quirk on Mac non-e10s. 

I'll adjust the patch for browser.ini and put it through a full browser chrome try run, to be certain.

Comment 8

[Tracy Walker [:tracy]]

Attachment: tabkeynavigation.patch

try run: https://hg.mozilla.org/try/rev/bf34cc7614ec  (The failures there are in toolkit. This test is in layout.)

I'll circle back here to modernize this once I have a better handle on those techniques.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e97ba0de7c95

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e97ba0de7c95

Comment 11

[Tracy Walker [:tracy]]

Attachment: bug_1237713.patch

as promised, a follow up patch to modernize this test case.

passed try run with a few unrelated oranges: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=17637334

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8727410 [details] [diff] [review]
bug_1237713.patch

Review of attachment 8727410 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_tabkeynavigation.js
@@ +17,3 @@
>  
> +  gBrowser.selectedTab = tab1;
> +  browser1.contentWindow.focus();

This looks like it'll generate a CPOW, can you confirm? I wonder what the approved way to do this is now.

Comment 13

[Tracy Walker [:tracy]]

Attachment: tabkeynavigation.patch

per mconley; use ".focus" in place of "contentWindow.focus".  It works fine locally. I don't expect that change to affect try run results, so not re-running it.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8727974 [details] [diff] [review]
tabkeynavigation.patch

Review of attachment 8727974 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_tabkeynavigation.js
@@ +17,4 @@
>  
> +  gBrowser.selectedTab = tab1;
> +  browser1.focus();
> +  

nit - you added so me whitespace here in your last edit.

Comment 15

[Tracy Walker [:tracy]]

Attachment: tabkeynavigation.patch

fixed white space nit.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Ugh, this landed with a completely borked commit message. Please double-check that before requesting checkin in the future.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3071a0f6359d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3071a0f6359d