Bug 22119 – [Transforms] clicks in scrollbars of transformed element don't work (e.g. listbox or overflow)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

If a multiline <select> is transformed, clicks in its scrollbar don't do anything.

[Simon Fraser (smfr)]

<rdar://problem/6672445>

[Simon Fraser (smfr)]

Created attachment 28619 [details]
Testcase

[Simon Fraser (smfr)]

I have a patch for this.

[Simon Fraser (smfr)]

Created attachment 28641 [details]
Patch, testcases, changelog

[Dave Hyatt]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Created attachment 28676 [details]
New patch

[Dave Hyatt]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 31631 [details]
WIP patch

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

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.

[Dave Hyatt]

Fixed in r45478.