Latching in iframes seems to be a bit broken right now. In the attached test case, start a strong momentum scroll gesture in the iframe. The one single gesture will continue scrolling the main frame, which shouldn't happen. Non-momentum gestures work as expected.
Created attachment 237921 [details] Test case
Ugh!
The latching logic thinks that the <iframe> is at the maximum scroll position because the scroll position is 0, and the maximum scrolled position is -1094. It's like the scrollable area coordinates are flipped for this case.
Actually, it looks like the issue here is that this test case puts us into the RenderLayer logic for maximum scroll position (and current scroll position), while the other scroll latch test cases seem to stay in the "ScrollableArea" implementation.
Created attachment 237993 [details] Counter-test case A nearly identical page where the iframe content is defined by "data" instead of a URL does not suffer from this problem.
On page load, the contentSize of the ScrollView representing the content of the <iframe> is (768x150). The <iframe> element is 300x150. The calculated maximum scroll position y coordinate is therefore 0, and we believe the content is already scrolled to the bottom.
Latching seems to set itself up properly if I do the following: 1. Load page. 2. Scroll the iframe slightly down so I am not at the upper limit of the scroll. 3. Scroll up (causing a rubber-band). 4. Now if I scroll down it is latched properly. This causes the scroll dimensions to be recalculated internally and everything works properly. It might be that after the iframe loads, the scroll bounds are not calculated without this manual step?
The problem seems to be related to the apple.com page used in the test. The page markup is causing the viewSize of the iframe to match the size of the iframe (e.g., we give the iframe a 150px height in the test case, and the page markup explicitly sets the view size to also have a 150px height). This behavior causes our latching logic to fail, because it always thinks that we are starting wheel gestures at the edge of the iframe. Consequently, all events get propagated to the enclosing parent, causing the main page to scroll once the iframe has completed scrolling.
<rdar://problem/18370549>
Created attachment 238270 [details] Patch
Created attachment 238274 [details] Patch
Created attachment 238275 [details] Updated to correct Windows build.
Comment on attachment 238275 [details] Updated to correct Windows build. There's really no need to conditionalize the presence of a boolean and one smart pointer in the LatchedState class. Removing these conditionals and building on all platforms.
Looks like I need to update the CMake files!
Created attachment 238312 [details] Corrected Makefiles for GTK/EFL use.
Comment on attachment 238312 [details] Corrected Makefiles for GTK/EFL use. Pulling from review due to a crash when destroying the MainFrame.
Created attachment 238322 [details] Fixed crash in destructor call
Comment on attachment 238322 [details] Fixed crash in destructor call View in context: https://bugs.webkit.org/attachment.cgi?id=238322&action=review > Source/WebCore/page/LatchedState.h:38 > +class LatchedState { I think this should be called ScrollLatchingState. Out of context, it's hard to know what is being latched. You could also move it to page/scrolling. > Source/WebCore/page/MainFrame.cpp:38 > + , m_latchedState(std::make_unique<LatchedState>()) Should platforms without scroll events waste memory on a latched state? > Source/WebCore/page/MainFrame.cpp:46 > + m_latchedState = nullptr; > + m_recentWheelEventDeltaTracker = nullptr; Neither is necessary. > Source/WebCore/page/MainFrame.cpp:83 > + if (!m_latchedState) > + return; This should never happen, right? > Source/WebCore/page/MainFrame.h:48 > + bool hasLatchedState() { return m_latchedState.get(); } > + LatchedState& latchedState() { return *m_latchedState; } I don't see the advantage over just returning a pointer. > LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:16 > + // The IFrame should have scrolled, but not the main page. IFrame -> iframe everywhere. > LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:34 > + var iFrameBody = window.frames['target'].document.body; iFrame -> iframe
Comment on attachment 238322 [details] Fixed crash in destructor call View in context: https://bugs.webkit.org/attachment.cgi?id=238322&action=review >> Source/WebCore/page/LatchedState.h:38 >> +class LatchedState { > > I think this should be called ScrollLatchingState. Out of context, it's hard to know what is being latched. > > You could also move it to page/scrolling. Will do! >> Source/WebCore/page/MainFrame.cpp:38 >> + , m_latchedState(std::make_unique<LatchedState>()) > > Should platforms without scroll events waste memory on a latched state? Probably not. With this refactoring, it's actually a lot easier to skip the latching stuff on these platforms. I'll change this. >> Source/WebCore/page/MainFrame.cpp:46 >> + m_recentWheelEventDeltaTracker = nullptr; > > Neither is necessary. Will do! >> Source/WebCore/page/MainFrame.cpp:83 >> + return; > > This should never happen, right? It does happen in the destructor for Frame, which ends up triggering a call to EventHandler::clear() after it has destroyed part of the Frame. >> Source/WebCore/page/MainFrame.h:48 >> + LatchedState& latchedState() { return *m_latchedState; } > > I don't see the advantage over just returning a pointer. Agreed. Changing... >> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:16 >> + // The IFrame should have scrolled, but not the main page. > > IFrame -> iframe everywhere. Done. >> LayoutTests/platform/mac/fast/scrolling/scroll-iframe-100pct.html:34 >> + var iFrameBody = window.frames['target'].document.body; > > iFrame -> iframe Done.
Comment on attachment 238322 [details] Fixed crash in destructor call Attachment 238322 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6080750784872448 New failing tests: platform/mac/fast/scrolling/scroll-iframe-latched-iframe.html
Created attachment 238325 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Looks like WK1 scrolling is broken. Working on a fix.
Created attachment 238381 [details] Patch
Comment on attachment 238381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238381&action=review > Source/WebCore/page/MainFrame.cpp:39 > +#if PLATFORM(COCOA) > + , m_latchedState(std::make_unique<ScrollLatchedState>()) > +#endif COCOA includes iOS, and I don't think you want that. > Source/WebCore/page/MainFrame.h:47 > +#if PLATFORM(COCOA) MAC? > Source/WebCore/page/mac/EventHandlerMac.mm:819 > +static bool isWK1EventForOtherPlatformFrame(const Frame& frame) We try really hard to avoid explicitly distinguishing between WK1 and WK2 in WebCore. The name of this function needs to communicate WHY WK1 needs different handling. > Source/WebCore/page/scrolling/ScrollLatchedState.h:37 > +class ScrollLatchedState { I'd prefer ScrollLatchingState
Created attachment 238388 [details] Patch
FYI - The patch builds locally on Windows. I'm not sure why EWS is failing.
Created attachment 238397 [details] Patch
Comment on attachment 238397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238397&action=review > Source/WebCore/ChangeLog:9 > + Test: platform/mac/fast/scrolling/scrolling-iframe-100pct.html I don't see this in the patch. I think the name could be improved to reflect what's being tested.
Committed r173784: <http://trac.webkit.org/changeset/173784>