Bug 22119

<rdar://problem/6672445>

Created attachment 28619 [details] Testcase

I have a patch for this.

Created attachment 28641 [details] Patch, testcases, changelog

Comment on attachment 28641 [details] Patch, testcases, changelog Minus after IRC discussion. Want to see the scrollbarOwner thing removed and replaced with the right method(s) just being added to ScrollbarClient.

Note: need to fix windows-specific popup code too.

Created attachment 28676 [details] New patch

Comment on attachment 28676 [details] New patch My high level commentary here is that new methods have been added that essentially duplicate functionality of Widget and ScrollView. The reasoning being that the Widget/ScrollView methods aren't transform-aware. I think a better approach would be to make them transform-aware. Take for example: IntPoint Widget::convertToContainingWindow(const IntPoint& point) const { IntPoint windowPoint = point; const Widget* childWidget = this; for (const ScrollView* parentScrollView = parent(); parentScrollView; childWidget = parentScrollView, parentScrollView = parentScrollView->parent()) windowPoint = parentScrollView->convertChildToSelf(childWidget, windowPoint); return windowPoint; } IntPoint Widget::convertFromContainingWindow(const IntPoint& point) const { IntPoint widgetPoint = point; const Widget* childWidget = this; for (const ScrollView* parentScrollView = parent(); parentScrollView; childWidget = parentScrollView, parentScrollView = parentScrollView->parent()) widgetPoint = parentScrollView->convertSelfToChild(childWidget, widgetPoint); return widgetPoint; } There is nothing wrong with those methods if you imagine that convertChildToSelf and convertSelfToChild are transform-aware. In other words, try to find the "leaf" functions used by all of these Widget/ScrollView methods and make them virtual. Subclass them on FrameView to do the right thing. It is likely that you may need a "usesTransforms" boolean on FrameView, kind of like we have slow repaints on ScrollView. If that flag isn't set then you know you could fall through to the much faster ScrollView functions and not have to crawl up your render tree. That is an optimization that could be done in a separate patch though I think. Now there are still some good things here though. For example, it is good to just send local points to Scrollbars always. There's no reason to be sending them window coordinates when you already hit tested all the way to the scrollbar and so should know the local coordinate anyway. So eliminating window to/from conversions from scrollbar code is good.

I'm not sure how I can do this by subclassing methods in FrameView. We need to have rect/point conversions done via RenderObjects for Scrollbars and other widgets (e.g. plug-ins), not just for frames. It seems like the only way to do this in a generic way would be to resurrect WidgetClient? Otherwise there's no way to get from a Widget to its associated RenderObject.

Created attachment 31631 [details] WIP patch

Every Widget (scrollbar, plugin, frameview) has a non-platform implementation, so we can override virtual methods on Widget in those. The scrollbar one would call through ScrollbarClient.

Created attachment 31994 [details] Working patch Here's a mostly-complete patch that fixes Widget::convertTo/FromContainingWindow methods so that subclasses can modify their behavior to take transforms into account.

Created attachment 32111 [details] Almost done; needs testing on Windows

Created attachment 32161 [details] Almost done patch; needs ScrollbarClient cleanup With this patch nested overflow:scroll regions with transforms work on Windows too. For some reason, rotated scrollbars on Windows don't draw. I added convertTo/FromRenderer methods on FrameView as the funnel points for converting from some renderer up to its enclosing FrameView. convertToRenderer(RenderObject*,IntRect) has a FIXME, but as far as I know it's never used (we never map rects down into renderers). ScrollbarClient is troublesome, because it's too easy for a client to just turn around and call scrollbar->convertToContainingView() to get the default behavior, and hit infinite recursion.

Fixed in r45478.