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.

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.

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

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.

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

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

If this is reviewed and checked in shortly, we'll take it for aviary1.0

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

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

-> FIXED

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

Comment on attachment 153968 [details] [diff] [review] Patch rev. 2 not this late in the game and think we have trunk regressions for this.