<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Updated

•

20 years ago

Blocks: focusnav

Severity: major → normal

Keywords: sec508

Updated

•

20 years ago

Target Milestone: --- → Future

Emmanuel Bourg

Comment 2

•

20 years ago

Attached file Test case — Details

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

19 years ago

Severity: normal → minor

Target Milestone: Future → mozilla1.9beta

Comment 3

•

19 years ago

*** Bug 314851 has been marked as a duplicate of this bug. ***

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

19 years ago

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

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Updated

•

19 years ago

Blocks: 245142

Updated

•

18 years ago

Assignee: aaronleventhal → mats.palmgren

Phil Ringnalda (:philor)

Updated

•

15 years ago

QA Contact: keyboard.navigation

Comment 6

•

12 years ago

Attached patch fix (obsolete) — Details — 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)

Comment 7

•

12 years ago

Attached patch mochitest (obsolete) — Details — 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.

Comment 8

•

12 years ago

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.)

Comment 9

•

12 years ago

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

Comment 10

•

12 years ago

Attached patch mochitest, v2 (obsolete) — Details — 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 11

•

12 years ago

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.)

Comment 12

•

12 years ago

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 13

•

12 years ago

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 14

•

12 years ago

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.)

Comment 15

•

12 years ago

(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. :))

Comment 16

•

12 years ago

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. :)

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Comment 17

•

12 years ago

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".

Comment 18

•

12 years ago

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.

Comment 19

•

12 years ago

(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.

Comment 20

•

12 years ago

(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.)

Comment 21

•

12 years ago

Attached patch fix, v2 — Details — 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)

Comment 22

•

12 years ago

Attached patch mochitest, v3 — Details — 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().

Updated

•

12 years ago

Attachment #713744 - Attachment is obsolete: true

Comment 23

•

12 years ago

(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.

Comment 24

•

12 years ago

(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?

Liz Henry (:lizzard) (relman/hg->git project)

Comment 25

•

12 years ago

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+

CraftyInstigator

Comment 27

•

9 years ago

Can someone please explain to me the status of this 11 year old defect?

Comment hidden (me-too)

Comment 30

•

8 years ago

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)