Closed Bug 251986 Opened 19 years ago Closed 18 years ago

Keyboard scrolling does not work for elements such as div using overflow - auto or scroll

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: access, polish, testcase)

Attachments

(2 files, 2 obsolete files)

Spawned from bug 97283.

Robert O'Callahan wrote in bug 97283 comment 125:
>For keyboard scrolling, I think we should do a similar search but start the
>search at the frame where the caret is (effectively, the frame that was last
>clicked or focused).

Patch coming up that does that...

David Baron wrote in bug 97283 comment 120:
>Also, I think frames/iframes and 'overflow' should not be distinguishable
>to the user based on their scrolling / focus behavior.

Regarding scrolling: yes.  The patch does not address the focus issue,
which can be handled in a separate bug if we want that. I have checked what
IE6.0/XP and Opera 7.52/Linux does and neither supports focus on generic
overflow:scroll elements.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Implements what is said in comment 0. Note that the scrolling is not
"recursive" in the sense that mouse scrolling is - that is, when the end
position is reached the enclosing scrollable element is not scrolled.
Mouse position is not involved at all.
This is exactly what IE6.0/XP and Opera7.52/Linux does, as far as I can tell.
Attached patch Patch rev. 1 (diff -uw) (obsolete) — Splinter Review
Comment on attachment 153581 [details] [diff] [review]
Patch rev. 1

Note for reviewers:
The static helper function 'GetNearestScrollingView' is copy-pasted
from 'nsEventStateManager::GetNearestScrollingView' - maybe this
code should live in somewhere else? (nsIScrollableView?)
Attachment #153581 - Flags: superreview?(dbaron)
Attachment #153581 - Flags: review?(roc)
If someone could test the patch on Windows that would be great.
I have only tested it on Linux.
Since this was spawned from bug 97283: requesting the same flags.
Flags: blocking1.8a3?
Flags: blocking-aviary1.0?
Keywords: polish
Flags: blocking-aviary1.0RC1?
(In reply to comment #4)
> If someone could test the patch on Windows that would be great.
> I have only tested it on Linux.
I've tested this patch on Windows, I've build it with MingW. 
Afaict, the patch works great in all the complex testcases (precisely as
intended). Thank you very much!
(In reply to comment #3)
> (From update of attachment 153581 [details] [diff] [review])
> Note for reviewers:
> The static helper function 'GetNearestScrollingView' is copy-pasted
> from 'nsEventStateManager::GetNearestScrollingView' - maybe this
> code should live in somewhere else? (nsIScrollableView?)

You should move it to nsLayoutUtils and have them both reference it there.

Other than that, it looks just right. Fix the patch and repost it, and I'll
stamp the review...
Comment on attachment 153581 [details] [diff] [review]
Patch rev. 1

Looks good to me as well, but instead of just moving it, make it iterative
instead of recursive too. :-)
Attached patch Patch rev. 2Splinter Review
Added an iterative nsLayoutUtils::GetNearestScrollingView() that has an
ASSERTION if it's passed a null view, something the old code didn't have.
I have eliminated nsTypedSelection::GetClosestScrollableView() too (in
nsSelection.cpp).
I have checked all call sites and they should be OK regarding the null view
(the old ESM code would have crashed on that anyway).
Attachment #153581 - Attachment is obsolete: true
Attachment #153582 - Attachment is obsolete: true
Attachment #153581 - Flags: superreview?(dbaron)
Attachment #153581 - Flags: review?(roc)
Attachment #153968 - Flags: superreview?(dbaron)
Attachment #153968 - Flags: review?(roc)
If this is reviewed and checked in shortly, we'll take it for aviary1.0
Flags: blocking-aviary1.0RC1?
Flags: blocking-aviary1.0RC1+
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0-
Attachment #153968 - Flags: superreview?(dbaron) → superreview+
Mats, could you also take a look at bug 63663?
I can scroll the testcase in bug 63663 both by mouse-wheel and keyboard
so I guess the patch here and bug 97283 fixes that one too (I have some other
stuff in my tree also so I can't be completely sure, but almost ;).
Blocks: 63663
(In reply to comment #13)
> I can scroll the testcase in bug 63663 both by mouse-wheel and keyboard
> so I guess the patch here and bug 97283 fixes that one too (I have some other
> stuff in my tree also so I can't be completely sure, but almost ;).

Okay when you check it in I'll do more testing.
Perhaps bug 194904 is also something that could quite easily be fixed with these
patches :)
Blocks: 9086
-> FIXED
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: [FIX] Keyboard scrolling does not work for elements such as div using overflow - auto or scroll → Keyboard scrolling does not work for elements such as div using overflow - auto or scroll
Flags: blocking1.8a3?
Need testcase.
Here's the testcase:
http://bugzilla.mozilla.org/attachment.cgi?id=51338&action=view

Two problems with the fix:
1. You have to click in there to get focus to the scrollable div.
2. Once you click there you can't see focus via a dotted outline

Should we REOPEN or file a new bug? Perhaps "tabbing and focus appearance not
working on overflow: auto or position: fixed"
I think nsIFrame::IsFocusable(PRBool *aIsTabbable) needs to be changed to return
true and *aIsTabbable = PR_TRUE for cases with overflow: auto or position: fixed.
> Need testcase.

Use the three testacases in bug 97283 labeled "complex 1", they cover all cases
I could think of.

> 1. You have to click in there to get focus to the scrollable div.
> 2. Once you click there you can't see focus via a dotted outline

That is a separate problem. The question is, do we really want anything with a
scroll view take part in tabbing?  Open a new bug if you think so (maybe it's
even reported). I took a quick glance at Opera and they don't do it.
Comment on attachment 153968 [details] [diff] [review]
Patch rev. 2

Putting on approval radar
Attachment #153968 - Flags: approval-aviary?
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-
Comment on attachment 153968 [details] [diff] [review]
Patch rev. 2

not this late in the game and think we have trunk regressions for this.
Attachment #153968 - Flags: approval-aviary? → approval-aviary-
You need to log in before you can comment on or make changes to this bug.