Closed Bug 242829 Opened 21 years ago Closed 3 months ago

hyperlinks and form controls in a <caption> can't be tabbed to

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
140 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- wontfix
firefox55 --- wontfix
firefox109 --- wontfix
firefox137 --- wontfix
firefox138 --- wontfix
firefox139 --- wontfix
firefox140 --- fixed

People

(Reporter: sophia, Assigned: emilio)

References

(Blocks 5 open bugs)

Details

(Keywords: access)

Attachments

(2 files, 6 obsolete files)

Test case
471 bytes, text/html
Details
Bug 242829 - Remove table caption list. r=#layout!
48 bytes, text/x-phabricator-request
Details | Review
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040506 Firefox/0.8.0+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040506 Firefox/0.8.0+ Hyperlink text that is contained in a <caption> tag of a <table> cannot be tabbed to. When you tab to links, the links in the <caption> are skipped over. Reproducible: Always Steps to Reproduce: 1. Push the tab key to tab between links. Actual Results: links in a <caption> are skipped Expected Results: tab focus should land on links in a <caption>
Confirm NEW, 20040503 PC/Win2000
Assignee: firefox → aaronleventhal
Status: UNCONFIRMED → NEW
Component: General → Keyboard: Navigation
Ever confirmed: true
Keywords: access
OS: MacOS X → All
Product: Firefox → Browser
Hardware: Macintosh → All
Version: unspecified → Trunk
Blocks: focusnav
Severity: major → normal
Keywords: sec508
Target Milestone: --- → Future
Attached file Test case —
Severity: normal → minor
Target Milestone: Future → mozilla1.9beta
*** Bug 314851 has been marked as a duplicate of this bug. ***
Summary: hyperlinks in a <caption> of a <table> can't be tabbed to → hyperlinks and form controls in a <caption> can't be tabbed to
Assignee: aaronleventhal → mats.palmgren
QA Contact: keyboard.navigation
Attached patch fix (obsolete) — — Splinter Review
This is not the most beautiful solution but it seems to fix the problem at hand. I'm basically just hard-coding some table frame relations into the nsFrameTraversal methods. That is, * Next() at the end of a captionList -> go to the first frame of the principal list of the outerTableFrame, ie. the tableFrame. * Prev() at start of a tableFrame -> end of the captionList * GetFirstChild() of a tableOuterFrame -> start of captionList I don't see an alternative, other than nuking nsIFrameTraversal from orbit. Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c7819f083ae
Attachment #711090 - Flags: review?(dholbert)
Attached patch mochitest (obsolete) — — Splinter Review
Starting at document.body and tabbing forward N steps, then after tabbing backwards N steps we end up at <html>. That's odd. Anyway, that happens also without the patch.
A few observations from playing w/ the mochitest, in a patched build: (a) If I add additional <caption> elements, they don't render, but they *do* seem to get tab-focused. That seems potentially-wrong... (b) RE iteration order -- we visit <thead> and <tfoot> elements' children in DOM order, *not* in rendering order. (So if I have <table><tfoot/><tbody/><thead/> w/ links in each, then the thead is rendered first but visited last by tab-iteration.) <caption>, in contrast, seems to always be visited first, regardless of where it appears in the DOM. Is that intentional? (Tangent: do we still need kFrameTraversalCID / nsIFrameTraversal at all anymore? Seems like everyone who uses that interface could just as easily be dealing with a concrete nsFrameTraversal. Anyway -- if the interface & CID aren't needed, as I suspect, then I'll file a followup on ditching it.)
For completeness, it'd be great if the mochitest (or an additional more-complex mochitest) exercised the situations in comment 8, whatever behavior we end up with. In particular, it'd be great to have these situations tested: - multiple caption elements - thead/tfoot, maybe in the wrong order and/or duplicated
Attached patch mochitest, v2 (obsolete) — — Splinter Review
Thanks for a thorough review. Those are all valid points and I have added out-of-order tfoot/thead and multiple captions to the test. (a) this is bug 144517; I think I prefer to keep the patch as is (tabbing through them all) even if they are not visually present. An AT should pick them up and read them. (b) I think the convention so far has been "frame order" which in most cases reflects DOM order. <caption> is special since it's on its own frame list - I don't know what the "correct" order should be (the spec is vague on the matter[1]), so I'm happy with the behavior the patch gives (caption(s) are visited before the inner table frames). It will be correct most of the time, and doing something more advanced would require an overhaul of nsFrameTraversal which seems overkill for this bug. I'd rather remove this code and rewrite it from scratch. I agree the tab order of thead/tbody/tfoot is wrong when the DOM order isn't the visual order, but that's not this bug. (It seems to me that both DOM/frame order is wrong - I think users expects links to be visited in visual order (unless @tabindex says otherwise), taking into account inline/block direction of course) I see no reason to keep kFrameTraversalCID / nsIFrameTraversal, but I know very little about which binary APIs we need to keep and which we can remove. [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#sequential-focus-navigation-and-the-tabindex-attribute
Attachment #711124 - Attachment is obsolete: true
Comment on attachment 713744 [details] [diff] [review] mochitest, v2 >+function runTest() >+{ >+ is(document.activeElement, document.body, "focus in BODY before"); [...] >+ // Hmm, why do we end up at HTMLElement when tabbing back??? >+ // synthesizeKey("VK_TAB", { shiftKey: true }); >+ // is(document.activeElement, document.body, "focus in BODY after"); Perhaps worth using todo_is() at the end there, instead of a commented-out is() -- that way, if we fix this bug, we'll notice the behavior change and we'll know to remove the "Hmm" comment. (might be worth filing a bug on this behavior, too...) You could also add an "is" statement to check the _actual_ node we end up having focused, even if it's technically wrong. (document.body.parentNode, I believe) (Sorry for delay in review on the actual code -- I'm trying to be sure I understand all the contextual code, and I also figured this was low-ish priority since it's been open forever and ever, so I prioritized some other reviews first. :) I'll post review feedback on the code here tonight or tomorrow, though.)
So my first impression from the actual-patch: it's a bit unfortunate (and confusing) to have three layers of GetFirstChild / GetLastChild (a single-argument version, a two-argument-version with the same function-name, and an "inner" version -- where one calls another which calls another). I'd love to avoid that, if possible, perhaps with some clarifying function-renaming. I don't have any concrete suggestions yet, though.
Comment on attachment 711090 [details] [diff] [review] fix >+ nsIFrame* GetFirstChild(nsIFrame* aFrame, >+ nsIFrame::ChildListID aListID); > nsIFrame* GetFirstChild(nsIFrame* aFrame); [...] >+ virtual nsIFrame* GetFirstChildInner(nsIFrame* aFrame, >+ nsIFrame::ChildListID aListID); How about we add "FromList" to the functions that take a list ID? So that'd leave us with: nsIFrame* GetFirstChild(nsIFrame* aFrame); ...which calls... nsIFrame* GetFirstChildFromList(nsIFrame* aFrame, nsIFrame::ChildListID aListID); ...which calls... virtual nsIFrame* GetFirstChildFromListInner(nsIFrame* aFrame, nsIFrame::ChildListID aListID); (And the same for the "Last" variants) That makes it a lot easier to keep things straight, in my head at least.
Comment on attachment 711090 [details] [diff] [review] fix [side note: I love how "nsIFrame* parent" is only actually being treated as a parent a small percent af >@@ -281,19 +287,33 @@ nsFrameIterator::Next() >- } >- else { >+ } else { > result = GetParentFrameNotPopup(parent); >+ if (parent->GetType() == nsGkAtoms::tableCaptionFrame) { >+ // Go to inner table frame after end of caption frames. This probably wants MOZ_UNLIKELY, right? (same in Prev()) BUT: I actually think things would be clearer / cleaner if this we performed this logic in GetNextSiblingInner(), inside its existing special case for caption frames. Then Next() won't need any tweaks at all, IIUC. So -- your existing GetNextSiblingInner() does this: If I'm a caption, return my parent's next caption-child. Otherwise, return my parent's next principal child. I'm suggesting changing it to instead to this: If I'm a caption, return my parent's next caption-child, or if there's none, then return its first principal child. Otherwise, return my parent's next principal child. I'd imagine we could do this just by changing this: > nsIFrame* > nsVisualIterator::GetNextSiblingInner(nsIFrame* aFrame) { [...] >+ nsIFrame::ChildListID listID = nsIFrame::kPrincipalList; >+ if (MOZ_UNLIKELY(aFrame->GetType() == nsGkAtoms::tableCaptionFrame)) { >+ listID = nsIFrame::kCaptionList; >+ } >+ return parent->GetChildList(listID).GetNextVisualFor(aFrame); > } ...to look like this: if (MOZ_UNLIKELY(aFrame->GetType() == nsGkAtoms::tableCaptionFrame)) { nsIFrame* nextCaptionSibling = parent->GetChildList(nsIFrame::kCaptionList).GetNextVisualFor(aFrame); return nextCaptionSibling ? nextCaptionSibling : GetFirstChild(parent, nsIFrame::kPrincipalChildList); } } return parent->PrincipalChildList().GetNextVisualFor(aFrame); These will localize all of the caption-specific hacks for Next() into one spot, which is great for sanity / maintainability. Does that make sense? (This all applies to the chunk in Prev(), too. So, I don't think we need to make any direct modifications at all to Next() or Prev() at all.)
(In reply to Daniel Holbert [:dholbert] from comment #14) > Comment on attachment 711090 [details] [diff] [review] > fix > > [side note: I love how "nsIFrame* parent" is only actually being treated as > a parent a small percent af s/af/of the time]/ (this is part of the reason that I'd like to avoid reviewing code in Next() and Prev() -- I can't keep track of what "parent" and "result" mean from one line to the next. :))
I *think* this is r=me with the changes I've suggested above, but I'll hold off on marking it as such because I'm curious to see whether you think my suggestions are sane, & to see what the resulting code ends up looking like. :)
One question that dbaron brought up when I mentioned this bug to him: why is the caption-list special here? Shouldn't we need this for other child-lists, too? (e.g. float list) The answer to that is: entries in other child lists leave behind placeholders, and we follow those in this case. (because nsFocusManager::GetNextTabbableContent() passes 'true' for the aFollowOOFs parameter of NS_NewFrameTraversal) SO: given that, dbaron thinks we still might run into a version of this problem when a float gets split across columns in a multicol element (though we haven't tested to be sure). It seems possible that content in the float's continuation-frames wouldn't end up being focusable, because those continuation frames don't drop additional placeholders. That's sort of a version of this bug's original problem -- "things in special child lists aren't focusable, unless they drop placeholders on their parent".
Maybe a more general solution is to traverse all the child lists, but skip children with NS_FRAME_OUT_OF_FLOW set, since we'll find them through placeholders? It seems like a better solution would be to use the content tree rather than the frame tree, though.
(In reply to David Baron [:dbaron] from comment #18) > It seems like a better solution would be to use the content tree rather than > the frame tree, though. Incidentally, that's basically what we need for flexbox in bug 812687, too.
(I tried to test the float-in-multicol situation described at the end of comment 17, but it looks like we don't split floats inside of a multicol -- we just let them overflow. (Though we do split floats across pages in printed documents -- not sure why it works there but not in multicol.)
Attached patch fix, v2 (obsolete) — — Splinter Review
> I actually think things would be clearer / cleaner if this we performed this > logic in GetNextSiblingInner() Good suggestion to avoid messing with Prev()/Next(). However, there are two GetNextSiblingInner and I didn't want to repeat the caption logic twice so I chose GetNextSibling instead. The purpose of GetNextSibling seems to be to wrap GetNextSiblingInner anyway so I think it's appropriate. (same for GetPrevSibling)
Attachment #711090 - Attachment is obsolete: true
Attachment #711090 - Flags: review?(dholbert)
Attachment #715209 - Flags: review?(dholbert)
Attached patch mochitest, v3 (obsolete) — — Splinter Review
(In reply to Daniel Holbert [:dholbert] from comment #11) > Perhaps worth using todo_is() at the end there, instead of a commented-out > is() -- that way, if we fix this bug, we'll notice the behavior change and > we'll know to remove the "Hmm" comment. (might be worth filing a bug on this > behavior, too...) It turns out it's actually the correct behaviour and it's symmetric to the case when you tab forward into a document (from the URL/Seach bar). The first thing you hit is the documentElement (and a dotted outline appears around the viewport). The reason I was confused is that the test starts from document.body, which I guess is the default or perhaps a side-effect of SimpleTest.waitForFocus().
Attachment #713744 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #19) > (In reply to David Baron [:dbaron] from comment #18) > > It seems like a better solution would be to use the content tree rather than > > the frame tree, though. > > Incidentally, that's basically what we need for flexbox in bug 812687, too. Further improvements / refactoring of this code should be done in separate bugs IMHO.
(In reply to Mats Palmgren [:mats] from comment #23) > Further improvements / refactoring of this code should be done in separate > bugs IMHO. That sounds fine to me. So, regarding the updated patch -- this code is already somewhat hard to grok, and there are a few things about the current patch that shifts this in the harder-to-follow direction, which I'd like to avoid. My concerns are: (1) The patch creates an asymmetry between GetNextSiblingInner() and GetPrevSiblingInner() -- one requires a child list ID as a parameter (trusting its parent to have passed the right ID), while the other figures out the list ID for itself. That seems like an arbitrary difference, and it takes some manual code-reading to figure out why it's the case. (looks like it's because one parent alread needs to have looked up the list; the other doesn't) (2) The semantics of GetNextSibling()/GetPrevSibling() vs. their "Inner" helpers have changed. After this patch, the 'inner' versions are list-specific (they don't look outside the given frame's list), whereas GetNextSibling/GetPrevSibling() will now have the intelligence to step outside that list. This behavior-difference isn't really clear from their name. (It's not clear why you might call one vs. the other in a given circumstance, unless you read through the code.) One way to address these would be: (a) Make *both* GetNextSiblingInner and GetPrevSiblingInner take a list ID (for symmetry). This shouldn't really add any overhead -- we'd just be refactoring GetPrevSiblingInner()'s list-ID lookup out its parent. (b) Add some a brief header-comment to document that the Inner methods only look in the given list, and that they return nullptr if there's no next sibling in the given list. How does that sound?
Comment on attachment 715209 [details] [diff] [review] fix, v2 Canceling review and marking as feedback+, to stop the bugzilla auto-nag emails. :) (See review comments in prev. bug-comment)
Attachment #715209 - Flags: review?(dholbert) → feedback+
Can someone please explain to me the status of this 11 year old defect?
Recent developer report of running into this problem: https://twitter.com/iandevlin/status/860450918533390337 Mats, any thoughts here? I realize it may just have been lost in the backlog, or oddly complex and not a priority. Is there anyone else who may have time to work on it?
status-firefox55: --- → affected
Flags: needinfo?(mats)
Depends on: 1485335
Flags: needinfo?(mats)
Component: Keyboard: Navigation → User events and focus handling
Severity: minor → S4

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: MatsPalmgren_bugz → nobody

Still reproducible, but the URL is not available.

URL: http://jpfreeskool.org/calendar/
status-firefox109: --- → affected
Priority: -- → P3
Target Milestone: mozilla1.9alpha8 → ---
Duplicate of this bug: 1960188
Duplicate of this bug: 1961886
Duplicate of this bug: 1773693
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #715209 - Attachment is obsolete: true
Attachment #715211 - Attachment is obsolete: true

nsTableWrapperFrame does not have captions as normal children. Instead,
it has it as mCaptionFrames and they can be retrieved with
GetChildList(FrameChildListID::Caption).

Fortunately, currently caption-side is top or bottom, so, we can assume
that captions are children before its normal first child if caption-side: top
and after its last child if caption-side: bottom.

This patch treat the caption frames as "pseudo children" before/after normal
children and make the accessors of sibling frame treat they are consecutive.

Attachment #9480541 - Attachment is obsolete: true
Assignee: masayuki → emilio

Fwiw I agree with mats that long term we should use dom tree order for focus (we already do that for shadow dom iiuc)

Flags: needinfo?(emilio)

Masayuki, could you look into using the DOM traversal everywhere (not just on shadow dom) for focus navigation? That'd fix probably a variety of fun bugs in this area too, and should match other browsers. Separate bug is fine, I'll try to finish comment 39.

Flags: needinfo?(masayuki)

Hmm, I'm afraid the big change only for this case because that could cause a lot of regression reports and I'd be involved them. For the long term solution, I agree with that, but I think I don't have much time to touch the risky change for now... (I already have some ongoing big changes, so, I'd be involved regressions of them soon.)

Flags: needinfo?(masayuki)
Blocks: 144517
Blocks: 1963186
Attachment #9480645 - Attachment description: WIP: Bug 242829 - Remove table caption list. → Bug 242829 - Remove table caption list. r=#layout!
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fc420995880b Remove table caption list. r=dshin

https://hg.mozilla.org/mozilla-central/rev/fc420995880b

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
status-firefox140: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 140 Branch
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/52298 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Flags: needinfo?(emilio)
Blocks: 241042, 1485335
No longer blocks: 245142
No longer depends on: 1485335

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)
QA Whiteboard: [qa-triage-done-c141/b140]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: