Bug 139914

Summary: [Mac] Cannot scroll when a non-scrollable iframe is contained inside a scrollable iframe
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, jberlin, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: All   
Bug Depends on:    
Bug Blocks: 140564    
Attachments:
Description Flags
Patch darin: review+

Description Brent Fulgham 2014-12-23 14:12:20 PST
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."
Comment 1 Brent Fulgham 2014-12-23 14:12:38 PST
<rdar://problem/18750910>
Comment 2 Brent Fulgham 2014-12-23 14:47:35 PST
Created attachment 243696 [details]
Patch
Comment 3 Darin Adler 2014-12-24 12:44:54 PST
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;
    }
Comment 4 Brent Fulgham 2015-01-05 09:27:34 PST
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!
Comment 5 Brent Fulgham 2015-01-05 10:11:54 PST
Committed r177912: <http://trac.webkit.org/changeset/177912>