17605 - [TESTCASE] As without HREFs included in tab order, but shouldn't be

Status: VERIFIED FIXED

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

Description

[cpratt]

Build ID: 1999102908
Platform: Windows NT

To reproduce:
- Open the attached test case in the browser
- Click into the text input control
- Press Tab

Result: "SECTION B - etc." receives visual focus

Expected result: As "SECTION B" is not actually a hyperlink, but an anchor, it
should not receive focus in tab orders (because you can't 'do' anything with
it). This is the behavior in Nav 4.x and IE 5.x.

Comment 1

[cpratt]

Attachment: Test case for bug 17605

Comment 2

[rickg]

Eric -- do you own tab ordering?

Comment 3

[Eric Pollmann]

It's the tab manager.  I'll take a look.

Comment 4

[Eric Pollmann]

After careful consideration, I've decided that I probably won't get this bug in
for M12.  Currently I have nearly 50 bugs scheduled for M13, so there is a
possibility that this bug may need to be moved out farther still.

Comment 5

[ekrock's old account (dead)]

Moving to M15.

Comment 6

[Eric Pollmann]

Moving out to M17.

Comment 7

[Eric Pollmann]

I have a fix for this in my tree:

nsEventStateManager::GetNextTabbableContent
...
else if(nsHTMLAtoms::a==tag.get()) {
  nsCOMPtr<nsIDOMHTMLAnchorElement>nextAnchor(do_QueryInterface(child));
  if (nextAnchor)
    nextAnchor->GetTabIndex(&tabIndex);
    nsAutoString href;
    nextAnchor->GetHref(href);
    if (!href.Length()) {
      disabled = PR_TRUE; // Don't tab unless href, bug 17605
    } else {
      disabled = PR_FALSE;
    }
  }
...

Tom, can you code review this?

Comment 8

[Eric Pollmann]

Oops, sorry, typo:

nsEventStateManager::GetNextTabbableContent
...
else if(nsHTMLAtoms::a==tag.get()) {
  nsCOMPtr<nsIDOMHTMLAnchorElement>nextAnchor(do_QueryInterface(child));
  if (nextAnchor)
    nextAnchor->GetTabIndex(&tabIndex);
    nsAutoString href;
    nextAnchor->GetAttribute("href", href);
    if (!href.Length()) {
      disabled = PR_TRUE; // Don't tab unless href, bug 17605
    } else {
      disabled = PR_FALSE;
    }
  }
...

Tom, can you code review this updated fix?  Thanks!

Comment 9

[Eric Pollmann]

With scc's string changes, the line that changed above changes to this now:

nextAnchor->GetAttribute(NS_ConvertASCIItoUCS2("href"), href);

Comment 10

[Eric Pollmann]

Fix checked in.  To verify, see above test case.

Comment 11

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 12

[Loco]

Updating QA Contact.

Comment 13

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 14

[Madhur Bhatia]

QA contact updated

Comment 15

[luis rincon]

verified on build 2001-08-06-trunk