Bug 142789

Summary: Scroll latching logic can get stuck in 'scrollable="no"' iframes
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, commit-queue, jamesr, luiz, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 145574    
Attachments:
Description Flags
Patch
none
Patch dino: review+

Description Brent Fulgham 2015-03-17 13:19:12 PDT
Certain advertisements on website are hosted inside iframes that have content areas that are larger than their on-page dimensions. The iframes are marked with 'scrollinge="no"' to prevent them from scrolling content.

The latching logic didn't understand this case, and would only consider whether the iframe had scrollable content (which it does in this case), ignoring the 'scrolling="no"' declaration.
Comment 1 Brent Fulgham 2015-03-17 13:20:17 PDT
<rdar://problem/20129494>
Comment 2 Brent Fulgham 2015-03-17 15:59:15 PDT
Created attachment 248881 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-03-17 16:11:52 PDT
Comment on attachment 248881 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:764
> +    if (is<Element>(container)) {
> +        const auto& scrollingAttribute = downcast<Element>(container).fastGetAttribute(HTMLNames::scrollingAttr);
> +        if (scrollingAttribute == "no")
> +            return true;
> +    }

It seems odd to do this without checking that the element is an iframe.

I still think the rest of this function is confused about the relationship between the container node and the scrollable area.
Comment 4 Brent Fulgham 2015-03-23 13:49:54 PDT
Created attachment 249261 [details]
Patch
Comment 5 Dean Jackson 2015-03-23 14:07:33 PDT
Comment on attachment 249261 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:948
> +        // WebKit2 code path

Did you mean to leave this in?

> Source/WebCore/page/mac/EventHandlerMac.mm:964
> +        // If the platform widget is handling the event, we always want to return false

Another missing full stop. :)
Comment 6 Brent Fulgham 2015-03-23 15:53:05 PDT
Committed r181879: <http://trac.webkit.org/changeset/181879>
Comment 7 Simon Fraser (smfr) 2015-03-25 21:27:15 PDT
Comment on attachment 249261 [details]
Patch

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

Why no tests?

> Source/WebCore/ChangeLog:14
> +            different.

differently :)

> Source/WebCore/page/mac/EventHandlerMac.mm:748
> +        if (is<HTMLIFrameElement>(candidate))
> +            continue;

I think you could check this outside the loop, because you'll never hit children of a HTMLIFrameElement.
Comment 8 Tim Horton 2015-09-04 23:58:31 PDT
Comment on attachment 249261 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:-877
> -            latchingState->setStartedGestureAtScrollLimit(scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea, wheelEvent.deltaX(), wheelEvent.deltaY()));

You removed the only line of code that ever setStartedGestureAtScrollLimit to something non-false. I think this was an accident? Will continue investigating.