Bug 22119

Summary: [Transforms] clicks in scrollbars of transformed element don't work (e.g. listbox or overflow)
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt
Priority: P3 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 22120    
Attachments:
Description Flags
Testcase
none
Patch, testcases, changelog
hyatt: review-
New patch
hyatt: review-
WIP patch
none
Working patch
none
Almost done; needs testing on Windows
none
Almost done patch; needs ScrollbarClient cleanup simon.fraser: review+

Description Simon Fraser (smfr) 2008-11-06 17:15:13 PST
If a multiline <select> is transformed, clicks in its scrollbar don't do anything.
Comment 1 Simon Fraser (smfr) 2009-03-11 17:08:39 PDT
<rdar://problem/6672445>
Comment 2 Simon Fraser (smfr) 2009-03-14 11:01:04 PDT
Created attachment 28619 [details]
Testcase
Comment 3 Simon Fraser (smfr) 2009-03-14 23:02:44 PDT
I have a patch for this.
Comment 4 Simon Fraser (smfr) 2009-03-15 13:16:31 PDT
Created attachment 28641 [details]
Patch, testcases, changelog
Comment 5 Dave Hyatt 2009-03-16 15:14:17 PDT
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.
Comment 6 Simon Fraser (smfr) 2009-03-16 15:15:15 PDT
Note: need to fix windows-specific popup code too.
Comment 7 Simon Fraser (smfr) 2009-03-16 20:31:25 PDT
Created attachment 28676 [details]
New patch
Comment 8 Dave Hyatt 2009-03-18 11:16:32 PDT
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.
Comment 9 Simon Fraser (smfr) 2009-06-21 22:14:59 PDT
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.
Comment 10 Simon Fraser (smfr) 2009-06-21 22:15:30 PDT
Created attachment 31631 [details]
WIP patch
Comment 11 Simon Fraser (smfr) 2009-06-22 10:48:33 PDT
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.
Comment 12 Simon Fraser (smfr) 2009-06-28 18:18:15 PDT
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.
Comment 13 Simon Fraser (smfr) 2009-06-30 23:23:44 PDT
Created attachment 32111 [details]
Almost done; needs testing on Windows
Comment 14 Simon Fraser (smfr) 2009-07-01 17:02:56 PDT
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.
Comment 15 Dave Hyatt 2009-07-02 10:33:27 PDT
Fixed in r45478.