Open Bug 242829 Opened 18 years ago Updated 3 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

mozilla1.9alpha8
Tracking Status
firefox55 --- affected

People

(Reporter: sophia, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: access)

Attachments

(3 files, 3 obsolete files)

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
Blocks: 245142
Assignee: aaronleventhal → mats.palmgren
Duplicate of this bug: 407568
QA Contact: keyboard.navigation
Duplicate of this bug: 708526
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, v2Splinter 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, v3Splinter 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+
Duplicate of this bug: 899304
Can someone please explain to me the status of this 11 year old defect?
Duplicate of this bug: 1356216
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?
Flags: needinfo?(mats)
Depends on: 1485335
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.