Bug 136729 – Latching in iframes is not working as expected

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 237921 [details]
Test case

[Brent Fulgham]

Ugh!

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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?

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/18370549>

[Brent Fulgham]

Created attachment 238270 [details]
Patch

[Brent Fulgham]

Created attachment 238274 [details]
Patch

[Brent Fulgham]

Created attachment 238275 [details]
Updated to correct Windows build.

[Brent Fulgham]

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.

[Brent Fulgham]

Looks like I need to update the CMake files!

[Brent Fulgham]

Created attachment 238312 [details]
Corrected Makefiles for GTK/EFL use.

[Brent Fulgham]

Comment on attachment 238312 [details]
Corrected Makefiles for GTK/EFL use.

Pulling from review due to a crash when destroying the MainFrame.

[Brent Fulgham]

Created attachment 238322 [details]
Fixed crash in destructor call

[Simon Fraser (smfr)]

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

[Brent Fulgham]

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.

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Looks like WK1 scrolling is broken. Working on a fix.

[Brent Fulgham]

Created attachment 238381 [details]
Patch

[Simon Fraser (smfr)]

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

[Brent Fulgham]

Created attachment 238388 [details]
Patch

[Brent Fulgham]

FYI - The patch builds locally on Windows. I'm not sure why EWS is failing.

[Brent Fulgham]

Created attachment 238397 [details]
Patch

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

Committed r173784: <http://trac.webkit.org/changeset/173784>