Bug 153398 – ScrollAnimator is not notified when mouse entered, moved or exited a RenderListBox

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 269667 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 269667 [details]
Patch

Is this testable?

[Darin Adler]

Comment on attachment 269667 [details]
Patch

Fix looks great. Needs a test.

[Carlos Garcia Campos]

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.

[Beth Dakin]

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

[Michael Catanzaro]

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

[Simon Fraser (smfr)]

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

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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.

[Beth Dakin]

(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

[Carlos Garcia Campos]

Committed r195659: <http://trac.webkit.org/changeset/195659>