Scroll latching logic can get stuck in 'scrollable="no"' iframes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/20129494>

[Brent Fulgham]

Created attachment 248881 [details]
Patch

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

Created attachment 249261 [details]
Patch

[Dean Jackson]

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. :)

[Brent Fulgham]

Committed r181879: <http://trac.webkit.org/changeset/181879>

[Simon Fraser (smfr)]

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.

[Tim Horton]

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.