Bug 153398

Summary: ScrollAnimator is not notified when mouse entered, moved or exited a RenderListBox
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, darin, mcatanzaro, simon.fraser
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 153405    
Attachments:
Description Flags
Patch mcatanzaro: review+

Description Carlos Garcia Campos 2016-01-23 04:02:45 PST
EvenHandler is checking whether the enclosing layer of a node is registered as scrollable area of its frame view. That doesn't work for list boxes, because they are the scrollable area themselves. Also when entering a list box the node under mouse it not usually the list box itself, but any of its children, a HTMLOptionElement or a HTMLOptGroupElement. Instead of comparing layers, we should find the enclosing scrollable area of the target elements and compare them to decide whether the mouse has entered, left or moved a scrollable area.
Comment 1 Carlos Garcia Campos 2016-01-23 04:08:49 PST
Created attachment 269667 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-01-24 10:49:10 PST
Comment on attachment 269667 [details]
Patch

Is this testable?
Comment 3 Darin Adler 2016-01-24 14:17:51 PST
Comment on attachment 269667 [details]
Patch

Fix looks great. Needs a test.
Comment 4 Carlos Garcia Campos 2016-01-25 01:24:42 PST
I'm not sure how to test this, since it only affects the scroll animator notifications, you can still scroll and use the list box normally.
Comment 5 Beth Dakin 2016-01-25 11:06:55 PST
(In reply to comment #4)
> I'm not sure how to test this, since it only affects the scroll animator
> notifications, you can still scroll and use the list box normally.

Yeah, we never figured out how to test this stuff, which is really unfortunate because we definitely have a history of accidentally introducing regressions in this area.
Comment 6 Michael Catanzaro 2016-01-25 12:14:08 PST
Comment on attachment 269667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269667&action=review

OK then. Since it's apparently hard to test, and we (GTK folks) will notice almost immediately if this ever breaks, I think a test is not needed....

> Source/WebCore/ChangeLog:11
> +        themselves. Also when entering a list box the node under mouse it

it -> is
Comment 7 Simon Fraser (smfr) 2016-01-25 12:58:04 PST
(In reply to comment #6)
> Comment on attachment 269667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269667&action=review
> 
> OK then. Since it's apparently hard to test, and we (GTK folks) will notice
> almost immediately if this ever breaks, I think a test is not needed....

I disagree. If you want to avoid Mac people breaking GTK by accident, you should add a test.
Comment 8 Michael Catanzaro 2016-01-25 13:50:50 PST
(In reply to comment #7)
> I disagree. If you want to avoid Mac people breaking GTK by accident, you
> should add a test.

A test won't help one bit, since the EWS only runs tests for Mac, you will never notice when you break it, and it takes us weeks or months to triage and file bugs for failing tests. We will definitely notice if scrollbars break long before we notice the test failing.

That said, of course it would be better to have a test than not! But I would not want to block the feature on this.
Comment 9 Carlos Garcia Campos 2016-01-25 23:30:07 PST
I also don't want to block this on a test, but agree that we need tests for this, so I'll work on adding tests after these patches land.
Comment 10 Beth Dakin 2016-01-26 11:12:49 PST
(In reply to comment #9)
> I also don't want to block this on a test, but agree that we need tests for
> this, so I'll work on adding tests after these patches land.

YAAAAY
Comment 11 Carlos Garcia Campos 2016-01-27 02:25:13 PST
Committed r195659: <http://trac.webkit.org/changeset/195659>