Fix gesture scrolling when the target-element of scroll-begin is removed
Created attachment 176161 [details] Patch
Comment on attachment 176161 [details] Patch Attachment 176161 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14991828
Comment on attachment 176161 [details] Patch Attachment 176161 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14985971
Created attachment 176166 [details] Patch
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
(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.
Created attachment 176281 [details] Changed the logic a little bit to fix the failing tests.
(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. :)
(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!
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?
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.
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+
Comment on attachment 176281 [details] Changed the logic a little bit to fix the failing tests. Thanks! Requesting cq
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>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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
This patch breaks scrolling of overflow-scrollable divs contained in iframes.