Bug 96335 – Clicking a scrollbar unfocuses the current activeElement

• Component: DOM
• Status: RESOLVED FIXED

[Elliott Sprehn]

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.

[Elliott Sprehn]

Note: There's a catch here. We *should* move the focus when clicking the scrollbar in a <textarea>, but not for other elements.

[Elliott Sprehn]

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

[Elliott Sprehn]

Created attachment 163277 [details]
Patch

Don't move focus when clicking scrollbars unless inside an enabled form control.

[Elliott Sprehn]

Created attachment 163435 [details]
Patch

Pass the IntPoint instead of the whole PlatformMouseEvent.

[Antonio Gomes]

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?

[Ryosuke Niwa]

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.

[Elliott Sprehn]

Created attachment 164596 [details]
Patch

Remove all the cleanup code. Only fix the bug.

[Antonio Gomes]

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?

[Daniel Bates]

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

[Elliott Sprehn]

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

[Elliott Sprehn]

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?

[Adam Barth]

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

[Elliott Sprehn]

(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+).

[Elliott Sprehn]

Created attachment 164784 [details]
Patch

Review comment changes.

[WebKit Review Bot]

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

[Elliott Sprehn]

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

[Elliott Sprehn]

Created attachment 164807 [details]
Patch

[Daniel Bates]

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

[Ojan Vafai]

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?

[Ojan Vafai]

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?

[Ojan Vafai]

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.

[Elliott Sprehn]

Created attachment 177323 [details]
Patch

[Elliott Sprehn]

(In reply to comment #22)
> Created an attachment (id=177323) [details]
> Patch

Updated to now check isMouseFocusable and made the test cover these cases.

[Ojan Vafai]

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.

[Elliott Sprehn]

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.

[Elliott Sprehn]

Created attachment 177389 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177389 [details]
Patch for landing

Clearing flags on attachment: 177389

Committed r136642: <http://trac.webkit.org/changeset/136642>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.