525856 - White-space in HTML affects how link receive focus from keyboard (tab key).

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[philippe (part-time)]

Attachment: test case 1 - works

OK, this is a little difficult to explain. I hope the test cases will help...
An in-page link (element A) points to an element B lower down in the page (target). Element B is an unordered list and contains links.
User clicks element A (or hit return after focussing the link). Before this bug, the first link in element B would then receive focus when the user next presses the 'tab' key. With trunk builds, depending on how the source code is written (white-space nodes), this will fail - the next tab key press goes back to the top of the page or to element A.

This works
<ul><li><a href>

But this fails
<ul>
<li>

Real world usage on one of my own sites: 
http://emps.l-c-n.com/development/phw_sandSpace
Load page, hit tab 2x, follow the link 'jump to navigation'. Hit 'tab' to access one of the links. I can't change the source code formatting of the target, that is what the CMS outputs :-(.

Regression range:
works
Gecko/20090623 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/28aa23105a9e

fails
Gecko/20090624 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/5fe89f2c22f0

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=28aa23105a9e&tochange=5fe89f2c22f0

bug 495385 or bug 478465 ??

Comment 1

[philippe (part-time)]

Attachment: test case 2 - fails

Comment 2

[Boris Zbarsky [:bzbarsky]]

So what exactly are the steps to reproduce the bug, presumably with "test case 2"?  That's something conspicuously missing from comment 0....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple self-explaining testcase showing the same problem going back to Fx3

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the particular testcase this bug was filed on is a regression from bug 495385, but the underlying problem is very old.  PresShell::GoToAnchor has:

    // Even if select anchor pref is false, we must still move the
    // caret there. That way tabbing will start from the new
    // location
...
        while (content && content->GetChildCount() > 0) {
          content = content->GetChildAt(0);
        }
        nsCOMPtr<nsIDOMNode> node(do_QueryInterface(content));
        NS_ASSERTION(node, "No nsIDOMNode for descendant of anchor");
        jumpToRange->SelectNodeContents(node);

So in this case selects the whitespace node.

Then later, nsFocusManager::GetNextTabbableContent gets the whitespace node as the start content and does:

    nsIFrame* aStartFrame = aPresShell->GetPrimaryFrameFor(aStartContent);
    if (!aStartFrame) {
      // if there is no frame, just get the root frame
      aStartFrame = aPresShell->GetPrimaryFrameFor(aRootContent);

My testcase shows how this can fail pretty simply even without the whitespace magic.

One or the other needs to change.  Either the presshell needs to look for something with a frame, or focus manager needs to keep walking forward from aStartContent until it finds a frame.  Or something.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix?

This fixes Boris's testcase. I wonder what Neil thinks of it.

If this behaviour looks reasonable then I'll write a mochitest. I think it's clearly at least as reasonable as just going directly up to the root.

Comment 6

[Neil Deakin]

Seems OK, but the current behaviour is what IE does, whereas this behaviour seems somewhat arbitrary. And for completeness, the Safari behaviour is to keep the focus on the content and tab moves to the next content in sequence.

Might be better to do something different only when the focus was determined from the selection, where it would be more logicial to do the Safari behaviour.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not really sure what you mean. Do you want to take this?

Comment 8

[Neil Deakin]

Attachment: something like this

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Could you explain what behavior the patch gives us -
and actually put that as a comment to nsFocusManager::GetNextTabbableContent?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 411686 [details] [diff] [review]
something like this

It looks like you're implementing what you described as the Safari behaviour, moving through the content when there's no frame. This seems a little weird to me, since for example it will skip XBL anonymous content if we start in a display:none node, but if we start in a node with a frame we will traverse XBL anonymous content. But I think it's acceptable.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 411686 [details] [diff] [review]
something like this

>diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp
>--- a/content/xul/content/src/nsXULElement.cpp
>+++ b/content/xul/content/src/nsXULElement.cpp
>@@ -575,18 +575,20 @@ nsXULElement::IsFocusable(PRInt32 *aTabI
>         // the element becomes focusable.
>         PRInt32 tabIndex = 0;
>         xulControl->GetTabIndex(&tabIndex);
>         shouldFocus = *aTabIndex >= 0 || tabIndex >= 0;
>         *aTabIndex = tabIndex;
>       }
>       else {
>         // otherwise, if there is no tabindex attribute, just use the value of
>-        // *aTabIndex to indicate focusability
>+        // *aTabIndex to indicate focusability. Reset any supplied tabindex to 0.
Please document somewhere why tabindex must be reset to 0 here.


>+  PRBool getNextFrame = PR_TRUE;
>+  nsCOMPtr<nsIContent> iterStartContent = aStartContent;
>   while (1) {
>-    nsIFrame* aStartFrame = aPresShell->GetPrimaryFrameFor(aStartContent);
>-    if (!aStartFrame) {
>-      // if there is no frame, just get the root frame
>-      aStartFrame = aPresShell->GetPrimaryFrameFor(aRootContent);
>-      if (!aStartFrame)
>+    nsIFrame* startFrame = aPresShell->GetPrimaryFrameFor(iterStartContent);
>+    // if there is no frame, look for another content node that has a frame
>+    if (!startFrame) {
>+      // if the root content doesn't have a frame, just return
>+      if (iterStartContent == aRootContent)
>         return NS_OK;
>-      aStartContent = aRootContent;
>+
>+      // look for the next or previous content node in tree order
>+      nsTreeWalker walker(aRootContent, nsIDOMNodeFilter::SHOW_ALL, nsnull, PR_TRUE);
>+      nsCOMPtr<nsIDOMNode> nextNode = do_QueryInterface(iterStartContent);
>+      walker.SetCurrentNode(nextNode);
>+      if (NS_SUCCEEDED(aForward ? walker.NextNode(getter_AddRefs(nextNode)) :
>+                                  walker.PreviousNode(getter_AddRefs(nextNode)))) {
>+        iterStartContent = do_QueryInterface(nextNode);
>+        // we've already skipped over the initial focused content, so we
>+        // don't want to traverse frames.
>+        getNextFrame = PR_FALSE;
>+        if (iterStartContent)
>+          continue;
>+      }
>+
>+      // otherwise, as a last attempt, just look at the root content
>+      iterStartContent = aRootContent;
>+      continue;
As I said before, please document somewhere how this all is
supposed to work.

And please file a followup to fix XBL handling.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

smaug, I believe Neil's gone on vacation. Would you mind addressing these comments and landing?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: added some comments

I'll push this ASAP

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/bacd85de3ef8

Will push to 1.9.2 once there is one successful chrome test build.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.9.2

!iterStartContent->IsHTML() -> !iterStartContent->IsNodeOfType(nsINode::eHTML)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/111fcb165546

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks a ton!!

Comment 18

[philippe (part-time)]

smaug & Neil: Thank you. Works great.

Comment 19

[philippe (part-time)]

Is there anything left to do here ? Or can this marked fixed ?