382192 - If page starts with a link, Tab skips the first link

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a5pre) Gecko/20070527 Minefield/3.0a5pre

Steps to reproduce:
1. Load the testcase (or http://www.squarefree.com/shell/).
2. Press tab.

Result: the second link becomes focused.

Expected: the first link should become focused.

Regressed between 2007-04-14 and 2007-04-15 Firefox nightlies.  I suspect bug 144000.

Comment 1

[Uri Bernstein]

Attachment: patch v1

The problem here is that the (collapsed) selection is already inside the link, but the link isn't focused. The current code assumes that if the selection is inside a focusable element, then the element is focused, and we need to look for the next focused element.

This patch adds special handling for the case where there is no focused element. In this case, we first try to see if the element containing the selection, or one of its ancestors, is focusable - and if so, focus it.

This also changes the behavior in the following case: If a link is partially selected (e.g., by starting to drag from before the link into the link), pressing tab will focus the partially-selected link, rather than the next focusable element (as it does now). The new behavior is consistent with IE, and seems more correct to me anyway.

Comment 2

[Uri Bernstein]

Also note that there was a bug here even before the fix for bug 144000: If you clicked on the page margin to the left of "First link", and then pressed tab, the second link would be focused.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 267019 [details] [diff] [review]
patch v1

I can sr this, but I can't review this code -- I don't know it nearly well enough...

Mats, could you look at this?

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 267019 [details] [diff] [review]
patch v1

This patch regresses the attached testcase.
Tabbing from "second link" should focus the location bar,
but instead it focuses the document so you have a loop:
1 - 2 - document - 1 ...

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Another regression with "patch v1":
1. turn on caret navigation
2. load Testcase #2
3. navigate the caret using arrow keys to the text "1" or "2" that is
   inside a blue blue box but outside a text control
4. TAB  ==> focus goes to the enclosing blue box

Comment 6

[Uri Bernstein]

(In reply to comment #5)

Shouldn't navigating the caret into a focusable element focus that element (the same way that arrowing into a link focuses the link)?
I think that implementing this behavior would correct the regression.

I'll look into the other regression (and the other bug you filed) later today.

Comment 7

[Uri Bernstein]

Attachment: patch v2

Sorry for taking forever to get back to this.

This patch fixes the regression described in comment #4 by applying the new logic only if there is no current focus, and no starting element.
It fixes the regression described in comment #5 by implementing what I suggested in comment #6. Aaron - is there a reason this wasn't done originally?

Comment 8

[Uri Bernstein]

What I described in comment 6 (and implemented in Patch v2) is actually the fix for bug 376369.

Comment 9

[Robert Sayre]

Mats, any chance you will get to this review soon?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not holding beta, but please try to get the patch landed of course.

Comment 11

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

The patch changes the Bug 383109 behavior so that tabindexes are traversed: 0, (chrome), 1, 2. So still not right, but perhaps after this bug fixing bug 383109 gets easier. The patch applies with --fuzz=6 and one PR_FALSE is need as a
parameter.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 274777 [details] [diff] [review]
patch v2

Bug 144000 added this code to setup initial caret position:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=3.1074&root=/cvsroot&mark=4266#4226
CompleteMoveInner() leads to DrillDownToSelectionFrame() to find the
frame, which is rather aggressive in its drilling:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrame.cpp&rev=3.766&root=/cvsroot&mark=2378,2382,2401#2344
the result is that the caret ends up inside the link.

Given this position, I think the tabbing is actually correct -
we *should* get to the second link from there.
However, there is some code in ESM that used to account for this
specific case:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/events/src/nsEventStateManager.cpp&rev=1.720&root=/cvsroot&mark=3491-3492#3438
but the new initial caret position effectively disabled it.

AFAICT, the added code GetNextTabbableContent() isn't needed if we
can detect the "initial caret" in ESM as we used to.
I've added a patch in bug 383109 that I think could work.

There are a couple of other problems with this patch.
It chooses the nearest ancestor, which probably would work for most
real world pages, but is incorrect in some edge cases
(I'll attach a testcase to demonstrate).

Also, this patch fixes bug 376369 as you said, but there seems to
also introduce a new problem: once you navigate into an <input>
for example, it's not possible to navigate out again using just
the arrow keys. I guess this has something to do with focusing
the input but I haven't digged deeper into it (yet).

I like the way ancestors are focused when the caret moves around though,
so the improved "focusability tests" in MoveFocusToCaret() is something
I think we should take, once we can solve the caret navigation problem.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase, edge case

Here we should focus the ancestor DIV, not the first link.

Comment 14

[Boris Zbarsky [:bzbarsky]]

We should make sure we get in tests for this.

Comment 15

[Mike Schroepfer]

Mats, Boriz, Uri - we've had no activity on this for 5+ months, no dupes and we are past code freeze.  So I'm going to bump this to next release.  Please re-nom/yell if you disagree...

Comment 16

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

I think we really should backout bug 144000, if possible.
That one caused several regressions, of which still 2 blockers are open.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lets take that discussion over to bug 144000

Comment 18

[Jesse Ruderman]

This was fixed by backing out bug 144000.