Closed Bug 525856 Opened 13 years ago Closed 13 years ago

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


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




Tracking Status
status1.9.2 --- beta4-fixed


(Reporter: phiw2, Assigned: enndeakin)


(Depends on 1 open bug)


(Keywords: regression)


(7 files)

Attached file 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

Real world usage on one of my own sites:
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:
Gecko/20090623 Minefield/3.6a1pre

Gecko/20090624 Minefield/3.6a1pre

bug 495385 or bug 478465 ??
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Attached file test case 2 - fails
OS: Mac OS X → All
So what exactly are the steps to reproduce the bug, presumably with "test case 2"?  That's something conspicuously missing from comment 0....
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");

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.
Blocks: 495385
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Priority: -- → P2
Attached patch fix?Splinter Review
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.
Attachment #411128 - Flags: review?(enndeakin)
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.
I'm not really sure what you mean. Do you want to take this?
Assignee: roc → enndeakin
Attachment #411686 - Flags: superreview?(roc)
Attachment #411686 - Flags: review?(Olli.Pettay)
Could you explain what behavior the patch gives us -
and actually put that as a comment to nsFocusManager::GetNextTabbableContent?
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.
Attachment #411686 - Flags: superreview?(roc) → superreview+
Attachment #411686 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #411128 - Flags: review?(enndeakin)
Whiteboard: [needs review] → [needs landing]
smaug, I believe Neil's gone on vacation. Would you mind addressing these comments and landing?
I'll push this ASAP

Will push to 1.9.2 once there is one successful chrome test build.
Attached patch for 1.9.2Splinter Review
!iterStartContent->IsHTML() -> !iterStartContent->IsNodeOfType(nsINode::eHTML)
smaug & Neil: Thank you. Works great.
Summary: White-space in source code affects how link receive focus from keyboard (tab key). → White-space in HTML affects how link receive focus from keyboard (tab key).
Is there anything left to do here ? Or can this marked fixed ?
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.