Bug 38189 – [chromium] clicking a scrollbar in an iframe shouldn't lose the selection

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] clicking a scrollbar in an iframe shouldn't lose the selection

[Tony Chang]

Created attachment 54404 [details]
Patch

[Tony Chang]

On Safari Mac, selection isn't lost because the event gets sent to the NSScroller.  So we add some logic to do the same in Chromium.

This is http://crbug.com/5559

[Evan Martin]

+    PlatformMouseEvent mouseEvent = mev.event();
maybe
const PlatformMouseEvent& mouseEvent = mev.event();
?


Your fix feels a bit hacky to me, but I think a comment might clear it up ("on other ports subviews never get mouse clicks on their scrollbars because scrollbars are native widgets etc").

[Tony Chang]

Created attachment 54489 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> +    PlatformMouseEvent mouseEvent = mev.event();
> maybe
> const PlatformMouseEvent& mouseEvent = mev.event();
> ?

I'm never sure if I need to do this or if the compiler will do it for me.  I guess it doesn't hurt to make it explicit.

[Tony Chang]

Adding Darin as a potential reviewer.

Let me describe the code in EventHandlerMac.mm.  When you click in a subframe, you end up in passSubframeEventToSubframe:
http://trac.webkit.org/browser/trunk/WebCore/page/mac/EventHandlerMac.mm#L354

For a mouse clicks, this calls passWidgetMouseDownEventToWidget:
http://trac.webkit.org/browser/trunk/WebCore/page/mac/EventHandlerMac.mm#L390

Which calls through to passMouseDownEventToWidget:
http://trac.webkit.org/browser/trunk/WebCore/page/mac/EventHandlerMac.mm#L206

A hit test is performed to see which NSView is hit:
http://trac.webkit.org/browser/trunk/WebCore/page/mac/EventHandlerMac.mm#L223

And the view is passed the event directly in line 255.  If we're over a scrollbar, |view| is an NSScroller, otherwise it's a WebHTMLView.

It seems like some of this logic is in EventHandler::handleMousePressEvent, which is what we used to always call:
http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L1166

However, the logic for passing the event to a scrollbar doesn't happen until after the page has a chance to handle the event.  The page handling the event is what causes us to lose the selection.

In writing this, I realized that this also means that previously, if you set mousedown to preventDefault(), you could not scroll the iframe.  With this change, you can still scroll the iframe.  This seems to match Safari mac and Firefox mac (in Firefox, I had to put the preventDefault in keypress to stop selection).

[Eric Seidel (no email)]

Comment on attachment 54489 [details]
Patch

This seems reasonable to me.  It seems that WebKit2 will need this code as well, so it's a shame it's in a Chromium file.

The test is certainly a huge win.

[Eric Seidel (no email)]

Adding Sam since WebKit2 will eventually need code like this as well, perhaps he has a suggestion for making it more generic.

[Eric Seidel (no email)]

Attachment 54489 [details] was posted by a committer and has review+, assigning to Tony Chang for commit.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> (From update of attachment 54489 [details])
> This seems reasonable to me.  It seems that WebKit2 will need this code as
> well, so it's a shame it's in a Chromium file.

Actually, this has more in common with other WebKit ports that do not use native widgets for scrollbars.

Patch LGTM2.

[WebKit Commit Bot]

Comment on attachment 54489 [details]
Patch

Rejecting patch 54489 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1
Last 500 characters of output:
st/events/mousedown-in-subframe-scrollbar-expected.txt
patching file LayoutTests/fast/events/mousedown-in-subframe-scrollbar.html
patching file LayoutTests/fast/events/resources/mousedown-in-subframe-scrollbar.html
patching file LayoutTests/platform/win/Skipped
Hunk #1 FAILED at 855.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/page/chromium/EventHandlerChromium.cpp

Full output: http://webkit-commit-queue.appspot.com/results/2187010

[Tony Chang]

Committed r58937: <http://trac.webkit.org/changeset/58937>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58937 might have broken Leopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/58936
http://trac.webkit.org/changeset/58937

[Tony Chang]

(In reply to comment #12)
> Committed r58937: <http://trac.webkit.org/changeset/58937>

Oh, so it turns out this was fixed by https://bugs.webkit.org/show_bug.cgi?id=16809 a few days before I landed this patch (which explains why the layout test passes on QT).

Should I revert this change (but keep the test)?  Reverting this change makes Chromium follow a code path more like QT.  Keeping this change makes us follow a code path more like Safari Mac.  I don't have a strong opinion here.

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
> (In reply to comment #12)
> > Committed r58937: <http://trac.webkit.org/changeset/58937>
> 
> Oh, so it turns out this was fixed by https://bugs.webkit.org/show_bug.cgi?id=16809 a few days before I landed this patch (which explains why the layout test passes on QT).
> 
> Should I revert this change (but keep the test)?  Reverting this change makes Chromium follow a code path more like QT.  Keeping this change makes us follow a code path more like Safari Mac.  I don't have a strong opinion here.

It looks like the Qt solution is cross-platform, so we should probably go with that one.  Does that make us at least consistent with Safari Win?

[Darin Fisher (:fishd, Google)]

Moreover, would it make sense for Safari Mac to be like the others?  File a bug on that if so?

[Antonio Gomes]

(In reply to comment #14)
> (In reply to comment #12)
> > Committed r58937: <http://trac.webkit.org/changeset/58937>
> 
> Oh, so it turns out this was fixed by https://bugs.webkit.org/show_bug.cgi?id=16809 a few days before I landed this patch (which explains why the layout test passes on QT).
> 
> Should I revert this change (but keep the test)?  Reverting this change makes Chromium follow a code path more like QT.  Keeping this change makes us follow a code path more like Safari Mac.  I don't have a strong opinion here.

the partial revert (tests are being kept) is happening in bug 38894