Created attachment 163242 [details] Reduction If you activate a textbox and then scroll a div the textbox loses focus. There's special code in EventHandler for the frame's scrollbar to not move the focus, but overflow scrollbars are not handled properly.
Note: There's a catch here. We *should* move the focus when clicking the scrollbar in a <textarea>, but not for other elements.
(In reply to comment #1) > Note: There's a catch here. We *should* move the focus when clicking the scrollbar in a <textarea>, but not for other elements. This is actually true for all form controls, not just textarea. ex. Clicking the scrollbar in a multi select.
Created attachment 163277 [details] Patch Don't move focus when clicking scrollbars unless inside an enabled form control.
Created attachment 163435 [details] Patch Pass the IntPoint instead of the whole PlatformMouseEvent.
Comment on attachment 163435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163435&action=review > Source/WebCore/ChangeLog:13 > + This patch also simplifies handleMousePressEvent and removes some dead code related to > + widget based input controls that no longer exist. can the removal be on its own patch?
Comment on attachment 163435 [details] Patch Yeah, this patch does too much. We need to separate the two changes. Historically, this code is very under-tested so making a big change like this is likely to cause a regression.
Created attachment 164596 [details] Patch Remove all the cleanup code. Only fix the bug.
Comment on attachment 164596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164596&action=review Implementation looks good. Most important thing now is to decide that is the behavior we want. Could you add to your changelog other browsers' behavior we now match/dismatch? One question below: > Source/WebCore/page/EventHandler.cpp:2298 > + if (FrameView* view = m_frame->view()) { > + if (view->scrollbarAtPoint(windowPoint)) > + return true; > + } > + > + if (RenderView* renderView = m_frame->contentRenderer()) { > + HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); > + HitTestResult result(windowPoint); > + renderView->hitTest(request, result); > + return result.scrollbar(); > + } > + > + return false; Do we need both ifs? when would one fail and the other not?
Comment on attachment 164596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164596&action=review > Source/WebCore/page/EventHandler.cpp:2263 > + // Don't change the focus if the click is inside a scrollbar unless it's in a form control. Interpreting a sentence that contains a negation tends to be more difficult than interpreting a sentence that is written in a positive form (i.e. doesn't contain with a negation). This is exacerbated when such a sentence contains more than one negation, especially a "unless" clause. See my remark below for one idea how we can encode the content of this comment into a more readable for by taking advantage of the flow control structure (below) to provide context for an inline comment. > Source/WebCore/page/EventHandler.cpp:2266 > + Element* element = node && node->isElementNode() ? toElement(node) : 0; > + if (!(element && element->isFormControlElement()) && isInsideScrollbar(mouseEvent.position())) > + return true; This code is OK as-is. We can simplify this code, noticing that element is null when either node is null or node->isElementNode() evaluates to false, to read: if (node && node->isElementNode()) { if (!toElement(node)->isFormControlElement() && isInsideScrollbar(mouseEvent.position())) return true; // Don't change focus. }
(In reply to comment #8) > (From update of attachment 164596 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164596&action=review > > ... > Do we need both ifs? when would one fail and the other not? I'm not sure if it's possible. If you look at FrameLoader::open we can create a Frame and set a FrameView, but it's not clear that the document will be attached immediately, or if you could have a lazy attach on a loading frame. It also looks like anything that goes through FrameLoader::clear will have document set to 0, removing the RenderView, but it would still have a FrameView.
Downstream Chrome bug: http://code.google.com/p/chromium/issues/detail?id=51469 After testing it appears that we match IE9 and Opera with our current behavior, and it's Gecko that doesn't move the focus. Hmm... so which one should we match?
> After testing it appears that we match IE9 and Opera with our current behavior, and it's Gecko that doesn't move the focus. Hmm... so which one should we match? It might be worth talking with folks in #developers on irc.mozilla.org to see if they're interested in changing. If Gecko changed, then all the browsers would work the same way, right?
(In reply to comment #12) > > After testing it appears that we match IE9 and Opera with our current behavior, and it's Gecko that doesn't move the focus. Hmm... so which one should we match? > > It might be worth talking with folks in #developers on irc.mozilla.org to see if they're interested in changing. If Gecko changed, then all the browsers would work the same way, right? The original bug referenced this stackoverflow post that mentions IE8, Firefox and Safari didn't have this behavior, but I just tested IE8/WinXP and IE9/Win7 and it does move the focus: http://stackoverflow.com/questions/1345473/google-chrome-focus-issue-with-the-scrollbar Firefox's behavior of not moving the focus seems more logical to me. Just because I scrolled down a form doesn't mean I wanted to unfocus the input I was typing into. It's also weird that the FrameView's scrollbars are special, it means you don't get the same behavior from a full height div with an overflow scrollbar (ex. Google+).
Created attachment 164784 [details] Patch Review comment changes.
Comment on attachment 164784 [details] Patch Attachment 164784 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13903656 New failing tests: scrollbars/scrollbar-iframe-click-does-not-blur-content.html scrollbars/scrollbar-click-does-not-blur-content.html fast/events/mousedown-in-subframe-scrollbar.html fast/overflow/scrollbar-click-retains-focus.html
Comment on attachment 164596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164596&action=review >> Source/WebCore/page/EventHandler.cpp:2266 >> + return true; > > This code is OK as-is. We can simplify this code, noticing that element is null when either node is null or node->isElementNode() evaluates to false, to read: > > if (node && node->isElementNode()) { > if (!toElement(node)->isFormControlElement() && isInsideScrollbar(mouseEvent.position())) > return true; // Don't change focus. > } Your suggested code is not correct and is not logically equivalent to the original code. It now fails for cases where there is no node, but the click is inside a scrollbar (ie. it's the FrameView's scrollbar).
Created attachment 164807 [details] Patch
(In reply to comment #16) > [...] > Your suggested code is not correct and is not logically equivalent to the original code. It now fails for cases where there is no node, but the click is inside a scrollbar (ie. it's the FrameView's scrollbar). You're right. Disregard my suggestion.
Comment on attachment 164807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164807&action=review > Source/WebCore/page/EventHandler.cpp:2265 > + if ((!element || !element->isFormControlElement()) && isInsideScrollbar(mouseEvent.position())) We probably also want to move focus for contentEditable elements?
The OS X, Windows and Ubuntu native UIs don't move focus when you click on a scrollbar and WebKit doesn't move focus if you click on the viewport scrollbar. So, it seems like the most consistent thing is to not move focus when clicking on scrollbars. It's not clear to me if we should be moving focus when you click on a textarea/contentEditable scrollbar. Seems more consistent to not do so. I'm not sure if there's a native platform convention here. Would be nice to get consensus on this in a spec somewhere. HTML talks about activeElements. Maybe WHATWG?
Comment on attachment 164807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164807&action=review >> Source/WebCore/page/EventHandler.cpp:2265 >> + if ((!element || !element->isFormControlElement()) && isInsideScrollbar(mouseEvent.position())) > > We probably also want to move focus for contentEditable elements? Firefox actually looks at whether the element is mouseFocusable. So, if we're going to have a check here, it should be that.
Created attachment 177323 [details] Patch
(In reply to comment #22) > Created an attachment (id=177323) [details] > Patch Updated to now check isMouseFocusable and made the test cover these cases.
Comment on attachment 177323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177323&action=review Please make sure to address the non-nits before committing. > Source/WebCore/page/EventHandler.cpp:2398 > + if ((!node || !node->isMouseFocusable()) && isInsideScrollbar(mouseEvent.position())) Looks like you can remove the null checks in the code below now that you're null-checking node here. > Source/WebCore/page/EventHandler.cpp:2420 > + HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); I don't think you need HitTestRequest::Active here. I know a bunch of places pass both ReadOnly and Active, but looking at the code, I expect they're all wrong. Not sure. Did you find something broke without the Active? > LayoutTests/fast/overflow/scrollbar-click-retains-focus.html:50 > +function click(x, y) { Nit: curly should go on new line. The de-facto style for JS is that we put the curly on new lines for names functions, but not anonymous ones. > LayoutTests/fast/overflow/scrollbar-click-retains-focus.html:79 > + shouldBeEqualToString("document.activeElement.tagName", "INPUT"); Won't these fail on platforms that use overlay scrollbars or do overlay scrollbars always show up for overflow:scroll? I'm not sure if there's a way to test overlay scrollbars at all. You could make the test work by measuring the width of the scrollbar (subtract clientWidth from offsetWidth on an overflow:scroll element). Or you could WONTFIX it for platforms with overlay scrollbars.
Comment on attachment 177323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177323&action=review >> Source/WebCore/page/EventHandler.cpp:2398 >> + if ((!node || !node->isMouseFocusable()) && isInsideScrollbar(mouseEvent.position())) > > Looks like you can remove the null checks in the code below now that you're null-checking node here. You can't because this null check only counts if the click is inside a scrollbar, notice the parens. >> Source/WebCore/page/EventHandler.cpp:2420 >> + HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); > > I don't think you need HitTestRequest::Active here. I know a bunch of places pass both ReadOnly and Active, but looking at the code, I expect they're all wrong. Not sure. Did you find something broke without the Active? Oh wow, it seems the code has changed quite a big since I originally uploaded this patch. At one point you needed to pass both. >> LayoutTests/fast/overflow/scrollbar-click-retains-focus.html:79 >> + shouldBeEqualToString("document.activeElement.tagName", "INPUT"); > > Won't these fail on platforms that use overlay scrollbars or do overlay scrollbars always show up for overflow:scroll? I'm not sure if there's a way to test overlay scrollbars at all. You could make the test work by measuring the width of the scrollbar (subtract clientWidth from offsetWidth on an overflow:scroll element). Or you could WONTFIX it for platforms with overlay scrollbars. DRT always uses regular scrollbars unless you do setUsesOverlayScrollbars so that shouldn't be the case.
Created attachment 177389 [details] Patch for landing
Comment on attachment 177389 [details] Patch for landing Clearing flags on attachment: 177389 Committed r136642: <http://trac.webkit.org/changeset/136642>
All reviewed patches have been landed. Closing bug.