Bug 103355 – Fix gesture scrolling when the target-element of scroll-begin is removed

• Component: New Bugs
• Status: REOPENED

[Sadrul Habib Chowdhury]

Fix gesture scrolling when the target-element of scroll-begin is removed

[Sadrul Habib Chowdhury]

Created attachment 176161 [details]
Patch

[EFL EWS Bot]

Comment on attachment 176161 [details]
Patch

Attachment 176161 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14991828

[Build Bot]

Comment on attachment 176161 [details]
Patch

Attachment 176161 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14985971

[Sadrul Habib Chowdhury]

Created attachment 176166 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176166 [details]
Patch

Attachment 176166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15010123

New failing tests:
fast/events/touch/gesture/touch-gesture-scroll-iframe.html
platform/chromium/plugins/gesture-events.html
platform/chromium/plugins/gesture-events-scrolled.html
fast/events/touch/gesture/touch-gesture-scroll-page.html

[Antonio Gomes]

(In reply to comment #5)
> (From update of attachment 176166 [details])
> Attachment 176166 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15010123
> 
> New failing tests:
> fast/events/touch/gesture/touch-gesture-scroll-iframe.html
> platform/chromium/plugins/gesture-events.html
> platform/chromium/plugins/gesture-events-scrolled.html
> fast/events/touch/gesture/touch-gesture-scroll-page.html

Look like real failures.

[Sadrul Habib Chowdhury]

Created attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

[Sadrul Habib Chowdhury]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 176166 [details] [details])
> > Attachment 176166 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/15010123
> > 
> > New failing tests:
> > fast/events/touch/gesture/touch-gesture-scroll-iframe.html
> > platform/chromium/plugins/gesture-events.html
> > platform/chromium/plugins/gesture-events-scrolled.html
> > fast/events/touch/gesture/touch-gesture-scroll-page.html
> 
> Look like real failures.

Indeed :(

I changed the logic to do essentially the same thing, but a bit differently, and the new function will now return the same node if there is no such scrollable node in the document. This fixes the tests. :)

[Sadrul Habib Chowdhury]

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 176166 [details] [details] [details])
> > > Attachment 176166 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/15010123
> > > 
> > > New failing tests:
> > > fast/events/touch/gesture/touch-gesture-scroll-iframe.html
> > > platform/chromium/plugins/gesture-events.html
> > > platform/chromium/plugins/gesture-events-scrolled.html
> > > fast/events/touch/gesture/touch-gesture-scroll-page.html
> > 
> > Look like real failures.
> 
> Indeed :(
> 
> I changed the logic to do essentially the same thing, but a bit differently, and the new function will now return the same node if there is no such scrollable node in the document. This fixes the tests. :)

Yay! The EWS runs are all green!

[Antonio Gomes]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

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

> Source/WebCore/page/EventHandler.cpp:287
> +static Node* getClosestScrollableNodeInDocumentIfPossible(Node* node)
> +{
> +    Node* firstNode = node;
> +    while (node) {
> +        if (node->isDocumentNode())
> +            return firstNode;

could you explain this part? is firstNode ensured to be scrollable?

[Sadrul Habib Chowdhury]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

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

>> Source/WebCore/page/EventHandler.cpp:287
>> +            return firstNode;
> 
> could you explain this part? is firstNode ensured to be scrollable?

It's not. If there is no scrollable node in the document that contains |node|, then this function returns the original node that was sent to the function (which is why the 'IfPossible' suffix), i.e. the node from the hit-test result.

The current behaviour is that it always uses the node from the hit test result. The change in this patch tries to use a scrollable node that contains the hit node. But if there is no such scrollable node, then it falls back to the current behaviour.

[Antonio Gomes]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

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

>>> Source/WebCore/page/EventHandler.cpp:287
>>> +            return firstNode;
>> 
>> could you explain this part? is firstNode ensured to be scrollable?
> 
> It's not. If there is no scrollable node in the document that contains |node|, then this function returns the original node that was sent to the function (which is why the 'IfPossible' suffix), i.e. the node from the hit-test result.
> 
> The current behaviour is that it always uses the node from the hit test result. The change in this patch tries to use a scrollable node that contains the hit node. But if there is no such scrollable node, then it falls back to the current behaviour.

Ah I was confusing ISdocumentNode with InDocument method. It makes sense, sure. r+

[Sadrul Habib Chowdhury]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

Thanks! Requesting cq

[WebKit Review Bot]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

Clearing flags on attachment: 176281

Committed r136012: <http://trac.webkit.org/changeset/136012>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 176281 [details]
Changed the logic a little bit to fix the failing tests.

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

> Source/WebCore/page/EventHandler.cpp:282
> +static Node* getClosestScrollableNodeInDocumentIfPossible(Node* node)

WebKit coding style does not use the word “get” in these function names.

> Source/WebCore/page/EventHandler.cpp:284
> +    Node* firstNode = node;

The word “first” in this variable name does not make all that much sense. In what sequence is this node “first”?

> Source/WebCore/page/EventHandler.cpp:285
> +    while (node) {

Normally we’d use a for loop here rather than using a while loop.

[Antonio Gomes]

(In reply to comment #16)
> (From update of attachment 176281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176281&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:282
> > +static Node* getClosestScrollableNodeInDocumentIfPossible(Node* node)
> 
> WebKit coding style does not use the word “get” in these function names.
> 
> > Source/WebCore/page/EventHandler.cpp:284
> > +    Node* firstNode = node;
> 
> The word “first” in this variable name does not make all that much sense. In what sequence is this node “first”?
> 
> > Source/WebCore/page/EventHandler.cpp:285
> > +    while (node) {
> 
> Normally we’d use a for loop here rather than using a while loop.

Agreed. My bad and thanks Darin for catching these.

Sadrul, please address as a follow up, once it has landed.

[Sadrul Habib Chowdhury]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 176281 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=176281&action=review
> > 
> > > Source/WebCore/page/EventHandler.cpp:282
> > > +static Node* getClosestScrollableNodeInDocumentIfPossible(Node* node)
> > 
> > WebKit coding style does not use the word “get” in these function names.
> > 
> > > Source/WebCore/page/EventHandler.cpp:284
> > > +    Node* firstNode = node;
> > 
> > The word “first” in this variable name does not make all that much sense. In what sequence is this node “first”?
> > 
> > > Source/WebCore/page/EventHandler.cpp:285
> > > +    while (node) {
> > 
> > Normally we’d use a for loop here rather than using a while loop.
> 
> Agreed. My bad and thanks Darin for catching these.
> 
> Sadrul, please address as a follow up, once it has landed.

Submitted a cleanup patch in: https://bugs.webkit.org/show_bug.cgi?id=103543

[Robert Kroeger]

This patch breaks scrolling of overflow-scrollable divs contained in iframes.