>>> My Info: Win7_64, Nightly 52, 32bit, ID 20161028030204 (2016-10-28) STR_1: 1. Open attachment 8675621 [details] 2. Click in urlbar 3. Press Shift+Tab 4. Press Shift+Tab AR: Identity block doesn't become focused ER: Identity block should befome focused in Step 3 or Step 4 This is regression from bug 1267617. Regression range: > https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=cc1d6ac8152de5394ca090ff24aa89c590ef87a4&tochange=3705eb1dc2cce4582cc3f95af52aeec700801fae@ Johann Hofmann [:johannh]: It seems that this is a regresion caused by your change. Please have a look.

Please see the attachment, this is the identity block? I tested with an older Nightly build, before bug 1267617 was implemented and there are some function differences. Johann can you please take a look at this bug and tell us your opinion about it?

I think this is indeed a regression. At least this only seems to happen if there's an active permission indicator. I can focus the identity block otherwise.

This is all pretty horrible. If you look at http://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#693 you can see that we use custom hackery to enable user focus on the identity block only on focusing the urlbar. So, when a child element of the identity block is focused, the urlbar loses focus and the identity block can't be focused anymore. This will become a big mess of onfocus and onblur if we don't find a clever way to solve it. I need to think about this. Ideas welcome :/

Maybe Drew knows more about how all this works?

I'm not sure what's going on, but the weird onfocus and onblur have been there since early 2008 in bug 413844: https://hg.mozilla.org/mozilla-central/rev/9f638011fffb The setTimeout was added to the onblur in 2009: https://hg.mozilla.org/mozilla-central/diff/cabb8925dcd3/browser/base/content/browser.xul It might help to look at bug 413844 to see why the handlers were added, if in fact they are related to the problem. Maybe things have changed since then?

Can we just change the logic of that onblur handler from unconditionally removing MozUserFocus to checking whether the identity block or any of its ancestors has focus before removing it? The identity block would need to run the same check when it's blurred, except it would check the textbox... :-/

I thought, hey, let's just try to remove the old behavior and see what happens. So after trying it out it just magically works fine. The problem from bug 413844 does not occur (the urlbar is focused on F6) and you can now normally tab through the urlbar and the identity block. Tested on OSX and Ubuntu so far, I'll test on Win7 once I have restored my broken VM (I need to get a Win10 license). Flagging Gijs for review because I want as many eyes on this as possible and he has a pretty good record of reminding me of some simple thing I'm overlooking :)

Comment on attachment 8829810 [details] Bug 1327946 - Remove conditional moz-user-focus on identity block from urlbar. https://reviewboard.mozilla.org/r/106798/#review107814 Tentative r+ based on your testing, but can you push to try and ask MarcoZ to verify that this works? ::: browser/themes/shared/identity-block/identity-block.inc.css:24 (Diff revision 1) > padding: 3px 5px; > overflow: hidden; > /* The padding-left and padding-right transitions handle the delayed hiding of > the forward button when hovered. */ > transition: padding-left, padding-right; > + -moz-user-focus: normal; This feels like something that should be in browser/base/content/browser.css instead.

Thanks! Try is underway. Marco, could you check the patch and verify that my testing was correct (that the urlbar and the identity block can be focused correctly)? Thank you!

(In reply to Johann Hofmann [:johannh] from comment #11) > Thanks! Try is underway. Marco, could you check the patch and verify that my > testing was correct (that the urlbar and the identity block can be focused > correctly)? Will gladly do that, but unfortunately you forgot to include the link where the try build will appear. ;)

(In reply to Marco Zehe (:MarcoZ) from comment #12) > (In reply to Johann Hofmann [:johannh] from comment #11) > > Thanks! Try is underway. Marco, could you check the patch and verify that my > > testing was correct (that the urlbar and the identity block can be focused > > correctly)? > > Will gladly do that, but unfortunately you forgot to include the link where > the try build will appear. ;) This one? https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaf35320eb2f5c5a19539e44ae88b417d6d1073c In fact, we already have a failure, so let me fix that first to not waste your time :)

(In reply to Johann Hofmann [:johannh] from comment #13) > (In reply to Marco Zehe (:MarcoZ) from comment #12) > > (In reply to Johann Hofmann [:johannh] from comment #11) > > > Thanks! Try is underway. Marco, could you check the patch and verify that my > > > testing was correct (that the urlbar and the identity block can be focused > > > correctly)? > > > > Will gladly do that, but unfortunately you forgot to include the link where > > the try build will appear. ;) > > This one? > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=eaf35320eb2f5c5a19539e44ae88b417d6d1073c Yeah that will do, I can dig up the actual build from there. > > In fact, we already have a failure, so let me fix that first to not waste > your time :) Heh OK. :) Please re-request NI when you are ready for me to try something. :-)

So the test failure was related to Gijs comment, I moved the change to browser/base/content/browser.css and made sure that the identity block will not be focused when the url bar is set to pageproxystate=false. Marco, there's another try build running (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7967aafabb8) but I'm relatively certain that this one will succeed, so please go ahead.

I just gave the try build a spin, and the site identity button as well as any permission prompt buttons can once again be reached via the keyboard. However, there is one particular problem: 1. I CTRL-L to the location bar. 2. I shift-tab. 3. If I have a password prompt I didn't answer, for example, focus first lands on the site identity, then gets immediately shifted to the Open Password Prompt button. I would expect shift-tab to immediately land on that one instead. 4. I can then shift-tab once more to the site identity button, and this time, focus stays there. The focus shift in step 3 above may seem visually very quick, barely noticeable, but it is slow enough for the screen reader to pick it up and double-speak. It will first speak finishing the View Site Identity button, then speak the Open Password Prompt button afterwards. If a user doesn't stop listening to the announcement and presses Space, they might end up opening the wrong panel, one they didn't intend to. So this focus inconsistency should be investigated.

Re-requesting review since I added tests and changed the CSS.

(In reply to Marco Zehe (:MarcoZ) from comment #18) > I just gave the try build a spin, and the site identity button as well as > any permission prompt buttons can once again be reached via the keyboard. > Thank you! > However, there is one particular problem: > 1. I CTRL-L to the location bar. > 2. I shift-tab. > 3. If I have a password prompt I didn't answer, for example, focus first > lands on the site identity, then gets immediately shifted to the Open > Password Prompt button. I would expect shift-tab to immediately land on that > one instead. > 4. I can then shift-tab once more to the site identity button, and this > time, focus stays there. > > The focus shift in step 3 above may seem visually very quick, barely > noticeable, but it is slow enough for the screen reader to pick it up and > double-speak. It will first speak finishing the View Site Identity button, > then speak the Open Password Prompt button afterwards. If a user doesn't > stop listening to the announcement and presses Space, they might end up > opening the wrong panel, one they didn't intend to. > So this focus inconsistency should be investigated. Huh, I'm not sure what's causing this. I can look into it, but I think we shouldn't block this patch from landing so I'd rather make a new bug for the issue. Do you agree?

(In reply to Johann Hofmann [:johannh] from comment #21) > (In reply to Marco Zehe (:MarcoZ) from comment #18) > > However, there is one particular problem: > > 1. I CTRL-L to the location bar. > > 2. I shift-tab. > > 3. If I have a password prompt I didn't answer, for example, focus first > > lands on the site identity, then gets immediately shifted to the Open > > Password Prompt button. I would expect shift-tab to immediately land on that > > one instead. > > 4. I can then shift-tab once more to the site identity button, and this > > time, focus stays there. > > > > The focus shift in step 3 above may seem visually very quick, barely > > noticeable, but it is slow enough for the screen reader to pick it up and > > double-speak. It will first speak finishing the View Site Identity button, > > then speak the Open Password Prompt button afterwards. If a user doesn't > > stop listening to the announcement and presses Space, they might end up > > opening the wrong panel, one they didn't intend to. > > So this focus inconsistency should be investigated. > > Huh, I'm not sure what's causing this. I can look into it, but I think we > shouldn't block this patch from landing so I'd rather make a new bug for the > issue. Do you agree? Yes that's fine.

Comment on attachment 8829810 [details] Bug 1327946 - Remove conditional moz-user-focus on identity block from urlbar. https://reviewboard.mozilla.org/r/106798/#review108218 Code change is fine, I have a bunch of nits for the tests but nothing that I'd need to see again before you land. :-) ::: browser/base/content/test/general/browser.ini:329 (Diff revision 3) > skip-if = e10s # Bug 863514 - no gesture support. > [browser_getshortcutoruri.js] > [browser_hide_removing.js] > [browser_homeDrop.js] > [browser_identity_UI.js] > +[browser_identityBlock_focus.js] Put this in test/urlbar instead, please. You might need to include the permissions file as a support file with an absolute path. ::: browser/base/content/test/general/browser_identityBlock_focus.js:1 (Diff revision 3) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ Don't need this. :-) ::: browser/base/content/test/general/browser_identityBlock_focus.js:8 (Diff revision 3) > + > +/* Tests that the identity block can be reached via keyboard > + * shortcuts and that it has the correct tab order. > + */ > + > +const PERMISSIONS_PAGE = "https://example.com/browser/browser/base/content/test/general/permissions.html"; Nit: use getRootDirectory() or similar to avoid hardcoding the entire path, so that it keeps working if you move the test. ::: browser/base/content/test/general/browser_identityBlock_focus.js:13 (Diff revision 3) > +const PERMISSIONS_PAGE = "https://example.com/browser/browser/base/content/test/general/permissions.html"; > + > +function* synthesizeKeyAndWaitForFocus(element, keyCode, options) { > + let focused = BrowserTestUtils.waitForEvent(element, "focus"); > + EventUtils.synthesizeKey(keyCode, options); > + yield focused; could just return this, and not have it a generator function... ::: browser/base/content/test/general/browser_identityBlock_focus.js:35 (Diff revision 3) > + // Request a permission; > + yield ContentTask.spawn(browser, {}, function() { > + E10SUtils.wrapHandlingUserInput(content, true, function() { > + content.document.getElementById("geo").click(); > + }); > + }); Can't you use BTU.synthesizeMouse ? That takes a selector and runs in the child. ::: browser/base/content/test/general/browser_identityBlock_focus.js:59 (Diff revision 3) > + > +// Checks that with invalid pageproxystate the identity block is ignored. > +add_task(function* testInvalidPageProxyState() { > + yield BrowserTestUtils.withNewTab("about:blank", function*(browser) { > + // Make sure the urlbar is focused. > + gURLBar.focus(); Why are we not using this everywhere? Seems less error-prone than relying on the shortcut. :-) ::: browser/base/content/test/general/browser_identityBlock_focus.js:61 (Diff revision 3) > + isnot(document.activeElement, gIdentityHandler._identityBox, > + "identity block should not be focused"); I would expect that you'd need to restore focus after you're done here.

> I would expect that you'd need to restore focus after you're done here. Isn't the tab closing after the test restoring the focus?

(In reply to Johann Hofmann [:johannh] from comment #25) > > I would expect that you'd need to restore focus after you're done here. > > Isn't the tab closing after the test restoring the focus? Oh, right, it might do. I'm not sure. :-)

(In reply to :Gijs from comment #26) > (In reply to Johann Hofmann [:johannh] from comment #25) > > > I would expect that you'd need to restore focus after you're done here. > > > > Isn't the tab closing after the test restoring the focus? > > Oh, right, it might do. I'm not sure. :-) I thought it would return to the urlbar when closing the tab but it seems it's stuck on the original tab. I'll restore it to the urlbar then. :)

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e476d64cf93 Remove conditional moz-user-focus on identity block from urlbar. r=Gijs

sorry had to back this out for timeouts in https://treeherder.mozilla.org/logviewer.html#?job_id=72170457&repo=autoland

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cdc5e22c1268 Backed out changeset 7e476d64cf93 for timeouts in browser_identityBlock_focus.js

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ce01ff2ef384 Remove conditional moz-user-focus on identity block from urlbar. r=Gijs

Please request Aurora/Beta approval on this when you feel it's sufficiently baked on Nightly.

Comment on attachment 8829810 [details] Bug 1327946 - Remove conditional moz-user-focus on identity block from urlbar. Approval Request Comment [Feature/Bug causing the regression]: bug 1267617 [User impact if declined]: Users can not reach the identity block with their keyboard if another icon such as the plugin icon or a doorhanger anchor icon is visible. This is really not great for a11y. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: See comment 0 [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Medium [Why is the change risky/not risky?]: Most of the patch simply reverts an old hack, but we don't know why it's not necessary anymore. Luckily the behavior is well covered in the attached tests so we'd know quickly if the hack would still be required in 52 or 53. [String changes made/needed]: None

Comment on attachment 8829810 [details] Bug 1327946 - Remove conditional moz-user-focus on identity block from urlbar. Let's bring this fix to aurora and beta, the bug breaks a11y and keyboard nav.

Once this lands (likely in beta 3), Andrei, can your team help verify the fix? Thanks. If you need help testing a11y you can try n-i to Marco.

Build ID: 20170131030205 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1.

Backed out for failing own test browser_identityBlock_focus.js: https://hg.mozilla.org/releases/mozilla-beta/rev/964388f3501946027b15ef9af6e6bca2f4096552 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=e80cbae08ecb0b43585f609758c8540de66d31a8 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73586381&repo=mozilla-beta [task 2017-02-01T16:56:44.051337Z] 16:56:44 INFO - TEST-PASS | browser/base/content/test/siteIdentity/browser_identityBlock_focus.js | identity block should be focused - [task 2017-02-01T16:56:44.054099Z] 16:56:44 INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“r†modifiers=“accel,alt†id=“toggleReaderModeâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 776}] [task 2017-02-01T16:56:44.056373Z] 16:56:44 INFO - Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key=“i†modifiers=“accel,alt,shift†id=“key_browserToolboxâ€" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 776}] [task 2017-02-01T16:56:44.059453Z] 16:56:44 INFO - Leaving test bound testWithoutNotifications [task 2017-02-01T16:56:44.061599Z] 16:56:44 INFO - Entering test bound testWithoutNotifications [task 2017-02-01T16:56:44.065640Z] 16:56:44 INFO - Buffered messages finished [task 2017-02-01T16:56:44.067723Z] 16:56:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/siteIdentity/browser_identityBlock_focus.js | Test timed out - [task 2017-02-01T16:56:44.069889Z] 16:56:44 INFO - MEMORY STAT vsizeMaxContiguous not supported in this build configuration. [task 2017-02-01T16:56:44.074416Z] 16:56:44 INFO - MEMORY STAT | vsize 975MB | residentFast 203MB | heapAllocated 87MB [task 2017-02-01T16:56:44.077141Z] 16:56:44 INFO - TEST-OK | browser/base/content/test/siteIdentity/browser_identityBlock_focus.js | took 45135ms [task 2017-02-01T16:56:44.079128Z] 16:56:44 INFO - Not taking screenshot here: see the one that was previously logged [task 2017-02-01T16:56:44.081290Z] 16:56:44 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/siteIdentity/browser_identityBlock_focus.js | Found a tab after previous test timed out: https://example.com/browser/browser/base/content/test/siteIdentity/permissions.html -

Verified fixed FX 53.0a2 (2017-02-02) (32-bit) Win 7. Still reproduces on FX 52b3.

Tracking for 52, we seem close to a fix and we could still likely take a patch for beta.

Sorry for the delay. The beta timeout reason is actually quite trivial. It happens because http://searchfox.org/mozilla-central/source/browser/base/content/test/general/permissions.html contains a button that's not present in beta yet (see blame on line 12). Running the tests locally with that line added makes them pass. Aryx/Ryan, can one of you give some advice on what would be the easiest way to add this single line to beta? Just uplift another patch? (I'm absolutely certain it will not have any unintended side effects).

Ok, after chatting with RyanVM on IRC I made attachment 8834186 [details] [diff] [review] which should solve the beta test failures if applied to beta (at least it does on my machine). It's simply adding a button to a test file that was introduced in 53.

Verified fixed FX 52b5, Win 7.