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:

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

Michael Brasser

Reported 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.

Attachments
Same testcase as in #4, but attached to fix the linebreak (716 bytes, text/html) 2008-04-22 07:30 PDT, Tor Arne Vestbø	no flags	Details
Blur behaviour in various browsers (86.95 KB, image/png) 2008-04-24 05:07 PDT, Tor Arne Vestbø	no flags	Details
patch v0 - WIP (6.19 KB, patch) 2010-04-23 12:57 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
patch v1 (11.44 KB, patch) 2010-04-26 19:58 PDT, Antonio Gomes	darin: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
patch v1.1 (11.39 KB, patch) 2010-04-26 20:06 PDT, Antonio Gomes	darin: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
patch v2 (11.66 KB, patch) 2010-04-28 10:25 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
(committed in r58476) patch v3 (11.64 KB, patch) 2010-04-28 12:17 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Michael Brasser

Comment 1 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))

Tor Arne Vestbø

Comment 2 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>

Tor Arne Vestbø

Comment 3 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.

Tor Arne Vestbø

Comment 4 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.

Tor Arne Vestbø

Comment 5 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.

Tor Arne Vestbø

Comment 6 2008-04-22 07:30:54 PDT

Created attachment 20748 [details] Same testcase as in #4, but attached to fix the linebreak

Tor Arne Vestbø

Comment 7 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.

Tor Arne Vestbø

Comment 8 2008-04-23 04:23:27 PDT

Yet another testcase :) http://chaos.troll.no/~tavestbo/webkit/mouse-events.html

mitz

Comment 9 2008-04-23 08:25:44 PDT

<rdar://problem/5586891>

Tor Arne Vestbø

Comment 10 2008-04-23 08:38:46 PDT

I messed up, didn't mean to mark as RESOLVED INVALID

Tor Arne Vestbø

Comment 11 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?

Antonio Gomes

Comment 12 2010-04-23 12:57:53 PDT

Created attachment 54182 [details] patch v0 - WIP ++functional ++has_layout test --needs some love before review

Antonio Gomes

Comment 13 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

Antonio Gomes

Comment 14 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.

Darin Adler

Comment 15 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.

Antonio Gomes

Comment 16 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?

Darin Adler

Comment 17 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.

Antonio Gomes

Comment 18 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.

Antonio Gomes

Comment 19 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.

Antonio Gomes

Comment 20 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.

Darin Adler

Comment 21 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.

Antonio Gomes

Comment 22 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?

Darin Adler

Comment 23 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.

Antonio Gomes

Comment 24 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

Antonio Gomes

Comment 25 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.

Antonio Gomes

Comment 26 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>

Antonio Gomes

Comment 27 2010-04-29 06:29:42 PDT

Thank you Darin!

Note You need to log in before you can comment on or make changes to this bug.