Bug 16809

Summary: Clicking a scrollbar blurs the currently focused element
Product: WebKit Reporter: Michael Brasser <michael.brasser>
Component: WebCore Misc.Assignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, aroben, darin, hausmann, hyatt, mitz, tonikitoo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 29484    
Attachments:
Description Flags
Same testcase as in #4, but attached to fix the linebreak
none
Blur behaviour in various browsers
none
patch v0 - WIP
none
patch v1
darin: review-, tonikitoo: commit-queue-
patch v1.1
darin: review-, tonikitoo: commit-queue-
patch v2
none
(committed in r58476) patch v3 none

Description Michael Brasser 2008-01-09 17:01:56 PST
When clicking a scrollbar, the focus on the page is lost. This behavior is different to the behavior in Safari, IE, and Firefox.

1. Go to a page that has an input field and scrolls (such as search results from google)
2. Make sure the input field has focus
3. Press on the scrollbar to begin scrolling.
4. The input field will lose focus.

This is especially apparent in Qtopia, since losing focus causes the input method (virtual keyboard) to hide and the view to resize.
Comment 1 Michael Brasser 2008-01-10 15:42:24 PST
The loss of focus comes from EventHandler.cpp:1275 (in EventHandler::dispatchMouseEvent):

if (!m_frame->page()->focusController()->setFocusedNode(0, m_frame))
Comment 2 Tor Arne Vestbø 2008-04-22 01:55:17 PDT
Small test case:

data:text/html,<frameset cols=50,50><frame src="data:text/html,<body style='height: 1000'>foo</body>"><frame src="data:text/html,<body style='height: 1000'><form><input type=text value=bar></form></body>"></frameset>
Comment 3 Tor Arne Vestbø 2008-04-22 06:31:05 PDT
Test case for seeing which events gets dispatched when clicking on the scrollbar:

data:text/html,<body style='height: 500; width: 500;'><script language=javascript>document.onclick = function() { alert('document'); }; document.body.onclick = function() { alert('body'); }; window.onclick = function() { alert('window'); }</script><div style='width: 100%; height: 100%; background: gray;'>body</div></body>

Neither Safari/Mac, Opera/Linux, or Konqueror dispatches events when clicking the scrollbar. Will check on IE.
Comment 4 Tor Arne Vestbø 2008-04-22 07:04:36 PDT
Updated testcase, more complete:

data:text/html,<body style='height: 500; width: 500;'><script language=javascript>function printevent(e) { if (!e) var e = window.event; if (e.target) targ = e.target; else if (e.srcElement) targ = e.srcElement; if (targ.nodeType == 3) targ = targ.parentNode; document.getElementById('log').appendChild(document.createTextNode(Date().toLocaleString() + ': Event ' + e.type + ' on ' + targ.nodeName + '\n'));}; document.onmousedown = document.onmouseup = document.onclick = document.body.onmousedown = document.body.onmouseup = document.body.onclick = window.onmousedown = window.onmouseup = window.onclick = printevent; </script><pre><div id=log style='width: 100%; height: 100%; background: gray;'><h3>body</h3></div></pre></body>

It seems we _do_ get events on scrollbars in Firefox. The mousedown and mouseup event, but not clicked. In QtWebKit we only get mousedown. Safari/Mac dispatches nothing because the event is picked up by the native control.
Comment 5 Tor Arne Vestbø 2008-04-22 07:12:27 PDT
IE 7 propagates mousedown, so it appears that most browsers (except Safari/Mac) propagate _at least_ mousedown, which means we can't block event dispatching just to solve this bug.
Comment 6 Tor Arne Vestbø 2008-04-22 07:30:54 PDT
Created attachment 20748 [details]
Same testcase as in #4, but attached to fix the linebreak
Comment 7 Tor Arne Vestbø 2008-04-22 08:01:01 PDT
Concidered moving the scrolling logic to HTMLHtmlElement's defaultEventHandler, like for HTMLFrameSetElement, but we would have to protect against clients calling preventDefault() and killing of something as basic as scrolling. Tests show that Firefox does not allow this (not even for <div style="overflow:scroll">). 

Another thing about <div style="overflow:scroll"> is that when clicking the scrollbars dispatchMouseEvent is called with the DIV, not HTML like for frame scrollbars.
Comment 8 Tor Arne Vestbø 2008-04-23 04:23:27 PDT
Yet another testcase :)

http://chaos.troll.no/~tavestbo/webkit/mouse-events.html
Comment 9 mitz 2008-04-23 08:25:44 PDT
<rdar://problem/5586891>
Comment 10 Tor Arne Vestbø 2008-04-23 08:38:46 PDT
I messed up, didn't mean to mark as RESOLVED INVALID
Comment 11 Tor Arne Vestbø 2008-04-24 05:07:58 PDT
Created attachment 20791 [details]
Blur behaviour in various browsers 

Tested blur behaviour in Firefox, IE7/Win, Opera 9, Safari/Mac, Safari/Win and WebKit/Qt.

The behaviour of Safari/Win and WebKit/Qt to blur focus on clicking frame scrollbars, plus disabling scrolling if preventDefault() is called, seems to be inconcistent with other browsers. The reason these two issues are not observable on Safari/Mac is because it uses native scrollbars for the frame so the mousedown event never gets down to the core WebKit handling. 

Would like some feedback on whether or not we should change this behaviour to match Safari/Mac++, and if so, how we best would do that?
Comment 12 Antonio Gomes 2010-04-23 12:57:53 PDT
Created attachment 54182 [details]
patch v0 - WIP

++functional
++has_layout test
--needs some love before review
Comment 13 Antonio Gomes 2010-04-26 19:58:18 PDT
Created attachment 54373 [details]
patch v1

Summary: patch avoids clicking on frame scrollbars to cause focused elements to blur. It matches Firefix and Opera behavior.

Details:

0) Patch only affects ports that do *not* make use of platform (native) widgets for rendering scrollbars, including Mac, Gtk and Efl.
i.e. patches does affect ports including Qt and Chromium, where scrollbars are handled and rendered entirely by WebCore.

1) The added layout test (scrollbar-click-does-not-blur-content.html) can not be tested on Mac DRT because EventSendingController can only synthesize event if it targets WebView. As on Mac scrollbars are native widgets, when they get clicked, they process the event themselves, and do not forward to Web content.

2) The added layout test (scrollbar-click-does-not-blur-content.html) can not be tested on Gtk DRT because in Gtk port document.body.{clientWidth,Height} wrongly include size of scrollbars, which according to docs is wrong [1].

3) Patch does not affect in-frame scrollbars, including listbox', scrollable <div>'s, or <textarea>'s scrollbars.

[1] https://developer.mozilla.org/en/DOM/Element.clientWidth
Comment 14 Antonio Gomes 2010-04-26 20:06:58 PDT
Created attachment 54375 [details]
patch v1.1

now the right newer patch.

please read comment #13 for a very detailed summary.
Comment 15 Darin Adler 2010-04-27 13:06:33 PDT
Comment on attachment 54373 [details]
patch v1

> +    // If clicking on a frame scrollbar, do not mess up with content focus.
> +    if (FrameView* view = m_frame->view())
> +        if (view->scrollbarAtPoint(mouseEvent.pos()))
> +            return false;

Need braces around the body of this if statement.

Is an extra hit test necessary here? Is there some other way to find we clicked on the scrollbar other than repeating the hit testing process? I'd like to see that information passed back from the hit testing we are already doing, rather than adding an extra check for this.
Comment 16 Antonio Gomes 2010-04-27 13:37:17 PDT
Thank for looking at it, Darin.

(In reply to comment #15)
> (From update of attachment 54373 [details])
> > +    // If clicking on a frame scrollbar, do not mess up with content focus.
> > +    if (FrameView* view = m_frame->view())
> > +        if (view->scrollbarAtPoint(mouseEvent.pos()))
> > +            return false;
> 
> Need braces around the body of this if statement.

I missed that, sorry.

> Is an extra hit test necessary here?

As the code is today, it *is* needed. I commented about it in bug https://bugs.webkit.org/show_bug.cgi?id=19033#c8 today.

"I think the change looks ok (I am not a reviewer) since
MouseEventWithHitTestResults does not set 'scrollbar' when hit-testing a frame
scrollbar, but only in-frame scrollbars (like scrollable <div>, <textarea>,
etc).
"
> Is there some other way to find we clicked
> on the scrollbar other than repeating the hit testing process? I'd like to see
> that information passed back from the hit testing we are already doing, rather
> than adding an extra check for this.

In fact, it is not a *real* hit test.

ScrollView::scrollbarAtPoint(const IntRect& windowPoint){
(..)
 IntPoint viewPoint = convertFromContainingWindow(windowPoint);
    if (m_horizontalScrollbar &&  
        m_horizontalScrollbar->frameRect().contains(viewPoint))
        return m_horizontalScrollbar.get();
(...)
}

So, I assumed 'scrollbar' of HitTest only gets set if hit testing in-frame scrollbars, and it is by design (?)

Hum, I can double check that, but the silly point here (that might be the reason for the current behaviour is MouseEventWithHitTestResults does not set 'scrollbar' field for frame scrollbars clicks) is frame scrollbars can be plaftorm widget, and hit test can not be performed at al.

What do you think?
Comment 17 Darin Adler 2010-04-27 14:28:36 PDT
Comment on attachment 54375 [details]
patch v1.1

Braces are still wrong in this patch. Not sure what happened there.

A problem with this patch is that it doesn't seem to be making the check at just the right moment. We've already called updateMouseEventTargetNode, which presumably is setting the node under the mouse to something wrong, because the scrollbar is in the way. So I would think we'd want to do the work before calling updateMouseEventTargetNode.

But if I'm wrong and we correctly set the node under the mouse in updateMouseEventTargetNode, then I'm noot sure why it's right to unconditionally return false. After all, we dispatched a mouse event to the node already. Since in that case we are trying to affect only the focus behavior then I think the code should go inside the mousedownEvent if statement.

To be sure this is right, I'd need to confirm that we want to avoid setting the focused node, but we do want to dispatch the mouse event.
Comment 18 Antonio Gomes 2010-04-27 15:04:22 PDT
(In reply to comment #17)
> (From update of attachment 54375 [details])
> Braces are still wrong in this patch. Not sure what happened there.

Nothing went wrong :) You reviewed an obsolete version of the patch first, and then the latest one (both contain the same style issue you pointed out).

> A problem with this patch is that it doesn't seem to be making the check at
> just the right moment. We've already called updateMouseEventTargetNode, which
> presumably is setting the node under the mouse to something wrong, because the
> scrollbar is in the way. So I would think we'd want to do the work before
> calling updateMouseEventTargetNode.

Hum, I see your concern, however I think current behaviour is intentional, see the "backtrace" of the hitTest:

1) non pltform widget scrollbar gets clicked:
2) EventHandler::handleMousePressEvent calls Document::prepareMouseEvent (line 1194), retuning a MouseEventWithHitTest.
3) Document::prepareMouseEvent calls RenderLayer::hitTest, and there if scrollbars are clicked, it assumes the outer content layer got clicked. That is exactly where MouseEventWithHitTestResults::targetNode gets set to <HTML> (in this case). See the comment in the snippet below:

bool RenderLayer::hitTest(const HitTestRequest& request, HitTestResult& result)
{
(...)
  RenderLayer* insideLayer = hitTestLayer(this, 0, request, result, boundsRect, result.point(), false);
  if (!insideLayer) {
    // We didn't hit any layer. If we are the root
    // layer and the mouse is -- or just was -- down, 
    // return ourselves. We do this so mouse events
    // continue getting delivered after a drag has 
    // exited the WebView, and so hit testing over
    // a scrollbar hits the content document.
    if ((request.active() || request.mouseUp()) && renderer()->isRenderView()) {
      renderer()->updateHitTestResult(result, result.point());
      insideLayer = this;
    }
}
(...)


> But if I'm wrong and we correctly set the node under the mouse in
> updateMouseEventTargetNode, then I'm noot sure why it's right to
> unconditionally return false. After all, we dispatched a mouse event to the
> node already. Since in that case we are trying to affect only the focus
> behavior then I think the code should go inside the mousedownEvent if
> statement.

I see your point here too. But both Firefox and Opera dispatch both MouseDown and MouseUp for clicking on scrollbars (but *not* MousePress), and in both vendors, the targetNode is <HTML>. See the attachment " Blur behaviour in various browsers" from Tor Arne.

> To be sure this is right, I'd need to confirm that we want to avoid setting the
> focused node, but we do want to dispatch the mouse event.

Imho, we would be compliant to other vendors that way.
Comment 19 Antonio Gomes 2010-04-28 10:25:42 PDT
Created attachment 54576 [details]
patch v2

Based on my arguments on Darin's concern, I kept the same logic as in patch v1.1, but:

> Need braces around the body of this if statement.

Fixed.

> Since in that case we are trying to affect only the focus
> behavior then I think the code should go inside the mousedownEvent if
> statement.

Fixed.
Comment 20 Antonio Gomes 2010-04-28 10:30:15 PDT
Based on the discussion of have three issues/situations here:

1) Clicking on frame scrollbars currently blurs any focused node in content.

That is what patch tried to avoid. Patch only affect ports that do not use native widgets for scrollbars.

2) Do we want clicking on frame scrollbars dispatching events to content at all?

Patch keeps the same behavior as before it. If we decide to not dispatch events at all, I can do that, in a follow up. bug 29484 is all about it, and is also on my plate

3) Why HitTest does not set HitTest.scrollbar when clicking on Frame scrollbars?

Patch does not change that as well, but could be done in a followup indeed.
Comment 21 Darin Adler 2010-04-28 10:39:58 PDT
Comment on attachment 54576 [details]
patch v2

Why is it helpful to do the scrollbarAtPoint check in a case where the function doesn't do anything else? If you think the check needs to be outside the mousedownEvent if statement, then I think the test needs to check for more than just the click event. It needs to actually test the mouseup and mousedown.
Comment 22 Antonio Gomes 2010-04-28 11:24:08 PDT
(In reply to comment #21)
> (From update of attachment 54576 [details])
> Why is it helpful to do the scrollbarAtPoint check in a case where the function
> doesn't do anything else? If you think the check needs to be outside the
> mousedownEvent if statement, then I think the test needs to check for more than
> just the click event. It needs to actually test the mouseup and mousedown.

I did not get you question, sorry. Could you please clarify?
Comment 23 Darin Adler 2010-04-28 11:25:14 PDT
(In reply to comment #22)
> I did not get you question, sorry. Could you please clarify?

In my opinion, the code to return early should be inside this if statement:

    if (!swallowEvent && eventType == eventNames().mousedownEvent) {

Not outside it.

If you think the code belongs outside the if statement, it must be because of some event other than the mousedown event.
Comment 24 Antonio Gomes 2010-04-28 11:31:46 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > I did not get you question, sorry. Could you please clarify?
> 
> In my opinion, the code to return early should be inside this if statement:
> 
>     if (!swallowEvent && eventType == eventNames().mousedownEvent) {
> 
> Not outside it.
> 
> If you think the code belongs outside the if statement, it must be because of
> some event other than the mousedown event.

Thanks for clarifying. I agree with you, check should go inside.

I can change the patch
Comment 25 Antonio Gomes 2010-04-28 12:17:52 PDT
Created attachment 54609 [details]
(committed in r58476) patch v3

> In my opinion, the code to return early should be inside this if statement:
>     if (!swallowEvent && eventType == eventNames().mousedownEvent) {
> Not outside it.

Fixed.
Comment 26 Antonio Gomes 2010-04-29 06:29:15 PDT
Comment on attachment 54609 [details]
(committed in r58476) patch v3

Clearing flags on attachment: 54609

Committed r58476: <http://trac.webkit.org/changeset/58476>
Comment 27 Antonio Gomes 2010-04-29 06:29:42 PDT
Thank you Darin!