1168042 - [e10s] accesskeys in content do not work when chrome is focused

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, defect)

Description

[Minh Nguyễn]

Attachment: Textarea with accesskey "e"

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.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 3

[:Gijs (he/him)]

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.

Comment 4

[Marco Zehe (:MarcoZ)]

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. ;)

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

(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?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(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?

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(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.

Comment 8

[Minh Nguyễn]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 10

[Neil Deakin]

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.

Comment 12

[Neil Deakin]

Attachment: Forward accesskey handling to 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 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 15

[Neil Deakin]

(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.

Comment 16

[Neil Deakin]

Attachment: Part 1 - restructure HandleAccessKey

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

Comment 17

[Neil Deakin]

Attachment: Part 2 - return a bool indicating if focus was changed from PerformAccessKey

Comment 18

[Neil Deakin]

Attachment: Part 3 - support accesskey handling in remote tabs

Comment 19

[Neil Deakin]

Attachment: Part 4 - accesskey test

Comment 20

[Neil Deakin]

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 24

[Neil Deakin]

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

Comment 25

[Neil Deakin]

Attachment: Part 2.2 - return a bool indicating if focus was changed from PerformAccessKey

Comment 26

[Neil Deakin]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/07853d7b87c1ab9c8e210b096aa24790252a92bb
changeset:  07853d7b87c1ab9c8e210b096aa24790252a92bb
user:       Neil Deakin <[e-mail]>
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 <[e-mail]>
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 <[e-mail]>
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 <[e-mail]>
date:       Fri Sep 18 08:19:20 2015 -0400
description:
Bug 1168042, content process access key tests, r=masayuki

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 28

[mubinarahamanjerin]

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)

Comment 29

[Paul Silaghi, QA [:pauly]]

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