If you use the keyboard to give a tab the focus and then use the right arrow or left arrow to navigate to the neighboring tab, the tab loses focus so you cannot arrow to the next tab. Steps to reproduce: 1. Put the focus on the current selected tab (shift tab twice from the addressbar does it.) 2. Note the dotted border around the tab title 3. Hit the arrow key to move to the next tab 4. Note the lack of dotted border around the tab title 5. Hit the arrow key again to move to the next tab Results: can't move to the next tab because focus lost Expected Results: can arrow back and forth as much as I want without the tab strip losing focus. Tested 57 Beta 4 on Windows 10

I managed to narrow this down to this regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6d4744a6c81c498cc81cad11b4ead64660a3d269&tochange=aa48bb3f494410de514041e8c06ca15c38680181

[Tracking Requested - why for this release]: Regression in keyboard accessibility of primary UI.

This seems to be caused by the fact that we now check for the current state of focus a bit later in the e10s case, and in the arrow case the arrow key itself has moved focus to the new tab already, so in this code (from _adjustFocusBeforeTabSwitch): + // If focus is in the tab bar, retain it there. + if (document.activeElement == oldTab) { + // We need to explicitly focus the new tab, because + // tabbox.xml does this only in some cases. + newTab.focus(); + } else if (gMultiProcessBrowser && document.activeElement !== newBrowser) { + + let keepFocusOnUrlBar = newBrowser && + newBrowser._urlbarFocused && + gURLBar && + gURLBar.focused; + if (!keepFocusOnUrlBar) { + // Clear focus so that _adjustFocusAfterTabSwitch can detect if + // some element has been focused and respect that. + document.activeElement.blur(); + } + } we hit the 'else' block. We should probably just check that the new tab element wasn't explicitly focused before doing that. I've put up a patch that does this.

(In reply to :Gijs (no reviews, PTO EOB on 11th) from comment #4) > + // If focus is in the tab bar, retain it there. > + if (document.activeElement == oldTab) { > + // We need to explicitly focus the new tab, because > + // tabbox.xml does this only in some cases. > + newTab.focus(); > + } else if (gMultiProcessBrowser && document.activeElement !== > newBrowser) { > we hit the 'else' block. We should probably just check that the new tab > element wasn't explicitly focused before doing that. I've put up a patch > that does this. In other words, tabbox.xml actually has us covered now in the non-e10s case. We should clean this up some more then.

(In reply to Dão Gottwald [::dao] from comment #5) > (In reply to :Gijs (no reviews, PTO EOB on 11th) from comment #4) > > + // If focus is in the tab bar, retain it there. > > + if (document.activeElement == oldTab) { > > + // We need to explicitly focus the new tab, because > > + // tabbox.xml does this only in some cases. > > + newTab.focus(); > > + } else if (gMultiProcessBrowser && document.activeElement !== > > newBrowser) { > > > we hit the 'else' block. We should probably just check that the new tab > > element wasn't explicitly focused before doing that. I've put up a patch > > that does this. > > In other words, tabbox.xml actually has us covered now in the non-e10s case. > We should clean this up some more then. I'm confused - this change affects e10s rather than non-e10s. Can you expand on your comment to help me understand what you mean here?

Yeah, I meant e10s when I said non-e10s.

Comment on attachment 8915104 [details] Bug 1404651 - keep focus in the tabstrip when the focus has shifted with the arrow keys, https://reviewboard.mozilla.org/r/186370/#review193130 ::: browser/base/content/tabbrowser.xml:1426 (Diff revision 2) > let findBar = this.getFindBar(oldTab); > oldTab._findBarFocused = (!findBar.hidden && > findBar._findField.getAttribute("focused") == "true"); > } > > - // If focus is in the tab bar, retain it there. > + let activeEl = document.activeElement; Can you write a new comment explaining this whole block? Alternatively, at least keep "If focus is in the tab bar, retain it there" for now because this code is a bit obscure. ::: browser/base/content/tabbrowser.xml:1427 (Diff revision 2) > oldTab._findBarFocused = (!findBar.hidden && > findBar._findField.getAttribute("focused") == "true"); > } > > - // If focus is in the tab bar, retain it there. > - if (document.activeElement == oldTab) { > + let activeEl = document.activeElement; > + if (gMultiProcessBrowser && activeEl !== newBrowser && activeEl !== newTab) { nit: != instead of !==

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6bec02f9a646 keep focus in the tabstrip when the focus has shifted with the arrow keys, r=dao

Backed out for frequently (or permanently? hard to judge because of the test suite chunking) failing browser-chrome's browser/base/content/test/general/browser_tabfocus.js: https://hg.mozilla.org/integration/autoland/rev/6771bdd9b24eedc4df2f99f2e77874acf38a0b56 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6bec02f9a646f11777dbc12ceeb90af76198242d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135949201&repo=autoland [task 2017-10-10T15:28:08.123Z] 15:28:08 INFO - TEST-PASS | browser/base/content/test/general/browser_tabfocus.js | focusing tab element hasFocus - [task 2017-10-10T15:28:08.125Z] 15:28:08 INFO - TEST-PASS | browser/base/content/test/general/browser_tabfocus.js | focusing tab element activeElement - [task 2017-10-10T15:28:08.127Z] 15:28:08 INFO - TEST-PASS | browser/base/content/test/general/browser_tabfocus.js | main-window is always expected - [task 2017-10-10T15:28:08.129Z] 15:28:08 INFO - Buffered messages finished [task 2017-10-10T15:28:08.134Z] 15:28:08 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tabfocus.js | tab change when selected tab element was focused focusedElement - Got browser1, expected tab1 [task 2017-10-10T15:28:08.137Z] 15:28:08 INFO - Stack trace: [task 2017-10-10T15:28:08.140Z] 15:28:08 INFO - chrome://mochikit/content/browser-test.js:test_is:1011 [task 2017-10-10T15:28:08.143Z] 15:28:08 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabfocus.js:compareFocusResults/<:447 [task 2017-10-10T15:28:08.145Z] 15:28:08 INFO - chrome://mochikit/content/browser-test.js:run:1040

Please also check this bc failure on Windows 7 debug with e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=135977273&repo=autoland > TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug462289.js | click on tab2 while tab1 is activated activates tab - Got [object XULElement], expected [object XULElement]

Let's try the old version then, clearly there must be cases where tabbox.xml doesn't have us covered... (the new version fails tests locally and the old one doesn't): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f59dc864202850b580294df8d783b36bf3321a3

Comment on attachment 8915104 [details] Bug 1404651 - keep focus in the tabstrip when the focus has shifted with the arrow keys, (In reply to :Gijs (no reviews, PTO EOB on 11th) from comment #15) > Let's try the old version then, clearly there must be cases where tabbox.xml > doesn't have us covered... (the new version fails tests locally and the old > one doesn't): > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2f59dc864202850b580294df8d783b36bf3321a3 The problematic tests are green here, so let's try this, then.

Comment on attachment 8915104 [details] Bug 1404651 - keep focus in the tabstrip when the focus has shifted with the arrow keys, https://reviewboard.mozilla.org/r/186370/#review193510

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0304a90aee90 keep focus in the tabstrip when the focus has shifted with the arrow keys, r=dao

Given that the bar for uplifts to 57 is getting higher and we've already shipped this regression in 56, I'm inclined to let this ride the 58 train. Thoughts?

The fix is trivial and we have automated test coverage, so I think we should uplift.

Comment on attachment 8915104 [details] Bug 1404651 - keep focus in the tabstrip when the focus has shifted with the arrow keys, Approval Request Comment [Feature/Bug causing the regression]: bug 1362866 [User impact if declined]: broken keyboard navigation in the tab strip [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: / [Is the change risky?]: no [Why is the change risky/not risky?]: trivial fix [String changes made/needed]: /

Comment on attachment 8915104 [details] Bug 1404651 - keep focus in the tabstrip when the focus has shifted with the arrow keys, severe regression, beta57+

I have reproduced this bug with Nightly 58.0a1 (2017-09-30) on Windows 10 , 64 Bit ! This bug's fix is Verified with latest Beta & Nightly ! Build ID 20171013042429 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20171013100112 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0

I reproduced this issue using Fx 58.0a1, build ID: 20170930220116, on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 58.0a1(20171013100112) and Fx 57.0b9, on Windows 10 x64 and Ubuntu 14.04 LTS.