254966 - overflow:scroll/auto not keyboard accessible/focusable

Status: VERIFIED FIXED

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

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

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

Comment 2

[Aaron Leventhal]

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

Comment 3

[Ginn Chen]

duplicate of 140644?

Comment 4

[Aaron Leventhal]

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

Comment 5

[Aaron Leventhal]

Attachment: Patch, tested-works

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 6

[Aaron Leventhal]

Comment on attachment 155935 [details] [diff] [review]
Patch, tested-works

I'll use bryner or roc for sr=

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #1

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 8

[Mats Palmgren (inactive)]

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

Comment 9

[Aaron Leventhal]

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

Comment 10

[Aaron Leventhal]

Attachment: overflow:hidden and -moz-hidden-unscrollable not returning 0 for GetActualScrollbarSizes()

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Aaron Leventhal]

Attachment: 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

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

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Aaron Leventhal]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 16

[Aaron Leventhal]

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.

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[Mats Palmgren (inactive)]

Attachment: Testcase #3

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 19

[Mats Palmgren (inactive)]

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

Comment 20

[Mats Palmgren (inactive)]

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

Comment 21

[Aaron Leventhal]

Attachment: Fix mouse drag scrolling. Scroll to iframe only if not already partially visible via ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE, ...)

Filed bug 255881 on frame/iframe tabbing regression. That used to work -- nice
catch!

Also great work on the review in general. Thanks.

Comment 22

[Mats Palmgren (inactive)]

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=[e-mail] but there is one minor
edge case still...

Comment 23

[Mats Palmgren (inactive)]

Attachment: Testcase #5

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 26

[Aaron Leventhal]

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

Comment 27

[Aaron Leventhal]

Attachment: Stack trace (reflow for outline causes scrolling)

Comment 28

[Aaron Leventhal]

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.

Comment 29

[Aaron Leventhal]

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

Comment 30

[Mats Palmgren (inactive)]

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

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 33

[Aaron Leventhal]

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

Comment 34

[Mats Palmgren (inactive)]

The scroll problem was spawned off as bug 257672.

Verified fixed in 2004-09-02-05 trunk Linux.

Comment 35

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381