In Electrolysis windows, HTML accesskeys only work when content is focused. They do not work when chrome is focused. Steps to reproduce: 1. Open the attached HTML document in an e10s window, which contains a single <textarea> with accesskey="e". 2. With the document focused, press Alt+Shift+E (Windows or Linux) or Control-Option-E (Mac). => The textarea is focused. 3. Focus a chrome control, such as the URL bar, then repeat step 2. => The chrome control remains focused. The textarea should have become focused. 4. Open the document in a non-e10s window, focus a chrome control, then repeat step 2. => The textarea is focused. Tested in Firefox Developer Edition 40.0a2 (2015-05-24) on OS X 10.10.3.

David, is this something we need to worry about before beta?

Sorry for spam - didn't mean to un-NI myself when adding Gijs.

I don't know anything about key handling in e10s. I do know that the code used to be in nsWindow, then got moved here (at least for Windows): http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1587 with a charming "XXX I don't think we need this" comment above it, with 301 nsWindow::WM_KEYDOWN. YMMV. In any case, I presume that some/all of those should be sent to the content process? I don't know how input forwarding works in this case, so I'm going to defer to mconley.

I have honestly never tried to press an access key from a chrome control. Didn't even know this works in non-e10s until now. Having said that, I rarely use access keys myself, and if I do, am always somewhere in the document. So I'd say no, we don't need to worry about it before beta, but of course if we had this behavior before, we shouldn't break for too long. ;)

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1) > David, is this something we need to worry about before beta? Are we going to turn e10s on by default in beta?

(In reply to David Bolter [:davidb] from comment #5) > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1) > > David, is this something we need to worry about before beta? > > Are we going to turn e10s on by default in beta? let me rephrase: Is this something that should block turning it on by default in beta?

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6) > (In reply to David Bolter [:davidb] from comment #5) > > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1) > > > David, is this something we need to worry about before beta? > > > > Are we going to turn e10s on by default in beta? > > let me rephrase: Is this something that should block turning it on by > default in beta? I don't think we need to block for this one.

An additional data point: accesskeys are used extensively on MediaWiki installations, and of course Bugzilla. :-) Lack of accesskeys at Wikipedia finally forced me to disable e10s. I think this issue should be relnoted if the beta has e10s on by default.

(In reply to :Gijs Kruitbosch from comment #3) > I don't know anything about key handling in e10s. I do know that the code > used to be in nsWindow, then got moved here (at least for Windows): > http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout. > cpp#1587 with a charming "XXX I don't think we need this" comment above it, > with 301 nsWindow::WM_KEYDOWN. YMMV. In any case, I presume that some/all of > those should be sent to the content process? I don't know how input > forwarding works in this case, so I'm going to defer to mconley. And I, in turn, am going to defer to Enn, who I suspect is most likely to know how this mechanism works.

EventStateManager::HandleAccessKey checks for accesskeys on the chrome document, then iterates over child docshells calling HandleAccessKey recursively. This won't include remote children so the accesskeys don't work. We could fix this by just calling nsContentUtils::CallOnAllRemoteChildren afterwards with a function that calls into HandleAccessKey. It also needs to be able to invoke access keys in the parent process from the child.

Just looking for a bit of feedback to make sure the handling in the eventstatemanager is ok, before polishing this and adding some tests.

Comment on attachment 8654312 [details] [diff] [review] Forward accesskey handling to child Well, I feel odd this patch. You tries to check current document first. Next, all child documents in the process. Then, all remote processes *asynchronously*. Finally, all ancestors of the current document *synchronously*. Doesn't it cause two or more contents are focused/activated by a single key operation if both a remote child and an ancestor document has same access key?

And I hope that you'll separated the patches at least: 1. Changing the result of a lot of methods from void to bool. 2. Changing EventStateManager::HandleAccessKey() without logical change (i.e., changes its arguments and sorting out the implementation if it's necessary) 3. Changing for main purpose of your fix.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13) > Comment on attachment 8654312 [details] [diff] [review] > Forward accesskey handling to child > > Well, I feel odd this patch. > > You tries to check current document first. Next, all child documents in the > process. Then, all remote processes *asynchronously*. Finally, all ancestors > of the current document *synchronously*. > > Doesn't it cause two or more contents are focused/activated by a single key > operation if both a remote child and an ancestor document has same access > key? I'll move the handling of the remote children to the end after ancestors are checked. I don't think it will make much difference since we generally only have remote children as the tabs at the top-level window and only one is visible at a time, so I'm not sure it's worth adding extra code to wait for each remote child to see if it handles an accesskey. If the structure of remote children changes, I think we can add code for that then. I've broken this up into four patches which I will attach when I write some tests.

Changes arguments so that accesskey candidates are only determined once, and cleans up return value to use a bool.

The browser_printpreview.js test fails with these patches due to bug 1197266.

Comment on attachment 8662126 [details] [diff] [review] Part 1 - restructure HandleAccessKey >+ int32_t childCount; >+ docShell->GetChildCount(&childCount); >+ for (int32_t counter = 0; counter < childCount; counter++) { >+ // Not processing the child which bubbles up the handling >+ nsCOMPtr<nsIDocShellTreeItem> subShellItem; >+ docShell->GetChildAt(counter, getter_AddRefs(subShellItem)); >+ if (aAccessKeyState == eAccessKeyProcessingUp && >+ subShellItem == aBubbledFrom) >+ continue; Although, not your fault, could you add {} here?

Comment on attachment 8662127 [details] [diff] [review] Part 2 - return a bool indicating if focus was changed from PerformAccessKey > /** > * The method focuses (or activates) element that accesskey is bound to. It is > * called when accesskey is activated. > * > * @param aKeyCausesActivation - if true then element should be activated > * @param aIsTrustedEvent - if true then event that is cause of accesskey > * execution is trusted. >+ * @return true if the focus was changed. > */ >- virtual void PerformAccesskey(bool aKeyCausesActivation, >+ virtual bool PerformAccesskey(bool aKeyCausesActivation, > bool aIsTrustedEvent) > { >+ return false; > } I don't know the exact rule of IID change, though, if this needs to change the IID of nsIContent and its derives classed, please change them. >+bool > HTMLLabelElement::PerformAccesskey(bool aKeyCausesActivation, > bool aIsTrustedEvent) > { > if (!aKeyCausesActivation) { > nsRefPtr<Element> element = GetLabeledElement(); >+ if (element) { >+ return element->PerformAccesskey(aKeyCausesActivation, aIsTrustedEvent); >+ } If <label> doesn't have labeled element, should we focus the label element? Because you will return true at the end of this method. > } else { > nsPresContext *presContext = GetPresContext(eForUncomposedDoc); >+ if (!presContext) { >+ return false; >+ } > > // Click on it if the users prefs indicate to do so. > WidgetMouseEvent event(aIsTrustedEvent, eMouseClick, > nullptr, WidgetMouseEvent::eReal); > event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_KEYBOARD; > > nsAutoPopupStatePusher popupStatePusher(aIsTrustedEvent ? > openAllowed : openAbused); > > EventDispatcher::Dispatch(static_cast<nsIContent*>(this), presContext, > &event); > } >+ >+ return true; So, shouldn't this be |return aKeyCausesActivation;|? >+bool > HTMLLegendElement::PerformAccesskey(bool aKeyCausesActivation, > bool aIsTrustedEvent) > { > // just use the same behaviour as the focus method > ErrorResult rv; > Focus(rv); >+ return true; Don't you need to check the rv is "succeeded"? >+bool > nsGenericHTMLElement::PerformAccesskey(bool aKeyCausesActivation, > bool aIsTrustedEvent) > { > nsPresContext* presContext = GetPresContext(eForUncomposedDoc); > if (!presContext) >+ return false; I hope you add {} here, but up to you. >@@ -2760,16 +2760,18 @@ nsGenericHTMLElement::PerformAccesskey(b > event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_KEYBOARD; > > nsAutoPopupStatePusher popupStatePusher(aIsTrustedEvent ? > openAllowed : openAbused); > > EventDispatcher::Dispatch(static_cast<nsIContent*>(this), > presContext, &event); > } >+ >+ return true; Should this be |return fm != nullptr;|? However, nsFocusManager::SetFocusInner() doesn't set focus to this content if CheckIfFocusable() returns nullptr. But this method cannot detect it... If my review is wrong, please mark this r? again with explaining why I'm wrong.

Comment on attachment 8662128 [details] [diff] [review] Part 3 - support accesskey handling in remote tabs >@@ -909,25 +910,39 @@ EventStateManager::ExecuteAccessKey(nsTA >+ bool focusChanged = false; >+ if (shouldActivate) { >+ focusChanged = content->PerformAccesskey(shouldActivate, aIsTrustedEvent); >+ } > else { nit: } else { >@@ -962,28 +977,53 @@ EventStateManager::GetAccessKeyLabelPref > aPrefix.Append(modifierText + separator); > } > if (modifierMask & NS_MODIFIER_SHIFT) { > nsContentUtils::GetShiftText(modifierText); > aPrefix.Append(modifierText + separator); > } > } > >+struct AccessKeyInfo { nit: put the { to the next line. >+ nsTArray<uint32_t>& charCodes; >+ bool isTrusted; >+ int32_t modifierMask; >+ >+ AccessKeyInfo(nsTArray<uint32_t>& aCharCodes, bool aIsTrusted, int32_t aModifierMask) >+ : charCodes(aCharCodes), isTrusted(aIsTrusted), modifierMask(aModifierMask) : charCodes(aCharCodes) , isTrusted(aIsTrusted) , modifierMask(aModifierMask) >+ { >+ } >+}; >+ >+static void >+HandleAccessKeyInRemoteChild(TabParent* aTabParent, void* aArg) >+{ >+ AccessKeyInfo* accessKeyInfo = static_cast<AccessKeyInfo *>(aArg); nit: space before * isn't necessary. > bool > EventStateManager::HandleAccessKey(nsPresContext* aPresContext, > nsTArray<uint32_t>& aAccessCharCodes, > bool aIsTrusted, > nsIDocShellTreeItem* aBubbledFrom, > ProcessingAccessKeyState aAccessKeyState, > int32_t aModifierMask) > { >+ EnsureDocument(mPresContext); > nsCOMPtr<nsIDocShell> docShell = aPresContext->GetDocShell(); >- >- if (!docShell) { >- NS_WARNING("no docShellTreeNode for presContext"); >+ if (!docShell || !mDocument) { >+ NS_WARNING("no docshell or document for presContext"); How about |if (NS_WARN_IF(!docShell) || NS_WARN_IF(!mDocument)) {|? Then, you can check which is nullptr. >+ // Now try remote children >+ if (mDocument && mDocument->GetWindow()) { >+ // If the focus is currently on a node with a TabParent, the key event will >+ // get forwarded to the child process and HandleAccessKey called from there. >+ // If focus is somewhere else, then we need to check the remote children. >+ nsFocusManager* fm = nsFocusManager::GetFocusManager(); >+ nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nullptr; >+ if (!TabParent::GetFrom(focusedContent)) { >+ AccessKeyInfo accessKeyInfo(aAccessCharCodes, aIsTrusted, aModifierMask); >+ nsContentUtils::CallOnAllRemoteChildren(mDocument->GetWindow(), >+ HandleAccessKeyInRemoteChild, &accessKeyInfo); >+ } >+ } Basically, I don't like trying remote children at last since it changes the priority of access key target. However, this must be reasonable for now. >+ /** >+ * A potentional accesskey was just pressed. Look for accesskey targets potential? > bool >+TabChild::RecvHandleAccessKey(nsTArray<uint32_t>&& aCharCodes, >+ const bool& aIsTrusted, >+ const int32_t& aModifierMask) >+{ >+ nsCOMPtr<nsIDocument> document(GetDocument()); >+ nsCOMPtr<nsIPresShell> presShell = document->GetShell(); >+ if (presShell) { >+ nsPresContext* pc = presShell->GetPresContext(); Don't you need to check if pc is nullptr? "Get" methods may return nullptr.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/07853d7b87c1ab9c8e210b096aa24790252a92bb changeset: 07853d7b87c1ab9c8e210b096aa24790252a92bb user: Neil Deakin <neil@mozilla.com> date: Fri Sep 18 08:18:07 2015 -0400 description: Bug 1168042, restructure HandleAccessKey so that accesskey candidates are only determined once, and clean up return value to use a bool, r=masayuki url: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a9162ec47badf22c806d758c8c3fc075663dda changeset: b3a9162ec47badf22c806d758c8c3fc075663dda user: Neil Deakin <neil@mozilla.com> date: Fri Sep 18 08:18:42 2015 -0400 description: Bug 1168042, return a bool from PerformAccessKey indicating if focus was changed, r=masayuki url: https://hg.mozilla.org/integration/mozilla-inbound/rev/30333d9cbcb9aaa4c38e92cb3a299bc81954fe6a changeset: 30333d9cbcb9aaa4c38e92cb3a299bc81954fe6a user: Neil Deakin <neil@mozilla.com> date: Fri Sep 18 08:19:13 2015 -0400 description: Bug 1168042, support accesskey redirecting to content process, r=masayuki url: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3cc0e12f418b59e74f112192a7d83cb948147dd changeset: d3cc0e12f418b59e74f112192a7d83cb948147dd user: Neil Deakin <neil@mozilla.com> date: Fri Sep 18 08:19:20 2015 -0400 description: Bug 1168042, content process access key tests, r=masayuki

https://hg.mozilla.org/mozilla-central/rev/07853d7b87c1 https://hg.mozilla.org/mozilla-central/rev/b3a9162ec47b https://hg.mozilla.org/mozilla-central/rev/30333d9cbcb9 https://hg.mozilla.org/mozilla-central/rev/d3cc0e12f418

I have reproduced this bug by following comment 0 with firefox 40.0a2 (Build ID:20150524004010; User agent:Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0) This bug is now verified as fixed on latest nightly 45.0a1 (Build ID:20151203053521; User agent:Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0)

Can you please verify on 43 as well ? https://archive.mozilla.org/pub/firefox/candidates/43.0b9-candidates/build2/