[webkit-reviews] review granted: [Bug 96335] Clicking a scrollbar unfocuses the current activeElement : [Attachment 177323] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 96335: Clicking a scrollbar unfocuses the current activeElement
https://bugs.webkit.org/show_bug.cgi?id=96335

Attachment 177323: Patch
https://bugs.webkit.org/attachment.cgi?id=177323&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.