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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: philippe (part-time), Assigned: Neil Deakin)

Tracking

(Depends on: 1 bug, {regression})

Trunk
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(7 attachments)

(Reporter)

Description

8 years ago
Created attachment 409674 [details]
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 ??
Flags: wanted1.9.2?
Flags: blocking1.9.2?
(Reporter)

Comment 1

8 years ago
Created attachment 409675 [details]
test case 2 - fails
(Reporter)

Updated

8 years ago
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....
Created attachment 409689 [details]
Simple self-explaining testcase showing the same problem going back to Fx3
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.
Blocks: 495385
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Priority: -- → P2
Created attachment 411128 [details] [diff] [review]
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.
Attachment #411128 - Flags: review?(enndeakin)
(Assignee)

Comment 6

8 years ago
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)

Updated

8 years ago
Assignee: roc → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 years ago
Created attachment 411686 [details] [diff] [review]
something like this
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+
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.
(Assignee)

Updated

8 years ago
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?
Created attachment 413597 [details] [diff] [review]
for 1.9.2

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

Comment 18

8 years ago
smaug & Neil: Thank you. Works great.

Updated

8 years ago
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).
(Reporter)

Comment 19

8 years ago
Is there anything left to do here ? Or can this marked fixed ?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.