In order for position:fixed and overflow:scroll objects to be accessibility and section 508 compliant, they must be in the tab order and be able show focus. This is what IE already does.

Test case position:fixed http://bugzilla.mozilla.org/attachment.cgi?id=21251&action=view Test case for overflow:auto http://bugzilla.mozilla.org/attachment.cgi?id=51338&action=view Complex test case: http://bugzilla.mozilla.org/attachment.cgi?id=153476&action=view

Turns out IE has these problems too -- thought I had seen otherwise. We still need to fix this for section 508 compliance.

duplicate of 140644?

No, that bug is related to caret browsing (moving selection). This one is tabbing (moving focus).

1) nsFrame.cpp: Scrollable HTML frames should be focusable+tabbable 2) nsEventStateManager.cpp: Allow multiple focusable elements in the same ancestor chain 3) nsPresShell.cpp: GetNearestScrollingView uses focus not selection 4) SetFocus() asks frame if focusable, which knows more than content does

Comment on attachment 155935 [details] [diff] [review] Patch, tested-works I'll use bryner or roc for sr=

The patch works fine in most cases (and is close to what I had ;-), but there are a couple of cases where the tabbing isn't doing what I'm expecting. 1. When tabbing forward over the 2nd set of DIVs, the focus moves from the DIV to the 2nd link in that DIV (expecting first link would be focused). 2. When tabbing forward over the last set of DIVs, the first link seems to get focused at first but then focus is moved to the follwing DIV, (expecting the first link would be focused) (maybe this is the same as 1, I can't tell) 3. When tabbing backward in the 2nd and 3rd set of DIVs, focus gets stuck on the DIV (expecting focus to be moved to the last link in the preceeding DIV)

Comment on attachment 155935 [details] [diff] [review] Patch, tested-works >Index: layout/html/base/src/nsFrame.cpp >+#include "nsIScrollableView.h" This isn't needed for the code you added. >Index: layout/html/base/src/nsPresShell.cpp >- esm->GetDocSelectionLocation(getter_AddRefs(selectionContent), This means we can remove it from nsIEventStateManager.h and make it non-virtual in nsEventStateManager.h again, if we want to... (it was added in bug 251986 to get the caret for kbd scrolling.) >Index: content/base/src/nsGenericElement.cpp >+ nsIPresShell *presShell = aPresContext->GetPresShell(); >+ if (presShell) { Do we really need to null-check the result from GetPresShell() ? (GetDocument()/GetViewManager()/StyleSet() in nsPresContext.h doesn't) The rest of the code looks fine and I agree with your listed changes in comment 5, but there seems to be a couple of glitches still with Testcase #1, so I review- this one hoping you will fix those too :-)

I can't figure out when a frame has scrollbars or not.

Frame dump for the file: Type Manifest File: C:\moz\mozilla\dist\bin\components\xpti.dat ++WEBSHELL == 1 ++DOMWINDOW == 1 Note: verifyreflow is disabled Note: styleverifytree is disabled Note: frameverifytree is disabled webshell=01241CC8 Viewport(-1)@02B1E36C [view=02B13D88] {0,0,9180,4350} [state=00042004] [content=00000000] [sc=02B1E2A8]< HTMLScroll(html)(-1)@02B1E598 {0,0,9180,4350} [state=81c40004] [content=02B17860] [sc=02B1E514]< ScrollPortFrame(html)(-1)@02B1E6D0 [view=02B26298] next=02B1E9B8 {0,0,9180,4350} [state=80c42004] [content=02B17860] [sc=02B62B4C]< Canvas(html)(-1)@02B1E46C [view=02B6E9A0] {0,0,9180,4350} [state=00042004] [content=02B17860] [sc=02B6D8B4]< Area(html)(-1)@02B6DAE0 {0,0,9180,3660} [state=00c40014] sc=02B6D9AC(i=1,b=1)< line 02B6DD3C: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x800] {0,0,0,0} < Text(1)@02B6DBDC[0,1,T] next=02B6DCE8 {0,0,0,0} [state=41100024] sc=02B6DA5C< "\n" > > line 02B6DD6C: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,nobr[0x804] bm=240 {120,120,8940,3300} ca={120,120,8940,3300} < Block(body)(2)@02B6DCE8 {120,120,8940,3300} [state=00040004] sc=02B6DC1C(i=3,b=2)< line 02B8498C: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x800] {0,0,0,0} < Text(0)@02B6E084[0,2,T] next=02B6E214 {0,0,0,0} [state=51100024] sc=02B6E058< "\n\n" > > line 02B849BC: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,nobr[0x804] bm=240 {0,0,8940,1530} ca={0,0,8940,1530} < HTMLScroll(div)(1)@02B6E214 next=02B84498 {0,0,8940,1530} [state=80c40004] [content=02B836D8] [sc=02B6E11C]< ScrollPortFrame(div)(1)@02B6E2D8 [view=02B84208] {15,15,8910,1500} [state=80c42004] [content=02B836D8] [sc=02B6E388]< Area(div)(1)@02B6E1C0 [view=02B843A8] {0,0,8910,1500} [state=00d02004] sc=02B6E438(i=0,b=1)< line 02B84468: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,nobr[0x804] mew=1200 {0,0,8910,300} < Block(div)(0)@02B6E58C {0,0,8910,300} [state=00000004] sc=02B6E540(i=1,b=0)< line 02B84438: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x1000] mew=1200 {0,0,1200,300} < Inline(a)(0)@02B6E6F0 next=02B6E794 {0,7,600,285} [state=00000004] [content=02B83800] [sc=02B6E664]< Text(0)@02B6E754[0,6,T] {0,0,600,285} [state=40200024] sc=02B6E728< "hidden" > > Inline(a)(1)@02B6E794 {600,7,600,285} [state=00000004] [content=02B839B8] [sc=02B6E664]< Text(0)@02B6E7CC[0,6,T] {0,0,600,285} [state=44200024] sc=02B6E728< "hidden" > > > > > > > > > line 02B849EC: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x800] {0,1530,0,0} < Text(2)@02B84498[0,1,T] next=02B845A8 {0,1770,0,0} [state=41100024] sc=02B6E058< "\n" > > line 02B84A1C: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,nobr[0x804] bm=240 {0,1770,8940,1530} ca={0,1770,8940,1530} < Block(div)(3)@02B845A8 [view=02B85468] next=02B8494C {0,1770,8940,1530} [state=00002004] sc=02B84504(i=0,b=1)< line 02B8491C: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,nobr[0x804] {15,15,8910,300} < Block(div)(0)@02B84730 {15,15,8910,300} [state=00000004] sc=02B846E4(i=1,b=0)< line 02B848EC: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x1000] {0,0,4620,300} < Inline(a)(0)@02B847D0 next=02B84874 {0,7,2310,285} [state=00000004] [content=02B83D30] [sc=02B84784]< Text(0)@02B84834[0,24,T] {0,0,2310,285} [state=40200024] sc=02B84808< "-moz-hidden-unscrollable" > > Inline(a)(1)@02B84874 {2310,7,2310,285} [state=00000004] [content=02B83EF8] [sc=02B84784]< Text(0)@02B848AC[0,24,T] {0,0,2310,285} [state=44200024] sc=02B84808< "-moz-hidden-unscrollable" > > > > > > > line 02B84A4C: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,nobr[0x800] {0,3300,0,0} < Text(4)@02B8494C[0,2,T] {0,3540,0,0} [state=51100024] sc=02B6E058< "\n\n" > > > > > > > ScrollbarFrame(scrollbar)(-1)@02B1E9B8 [view=02B24050] next=02B621B4 {0,4350,9180,0} [state=80cc2024] [content=02B2D8B8] [sc=02B1E830]< Box(scrollbarbutton)(-1)@02B1EBC8 next=02B1EDE8 {0,15,255,255} [state=80c00024] [content=02B53D90] [sc=02B1E90C]<> SliderFrame(slider)(-1)@02B1EDE8 [view=02B61B08] next=02B6203C {255,15,8670,255} [state=80c02024] [content=02B52940] [sc=02B1ED44]< Box(thumb)(0)@02B61C68 {0,0,8670,255} [state=80400024] [content=02B53990] [sc=02B61BC4]< Box(gripper)(-1)@02B61E34 {4275,67,120,120} [state=80c00024] [content=02B62CF0] [sc=02B61D90]<> > > Box(scrollbarbutton)(-1)@02B6203C {8925,15,255,255} [state=80c00024] [content=02B43D50] [sc=02B61F98]<> > ScrollbarFrame(scrollbar)(-1)@02B621B4 [view=02B66DD0] next=02B62A50 {9180,0,0,4350} [state=808c2024] [content=02B41350] [sc=02B62108]< Box(scrollbarbutton)(-1)@02B62314 next=02B624B0 {15,0,255,255} [state=80c00024] [content=02B661F8] [sc=02B62270]<> SliderFrame(slider)(-1)@02B624B0 [view=02B26CB0] next=02B62938 {15,255,255,3840} [state=80802024] [content=02B66530] [sc=02B62464]< Box(thumb)(0)@02B62604 {0,0,255,3840} [state=80000024] [content=02B66688] [sc=02B62560]< Box(gripper)(-1)@02B62730 {67,1860,120,120} [state=80c00024] [content=02B6A1E8] [sc=02B6268C]<> > > Box(scrollbarbutton)(-1)@02B62938 {15,4095,255,255} [state=80c00024] [content=02B66980] [sc=02B62894]<> > Box(scrollcorner)(-1)@02B62A50 {9180,4350,0,0} [state=80c00024] [content=02B41520] [sc=02B62A04]<> > > nsPluginHostImpl::Observe "xpcom-shutdown" --WEBSHELL == 0 nsPluginHostImpl dtor

GetActualScrollbarSizes has a bug. It's including the borders as part of the scrollbar size. I can fix that. Now that I see what you're trying to do, I don't think you want GetActualScrollbarSizes at all. I don't think tabbability should depend on whether a frame is currently overflowing. I think you want to call nsIScrollableFrame::GetScrollbarStyles() and allow scrolling when the style is AUTO or SCROLL in either direction.

Mat, I fixed the tab order issue by changing nsEventStateManager::GetDocSelectionLocation()

+ mContent->IsContentOfType(nsIContent::eHTML) && Do you really want to restrict to HTML? I think the generic accessible should be created by a method in nsFrame, either nsFrame::GetAccessible, or if you detect accessibility based on the nsresult code, then put the generic accessible creation in an nsFrame helper function and just call it from nsHTMLScrollFrame/nsXULScrollFrame::GetAccessible.

(In reply to comment #13) > + mContent->IsContentOfType(nsIContent::eHTML) && > > Do you really want to restrict to HTML? For the moment, yes. The reason is that XUL provides much better semantics for what things are in the actual element names + XBL. The underlying tag in HTML is usually generic, such as <div>. We need to mess around in this way for HTML but not XUL. > I think the generic accessible should be created by a method in nsFrame, either > nsFrame::GetAccessible, or if you detect accessibility based on the nsresult > code, then put the generic accessible creation in an nsFrame helper function and > just call it from nsHTMLScrollFrame/nsXULScrollFrame::GetAccessible. I'm not sure understand this part. It seems like putting it in nsHTMLScrollFrame works perfectly. We don't detect whether accessibility is turned on by any nsresult. If we do_GetService for accessibilityservice that initializes the accessibility module. We detect whether accessibility is happening because someone calls nsIFrame::GetAccessible(). I don't think we're going to have generic accessibles for XUL, since we can more or less ensure that the language supports reasonable semantics instead of vanilla <div> tags everywhere.

(In reply to comment #14) > > Do you really want to restrict to HTML? > For the moment, yes. The reason is that XUL provides much better semantics for > what things are in the actual element names + XBL. The underlying tag in HTML > is usually generic, such as <div>. We need to mess around in this way for HTML > but not XUL. There is non-XUL, non-HTML content. But OK. My thought was that if we need to add generic accessible support to other frames than scrollframes, it'll be easier if the generic accessible creation code was factored out somewhere it can be shared. If for some reason you know that's not going to happen, then OK.

I understand. * We're not working to make SVG, MathML or other non-HTML non-XUL content accessible yet. * The other place we need to create generic accessibles is in the mozilla/accessible module itself. However, we're doing that based on tag names and attributes, not layout info.

We still don't handle tabbing to IFRAME and FRAME: http://bugzilla.mozilla.org/attachment.cgi?id=153477&action=view http://bugzilla.mozilla.org/attachment.cgi?id=153478&action=view I don't know if you want to address that here or not - I'm just noting it so we don't forget it...

I see that overflow:hidden is not focusable as it was in the first patch. Note that it *is* scrollable however (e.g. after focusing a link in it). In a way this makes sense though and I actually prefer this behaviour. Others may have a different opinion so I thought I would point it out.

Comment on attachment 156285 [details] [diff] [review] 1) Fixes issues noted by Mat, 2) Makes sure ScrollFrameIntoView() is called, 3) Put scrollable items in accessibility hierarchy, 4) Fix warning in nsAccessibilityService::CreateHTMLComboboxAccessible >Index: content/base/src/nsGenericElement.cpp >+ presShell->ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_ANYWHERE, >+ NS_PRESSHELL_SCROLL_ANYWHERE); The above will cause problems with mouse-selection. Try for example: 1. load http://bugzilla.mozilla.org/attachment.cgi?id=153476&action=view 2. scroll the green box (middle) to the end 3. try to mouse-drag-select some of the "_2_" text in the inner DIV Result: the inner DIV scrolls, interfering badly with my selection. Also, when clicking on the inner DIV scollbar to scroll it, the outer DIV first scrolls it then the inner DIV does the scroll I wanted... One quick solution could be to not scroll at all if any part of the focused frame is visible? (or the event was mouse-triggered?). If you remove the above lines then I'll review+ it. (in case you want to handle the scroll-into-view problem later).

> Result: the inner DIV scrolls, interfering badly with my selection. What I meant was that the outer DIV scrolls so that the inner DIV moves under the mouse which interfers badly with my ongoing mouse-selection.

Filed bug 255881 on frame/iframe tabbing regression. That used to work -- nice catch! Also great work on the review in general. Thanks.

Comment on attachment 156348 [details] [diff] [review] Fix mouse drag scrolling. Scroll to iframe only if not already partially visible via ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE, ...) r=mats.palmgren@bredband.net but there is one minor edge case still...

Also try: 3. scroll the SELECT to the end 4. click on the green background -> SELECT scrolls to top.

I don't understand. Mats gave r= even though there's still a bug?

(In reply to comment #25) > I don't understand. Mats gave r= even though there's still a bug? Maybe we should file a separate bug on that part. After a couple hours of debugging I realized that it's caused by the reflow which happens for the outline. I'm not sure how to fix it. Any ideas? I'll post a stack trace for the extra scrolling occuring inside of the <select>

So, the last flaws in testcase #5 are because of nsChangeHint nsStyleOutline::CalcDifference(const nsStyleOutline& aOther) const ... if (outlineWasVisible != outlineIsVisible || mOutlineWidth != aOther.mOutlineWidth) { return NS_CombineHint(nsChangeHint_ReflowFrame, nsChangeHint_RepaintFrame); } The reflow is causing the scrolling.

Robert, that sounds like it would be fixed by bug 249102. Is that right?

(In reply to comment #25) > I don't understand. Mats gave r= even though there's still a bug? My reasoning was that the remaining problem would only occur rarely and when it does it's a minor problem. Since the code in the patch is good, I thought I would just clearly point out the problem and leave the decision with you guys.

(In reply to comment #29) > Robert, that sounds like it would be fixed by bug 249102. Is that right? I don't think so.

Comment on attachment 156348 [details] [diff] [review] Fix mouse drag scrolling. Scroll to iframe only if not already partially visible via ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE, ...) content/events/public/nsIEventStateManager.h Change the IID please. accessible/public/nsIAccessibilityService.idl Here too.

Checking in content/base/src/nsGenericElement.cpp; /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v <-- nsGenericElement.cpp new revision: 3.354; previous revision: 3.353 done Checking in content/events/public/nsIEventStateManager.h; /cvsroot/mozilla/content/events/public/nsIEventStateManager.h,v <-- nsIEventStateManager.h new revision: 1.57; previous revision: 1.56 done Checking in content/events/src/nsEventStateManager.cpp; /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v <-- nsEventStateManager.cpp new revision: 1.524; previous revision: 1.523 done Checking in content/events/src/nsEventStateManager.h; /cvsroot/mozilla/content/events/src/nsEventStateManager.h,v <-- nsEventStateManager.h new revision: 1.120; previous revision: 1.119 done Checking in layout/html/base/src/nsFrame.cpp; /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v <-- nsFrame.cpp new revision: 3.513; previous revision: 3.512 done Checking in layout/html/base/src/nsGfxScrollFrame.cpp; /cvsroot/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp,v <-- nsGfxScrollFrame.cpp new revision: 3.170; previous revision: 3.169 done Checking in layout/html/base/src/nsGfxScrollFrame.h; /cvsroot/mozilla/layout/html/base/src/nsGfxScrollFrame.h,v <-- nsGfxScrollFrame.h new revision: 3.48; previous revision: 3.47 done Checking in layout/html/base/src/nsPresShell.cpp; /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.765; previous revision: 3.764 done Checking in layout/html/forms/src/nsComboboxControlFrame.cpp; /cvsroot/mozilla/layout/html/forms/src/nsComboboxControlFrame.cpp,v <-- nsComboboxControlFrame.cpp new revision: 1.292; previous revision: 1.291 done Checking in accessible/public/nsIAccessibilityService.idl; /cvsroot/mozilla/accessible/public/nsIAccessibilityService.idl,v <-- nsIAccessibilityService.idl new revision: 1.44; previous revision: 1.43 done Checking in accessible/src/base/nsAccessibilityService.cpp; /cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v <-- nsAccessibilityService.cpp new revision: 1.117; previous revision: 1.116 done Checking in accessible/src/base/nsBaseWidgetAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsBaseWidgetAccessible.cpp,v <-- nsBaseWidgetAccessible.cpp new revision: 1.29; previous revision: 1.28 done

The scroll problem was spawned off as bug 257672. Verified fixed in 2004-09-02-05 trunk Linux.