1132518 - [e10s] Document navigation with F6 doesn't work

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Neil Deakin]

It should cycle focus between documents/frames and the top-level window url field.

Comment 1

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

do we need this? how many people use it?

Comment 2

[Neil Deakin]

We've had this shortcut for a long time, so it's likely some are depending on it. Let's ask Marco if there is a specific accessibility need.

The issue here is that content document in a different process is skipped in F6 navigation.

Comment 3

[Marco Zehe (:MarcoZ)]

Yes, there are definitely implications here.

1. With all the one-off buttons appearing as soon as one types something into the search box, F6 is a quick way to get away from the address bar to the document.
2. F6 is also the way to go from the document to, say, the Hello pane. For keyboard users, there's no other way. Put away the mouse or disable the trackpad and try to navigate the Firefox UI with just the keyboard. F6 is very important here.
3. F6 is also used to find possible door hangers.

Comment 4

[Neil Deakin]

To clarify, the only aspect broken here is navigating from chrome to the content document and within frames of the content document. Navigating out to the chrome UI and between the various UI panels with F6 works fine.

Comment 5

[Marco Zehe (:MarcoZ)]

Yes, and that basically affects points 1 and 3 I made above. Point 1 with the one-off buttons, can take quite a number of Tab key presses before one reaches the document again.
Point 3 is important if you, say, logged into a site and want to check whether the login was successful before answering the door hanger password prompt. A blind user cannot simply skim the screen to see if the login succeeded, they have to go into the document to find out. the way to do this now is quickly use F6 and Shift+F6. That traverses into and out of the document from/to the browser chrome. If that gets broken, a lot more keystrokes are needed to achieve the same result.

Comment 7

[Neil Deakin]

Attachment: Part 1 - add a skip popup check flag

For document navigation, we need a flag to skip the popup frame checks.

Comment 8

[Neil Deakin]

Attachment: Part 2 - marge tab navigation and document navigation

Since we can't iterate over docshells in remote processes, I here instead merge the document navigation and tab navigation code and use the same iterating code for both, but skip non-documents for the former.

If a process boundary is found, we call into the other process to continue. There are several main differences:

1. MoveFocus can't return the content in another process, so it returns null.
2. We now use tab order instead of docshell order, which is more correct anyway.
3. The old code would navigate all panels in sequence then all child documents, whereas this code iterates them in tab order which could be panels and documents mixed together. This shouldn't be a problem in practise, but a later patch contains some fixups due to this.

Comment 9

[Neil Deakin]

Attachment: Part 3 - redirect focus to urlbar

MoveFocus can return null now, so we can't use the return value anymore. Instead I add an attribute to ensure that the urlbar is focused when the top-level window is focused. As a bonus, other windows could use this attribute as well.

Comment 10

[Neil Deakin]

Attachment: Part 4 - tests

The change to window_focus_docnav.xul is needed due to the taborder change mentioned in comment 8.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8614052 [details] [diff] [review]
Part 1 - add a skip popup check flag

> dom/base/nsFocusManager.cpp
>        false, // aVisual
>        false, // aLockInScrollView
>-       true      // aFollowOOFs
>+       true,  // aFollowOOFs
>+       false  // aSkipPopupChecks

OK since you're just adding yet another bool, but if this patch had
introduced all five of them I would ask that you use "uint32_t aFlags"
instead, together with an enum for the various flags.
Please file a follow-up bug to fix this later.

>layout/base/nsFrameTraversal.h	
>+                              bool aIncludePopups);

This should be named aSkipPopupChecks too?
(two places)

>layout/base/nsIFrameTraversal.h	
>+// {D33FE76C-207C-4359-A315-8EB1EECF80E5}
>+{ 0xd33fe76c, 0x207c, 0x4359, { 0xa3, 0x15, 0x8e, 0xb1, 0xee, 0xcf, 0x80, 0xe5 } }

I'd prefer if the first line also use lower-case for consistency.

(don't forget to add a commit message)

Comment 12

[Neil Deakin]

> OK since you're just adding yet another bool, but if this patch had
> introduced all five of them I would ask that you use "uint32_t aFlags"
> instead, together with an enum for the various flags.
> Please file a follow-up bug to fix this later.
> 

Looks like 779684 already tried that but was backed out.

Comment 13

[Neil Deakin]

The test depends on bug 1170166.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8614063 [details] [diff] [review]
Part 3 - redirect focus to urlbar

>--- a/browser/components/downloads/content/downloadsOverlay.xul
>+++ b/browser/components/downloads/content/downloadsOverlay.xul
>@@ -34,17 +34,17 @@
>     <command id="downloadsCmd_openReferrer"
>              oncommand="goDoCommand('downloadsCmd_openReferrer')"/>
>     <command id="downloadsCmd_copyLocation"
>              oncommand="goDoCommand('downloadsCmd_copyLocation')"/>
>     <command id="downloadsCmd_clearList"
>              oncommand="goDoCommand('downloadsCmd_clearList')"/>
>   </commandset>
> 
>-  <popupset>
>+  <popupset id="mainPopupSet">

What does this have to do with this bug? Please remove or explain this...

Comment 15

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8614063 [details] [diff] [review]
Part 3 - redirect focus to urlbar

>-  <popupset>
>+  <popupset id="mainPopupSet">
unrelated change?

+  // get checked as it will be skipped when the focus is rerargetted to it.
rerargetted?

>+    rootContent->GetAttr(kNameSpaceID_None, nsGkAtoms::retargetdocumentfocus, retarget);
>+    if (!retarget.IsEmpty()) {
Perhaps write this

if (rootContent->GetAttr(kNameSpaceID_None,
                         nsGkAtoms::retargetdocumentfocus,
                         retarget)) {
  nsIContent* retargetElement = ...
  ...
}
GetAttr return true, if the attribute is there.


>+      nsIContent* retargetElement = doc->GetElementById(retarget);
>+      // Check both the element and its binding parent, since the common case here
>+      // is the urlbar where focus is on the anonymous input inside the textbox,
>+      // but the retargetdocumentfocus attribute refers to the textbox.
>+      if (retargetElement && (retargetElement == startContent ||
>+                              retargetElement == startContent->GetBindingParent())) {
This feels a bit hackish. Could we be more generic, something like
retargetElement == startContent ||
(!retargetElement->Contains(startContent) &&
 nsContentUtils::ContentIsDescendantOf(startContent, retargetElement))

retargetElement->Contains(startContent) returns false if the nodes don't have the same binding parent
but nsContentUtils::ContentIsDescendantOf just traverses parent chain, so startContent should be a descendant of
retargetElement in case of urlbar


>+      // If the redirectdocumentfocus attribute is set, redirect the focus to a
>+      // specific element. This is primarily used to retarget the focus to the
>+      // urlbar during document navigation.
>+      nsAutoString retarget;
>+      aRootContent->GetAttr(kNameSpaceID_None, nsGkAtoms::retargetdocumentfocus, retarget);
>+      if (!retarget.IsEmpty()) {
also here I'd prefer using the return value of GetAttr and not the emptyness of attribute value


>+        nsIContent* retargetElement = CheckIfFocusable(doc->GetElementById(retarget), 0);
>+        if (retargetElement) {
>+          NS_ADDREF(*aNextContent = retargetElement);
>+          return NS_OK;
nsCOMPtr<nsIContent> retargetElement = CheckIfFocusable...
if (retargetElement) {
  retargetElement.forget(aNextContent);
  return NS_OK;
}
would be a bit modern way to do this.

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8614057 [details] [diff] [review]
Part 2 - marge tab navigation and document navigation


>+          if (validPopup) {
>+            // Find the first focusable content within the popup. If there isn't
>+            // any focusable content in the popup, look for another element.
>+            rv = GetNextTabbableContent(aPresShell, currentContent,
>+                                        nullptr, currentContent,
>+                                        true, 1, false, false,
>+                                        aResultContent);
Why true, why not aForward?
But I don't understand this behavior anyhow.
Why we don't pass aForDocumentNavigation as the latter false?

I think I'd like to see a -w version of this patch. There is significant amount code which is just re-indented


>+nsIContent*
>+nsFocusManager::GetRootForDocumentNavigation(nsIContent* aContent, TabParent** aRedirectedToTabParent)
>+{
>+  *aRedirectedToTabParent = nullptr;
>+
>+  if (!aContent ||
>+      !(aContent->IsXULElement(nsGkAtoms::browser) ||
>+        aContent->IsXULElement(nsGkAtoms::editor) ||
>+        aContent->IsHTMLElement(nsGkAtoms::frame))) {
I don't understand this. Why we check for xul:browser, xul:editor, but not xul:iframe, yet we check for
html:frame, but not html:iframe or html:object



(this is rather tricky to review, since so much code and behavior moves around)

Comment 17

[Neil Deakin]

(In reply to Dão Gottwald [:dao] from comment #14)
> > 
> >-  <popupset>
> >+  <popupset id="mainPopupSet">
> 
> What does this have to do with this bug? Please remove or explain this...

Sorry, I meant to add a comment to the bug when adding the patch. As described in comment 8, panels are now navigated in tab document order instead of all being navigated first as the old code did. The change here ensures that the downloads popup from the overlay is added into the mainPopupSet (and thus near the beginning of the document) instead of being appended at the end of the document, which would have made it appear at the end of the tab order.

Comment 18

[Neil Deakin]

> >+          if (validPopup) {
> >+            // Find the first focusable content within the popup. If there isn't
> >+            // any focusable content in the popup, look for another element.
> >+            rv = GetNextTabbableContent(aPresShell, currentContent,
> >+                                        nullptr, currentContent,
> >+                                        true, 1, false, false,
> >+                                        aResultContent);
> Why true, why not aForward?
> But I don't understand this behavior anyhow.
> Why we don't pass aForDocumentNavigation as the latter false?

At this point, we've decided that a panel is the next 'frame' to focus. Since, unlike documents, panels aren't focusable themselves, we use regular tab navigation to determine the first focusable child within the panel and focus that instead.


> >+  if (!aContent ||
> >+      !(aContent->IsXULElement(nsGkAtoms::browser) ||
> >+        aContent->IsXULElement(nsGkAtoms::editor) ||
> >+        aContent->IsHTMLElement(nsGkAtoms::frame))) {
> I don't understand this. Why we check for xul:browser, xul:editor, but not
> xul:iframe, yet we check for
> html:frame, but not html:iframe or html:object

Don't know as this is what the original code did (written in 2001). I suspect that it was assumed that iframes were used for things that were more integral to the page (or were just ads) as opposed to being considered a separate frame. IE does the same.

Comment 19

[Neil Deakin]

Attachment: docnav.txt

Part 2 with -w

Comment 20

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Neil Deakin from comment #18)

> > >+  if (!aContent ||
> > >+      !(aContent->IsXULElement(nsGkAtoms::browser) ||
> > >+        aContent->IsXULElement(nsGkAtoms::editor) ||
> > >+        aContent->IsHTMLElement(nsGkAtoms::frame))) {
> > I don't understand this. Why we check for xul:browser, xul:editor, but not
> > xul:iframe, yet we check for
> > html:frame, but not html:iframe or html:object
> 
> Don't know as this is what the original code did (written in 2001). I
> suspect that it was assumed that iframes were used for things that were more
> integral to the page (or were just ads) as opposed to being considered a
> separate frame. IE does the same.
But where is the original code? I don't see anything doing exactly that check

Comment 21

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Oh, there in nsFocusManager::GetRootForFocus. Well, that checks explicitly for iframe, but doesn't for object.
I guess using object as an iframe is rather rare.

Comment 22

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8615752 [details] [diff] [review]
docnav.txt

> nsFocusManager::DetermineElementToMoveFocus(nsPIDOMWindow* aWindow,
>                                             nsIContent* aStartContent,
>                                             int32_t aType, bool aNoParentTraversal,
>                                             nsIContent** aNextContent)
> {
>   *aNextContent = nullptr;
> 
>-  nsCOMPtr<nsIDocShell> docShell = aWindow->GetDocShell();
>-  if (!docShell)
>-    return NS_OK;
So we hopefully move focus outside the window if it doesn't have docshell anymore.
At least we shouldn't move focus within such window. MOVEFOCUS_FIRSTDOC is transformed to MOVEFOCUS_FORWARDDOC
which is fine. 


>-  if (aType == MOVEFOCUS_FORWARDDOC) {
>-    NS_IF_ADDREF(*aNextContent = GetNextTabbableDocument(startContent, true));
>-    return NS_OK;
>-  }
>-  if (aType == MOVEFOCUS_BACKWARDDOC) {
>-    NS_IF_ADDREF(*aNextContent = GetNextTabbableDocument(startContent, false));
>-    return NS_OK;
>   }
uh, this is hard to review. So much somewhat unrelated code moves within one patch.
Continuing from here....

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

I'll continue today... I've looked at the patch about 5 times and I have still no idea whether it might
regress some behavior.

Comment 24

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8615752 [details] [diff] [review]
docnav.txt


>         }
>         else if (aIgnoreTabIndex || aCurrentTabIndex == tabIndex) {
>           // break out if we've wrapped around to the start again.
>           if (aOriginalStartContent && currentContent == aOriginalStartContent) {
>             NS_ADDREF(*aResultContent = currentContent);
>             return NS_OK;
>           }
> 
>+          bool checkSubDocument = true;
>+          if (aForDocumentNavigation) {
So I have no idea why this leads to the same behavior as the previous docshell tree based navigation.
Please explain a bit.



>+   * Retrieves and returns the root node as with GetRootForFocus but for
>+   * document navigation. If aRedirectToTabParent is assigned, then the request
>+   * should be redirected to that TabParent.
>    */
So what is actually returned?
And since the param needs to be apparently xul:browser, xul:editor or html:frame, the method name
GetRootForDocumentNavigation sounds too generic.



Sorry, I need to go through this method by method, or more like line by line. This is super hard to review.

Comment 25

[Neil Deakin]

The F6 key needs to navigate between:

1. The top-level window, by focusing the urlbar, or, for non-browser windows such as dialogs or Thunderbird, the first focusable content within the window.
2. The content window, by focusing the root element of the window.
3. The first focusable element in any open sidebars or other <browser> elements that may be visible.
4. The first focusable element in any open <panel>s.
5. If a page contains a frameset, the root element of each frame within the frameset.

We treat <editor> as similar to <browser>.

I've tested these cases with the automated tests (part 4) as well as manually. I don't think it matters too much what the old code did, does it? The new code uses the same algorithm as regular tab navigation (contained mostly in GetNextTabbableContent), but skips over things that are not frames/browsers/panels/etc.

> >+          bool checkSubDocument = true;
> >+          if (aForDocumentNavigation) {
> So I have no idea why this leads to the same behavior as the previous
> docshell tree based navigation.
> Please explain a bit.
> 

The main loop here iterates over each nsIFrame in a document, starting from the current focused element, or, if nothing is focused, the node for the selection if one is present.  When navigation reaches the end, it starts again with the next tabindex. This process continues until no more tabindicies are found, at which point it starts again from the first tabindex. Once we reach the original tabindex and original starting content, then we don't find anything to navigate to and just return the same thing again.

The GetRootForDocumentNavigation is intended to be the function that filters out the things that are not those listed in the 5 points above, returning null if not accepted, or a content node to focus if acceptable. I agree that it should have a better name, or perhaps the code should just be included inline.

Comment 26

[Neil Deakin]

Olli, were you continuing to review this? Did you have more questions, or was there something you wanted me to update?

Comment 27

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8615752 [details] [diff] [review]
docnav.txt

I guess I can put it back to my queue, but if you can think of ways to split this
to smaller pieces that would be great.
(but perhaps there aren't too good ways to split this.)

Comment 28

[Neil Deakin]

Attachment: Part 2 - merge tab navigation and document navigation, v2

I'm not sure how to break this up. I could move the non-focusmanager changes out but I don't think that would make it easier to review.

This patch is similar but adds more documentation and improves and renames the GetRootForDocumentNavigation portion.

Comment 29

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

This is still in my review queue. I just need to find couple of days time to review this all. Maybe during Whistler, or right after that.

Comment 30

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8624240 [details] [diff] [review]
Part 2 - merge tab navigation and document navigation, v2

This patch seems to be created without diff -p

Comment 31

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8624240 [details] [diff] [review]
Part 2 - merge tab navigation and document navigation, v2

>+      nsIFrame* frame = startContent->GetPrimaryFrame();
>+      if (!frame)
>+        return NS_OK;
Always {} with if, please

>                 if (subframe) {
>-                  // If the subframe body is editable by contenteditable,
>-                  // we should set the editor's root element rather than the
>-                  // actual root element.  Otherwise, we should set the focus
>-                  // to the root content.
>-                  *aResultContent =
>-                    nsLayoutUtils::GetEditableRootContentByContentEditable(subdoc);
>-                  if (!*aResultContent ||
>-                      !((*aResultContent)->GetPrimaryFrame())) {
>-                    *aResultContent =
>-                      GetRootForFocus(subframe, subdoc, false, true);
>-                  }
>+                  *aResultContent = GetRootForFocus(subframe, subdoc, true, true);
Ah, contenteditable check is moved into GetRootForFocus.
So all but this one case where GetRootForFocus is used may change behavior.
Did you check it is always the right thing to do?

> nsFocusManager::GetRootForFocus(nsPIDOMWindow* aWindow,
>                                 nsIDocument* aDocument,
>-                                bool aIsForDocNavigation,
>+                                bool aDoChromeCheck,
>                                 bool aCheckVisibility)
Ok, so in practice if old code passed true as 3rd param, new code should pass false.



>   // Finally, check if this is a frameset
>   nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(aDocument);
>-  if (htmlDoc && aDocument->GetHtmlChildElement(nsGkAtoms::frameset)) {
>+  if (htmlDoc) {
>+    nsIContent* htmlChild = aDocument->GetHtmlChildElement(nsGkAtoms::frameset);
>+    if (htmlChild) {
>+      return aDoChromeCheck ? nullptr : htmlChild;
Ok, so we used to return null always if there was frameset, but now only with aDoChromeCheck. Why aDoChromeCheck is special here?

Comment 32

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(close to r+, but I'd like to get those couple of explanations at least)

Comment 33

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #31)
> Ah, contenteditable check is moved into GetRootForFocus.
> So all but this one case where GetRootForFocus is used may change behavior.
> Did you check it is always the right thing to do?
> 

I think so. The contenteditable check just redirects the focus to the body if it has contenteditable, treating the body as the root element instead of html. The new behaviour makes this more consistent, rather than only doing this in specific cases.


> >   // Finally, check if this is a frameset
> >   nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(aDocument);
> >-  if (htmlDoc && aDocument->GetHtmlChildElement(nsGkAtoms::frameset)) {
> >+  if (htmlDoc) {
> >+    nsIContent* htmlChild = aDocument->GetHtmlChildElement(nsGkAtoms::frameset);
> >+    if (htmlChild) {
> >+      return aDoChromeCheck ? nullptr : htmlChild;
> Ok, so we used to return null always if there was frameset, but now only
> with aDoChromeCheck. Why aDoChromeCheck is special here?

The aDoChromeCheck is only false when 'forDocumentNavigation' is true. Looking at this again, it would be clearer if I just left the argument as 'aIsForDocNavigation' and updated this code accordingly. When GetRootForChildDocument is called, the frameset will be returned; a special check is done for this and checkSubDocument remains set to true, continuing navigation into the frameset children.

Comment 34

[Neil Deakin]

Attachment: Part 2 - merge tab navigation and document navigation, v3

OK, I spent some time analyzing the callers of GetRootForFocus and I think they are all ok. This patch is just some polish.

Comment 35

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8632167 [details] [diff] [review]
Part 2 - merge tab navigation and document navigation, v3

This is of course rather scary change, so better get some testing on trunk.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4753659cf72
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5cb1d5802e
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f44cd6e7f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d58b43abf

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/c4753659cf72
https://hg.mozilla.org/mozilla-central/rev/cf5cb1d5802e
https://hg.mozilla.org/mozilla-central/rev/87f44cd6e7f0
https://hg.mozilla.org/mozilla-central/rev/dc9d58b43abf