Bug 139914 - [Mac] Cannot scroll when a non-scrollable iframe is contained inside a scrollable iframe
Summary: [Mac] Cannot scroll when a non-scrollable iframe is contained inside a scroll...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 140564
  Show dependency treegraph
 
Reported: 2014-12-23 14:12 PST by Brent Fulgham
Modified: 2015-01-16 14:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (19.55 KB, patch)
2014-12-23 14:47 PST, Brent Fulgham
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>