Closed
Bug 382192
Opened 17 years ago
Closed 15 years ago
If page starts with a link, Tab skips the first link
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: uriber)
References
(Blocks 2 open bugs, )
Details
(Keywords: access, regression, testcase)
Attachments
(4 files, 1 obsolete file)
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.
Flags: blocking1.9?
Assignee | ||
Comment 1•17 years ago
|
||
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.
Assignee | ||
Comment 2•17 years ago
|
||
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•17 years ago
|
||
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?
Attachment #267019 -
Flags: superreview+
Attachment #267019 -
Flags: review?(mats.palmgren)
Attachment #267019 -
Flags: review?(bzbarsky)
Comment 4•17 years ago
|
||
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 ...
Attachment #267019 -
Flags: review?(mats.palmgren) → review-
Comment 5•17 years ago
|
||
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
Assignee | ||
Comment 6•17 years ago
|
||
(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.
Assignee | ||
Comment 7•17 years ago
|
||
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?
Attachment #267019 -
Attachment is obsolete: true
Attachment #274777 -
Flags: review?(mats.palmgren)
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 8•17 years ago
|
||
What I described in comment 6 (and implemented in Patch v2) is actually the fix for bug 376369.
Depends on: 376369
Updated•17 years ago
|
Priority: -- → P1
Comment 9•17 years ago
|
||
Mats, any chance you will get to this review soon?
Updated•17 years ago
|
Whiteboard: [has-patch]
Not holding beta, but please try to get the patch landed of course.
Priority: P1 → P3
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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.
Attachment #274777 -
Flags: review?(mats.palmgren) → review-
Comment 13•17 years ago
|
||
Here we should focus the ancestor DIV, not the first link.
Updated•17 years ago
|
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Comment 15•17 years ago
|
||
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...
Flags: wanted-next+
Flags: blocking1.9-
Flags: blocking1.9+
Comment 16•17 years ago
|
||
I think we really should backout bug 144000, if possible. That one caused several regressions, of which still 2 blockers are open.
Lets take that discussion over to bug 144000
Reporter | ||
Comment 18•15 years ago
|
||
This was fixed by backing out bug 144000.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
No longer depends on: 376369
Priority: P2 → --
Resolution: --- → FIXED
Whiteboard: [has-patch]
Updated•5 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•