Bug 139914 – [Mac] Cannot scroll when a non-scrollable iframe is contained inside a scrollable iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

Simon and Jessie discovered a problem with scrolling when a non-scrollable iframe is nested inside a scrollable one. We latch to the non-scrollable frame and don't recognize that we need to relay scroll events to the parent.

As Simon wrote to me:
"The latching logic is broken for a non-scrollable iframe inside a scrollable iframe. It happens to work for a non-scrollable iframe in the main frame because, after EventHandler::platformCompleteWheelEvent() does latchingState->setFrame(nullptr), the mainframe's view then gets returned from frameViewForLatchingState() on the next event."

[Brent Fulgham]

<rdar://problem/18750910>

[Brent Fulgham]

Created attachment 243696 [details]
Patch

[Darin Adler]

Comment on attachment 243696 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2751
> +    Element* stopElement = (latchedState) ? latchedState->previousWheelScrolledElement() : nullptr;

I think this would read better without the parentheses around latchedState. I can understand a coding style where we’d use parentheses in contexts like that to emphasize that ? : is like an if statement, but that is not the coding style we have been using in WebKit up until this point.

> Source/WebCore/page/MainFrame.cpp:92
> +    if (!m_latchingState.size())

Nicer to use isEmpty here.

> Source/WebCore/page/MainFrame.h:74
> +    Vector<std::unique_ptr<ScrollLatchingState>> m_latchingState;

Why do these need to be allocated on the heap? Can’t this just be Vector<ScrollLatchingState> instead?

> Source/WebCore/page/mac/EventHandlerMac.mm:828
> +static bool latchingIsLockedToParentOfThisFrame(const Frame& frame)

I think the name should say "ancestor", not "parent", since a grandparent or great grandparent also causes this function to return true.

> Source/WebCore/page/mac/EventHandlerMac.mm:832
> +    if (!latchedState || !latchedState->frame())
> +        return false;

Do we need to null check latchedState->frame()? Code below works even if it’s null. Is that a case we need to optimize?

> Source/WebCore/page/mac/EventHandlerMac.mm:848
> +    FrameTree* frameTree = &frame.tree();
> +
> +    while (frameTree) {
> +        Frame* parent = frameTree->parent();
> +        if (!parent)
> +            break;
> +
> +        if (parent == latchedState->frame())
> +            return true;
> +
> +        frameTree = &parent->tree();
> +    }

Lets write this as a for loop. This would be the conventional way to write it:

    for (Frame* ancestor = &frame.tree().parent(); ancestor; ancestor = ancestor->tree().parent()) {
        if (ancestor == latchedState->frame())
            return true;
    }

[Brent Fulgham]

Comment on attachment 243696 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2751
>> +    Element* stopElement = (latchedState) ? latchedState->previousWheelScrolledElement() : nullptr;
> 
> I think this would read better without the parentheses around latchedState. I can understand a coding style where we’d use parentheses in contexts like that to emphasize that ? : is like an if statement, but that is not the coding style we have been using in WebKit up until this point.

Understood. I will change it.

>> Source/WebCore/page/MainFrame.cpp:92
>> +    if (!m_latchingState.size())
> 
> Nicer to use isEmpty here.

Done.

>> Source/WebCore/page/MainFrame.h:74
>> +    Vector<std::unique_ptr<ScrollLatchingState>> m_latchingState;
> 
> Why do these need to be allocated on the heap? Can’t this just be Vector<ScrollLatchingState> instead?

You are right. I'll change the implementation.

>> Source/WebCore/page/mac/EventHandlerMac.mm:828
>> +static bool latchingIsLockedToParentOfThisFrame(const Frame& frame)
> 
> I think the name should say "ancestor", not "parent", since a grandparent or great grandparent also causes this function to return true.

Done.

>> Source/WebCore/page/mac/EventHandlerMac.mm:832
>> +        return false;
> 
> Do we need to null check latchedState->frame()? Code below works even if it’s null. Is that a case we need to optimize?

I think it's worth checking. If no latching has taken place yet, the latchedState->frame() will be null and we should immediately return false. There's no sense walking up the chain of ancestors and comparing each of them to null when we can just bail out early.

>> Source/WebCore/page/mac/EventHandlerMac.mm:848
>> +    }
> 
> Lets write this as a for loop. This would be the conventional way to write it:
> 
>     for (Frame* ancestor = &frame.tree().parent(); ancestor; ancestor = ancestor->tree().parent()) {
>         if (ancestor == latchedState->frame())
>             return true;
>     }

Done. That is much nicer!

[Brent Fulgham]

Committed r177912: <http://trac.webkit.org/changeset/177912>