Bug 145574 – REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both select and iframe

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

[Brent Fulgham]

After <http://trac.webkit.org/changeset/181879>, scrolling in select/option region in iFrame scrolls both select and iframe.

[Brent Fulgham]

<rdar://problem/20966828>

[Brent Fulgham]

Created attachment 254110 [details]
Test Case

[Brent Fulgham]

This bug was caused by my recent refactoring in Bug 142789. It did not properly include the RenderListBox logic from the original version of the code.

[Brent Fulgham]

Created attachment 254123 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 254123 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        we were incorrectly skipping this test for Select regions (RenderListBox) elements because 

because?

> Source/WebCore/page/mac/EventHandlerMac.mm:995
> +        if (!frameHasPlatformWidget(m_frame) && !latchingState->startedGestureAtScrollLimit() && (scrollableContainer == latchingState->scrollableContainer()) && ((scrollableArea && (view != scrollableArea)) || isRenderListBox(scrollableContainer))) {

It's not clear to me why list boxes are special here. Why doesn't the scrollableContainer logic just take care of list boxes?

[Brent Fulgham]

Created attachment 254133 [details]
Patch

[Brent Fulgham]

Comment on attachment 254123 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        we were incorrectly skipping this test for Select regions (RenderListBox) elements because 
> 
> because?

Doh!

>> Source/WebCore/page/mac/EventHandlerMac.mm:995
>> +        if (!frameHasPlatformWidget(m_frame) && !latchingState->startedGestureAtScrollLimit() && (scrollableContainer == latchingState->scrollableContainer()) && ((scrollableArea && (view != scrollableArea)) || isRenderListBox(scrollableContainer))) {
> 
> It's not clear to me why list boxes are special here. Why doesn't the scrollableContainer logic just take care of list boxes?

You're right. List boxes were not getting handled properly because we had a mismatch between the scrollableContainer and the scrollableArea. When latching, these two were getting out of sync. See my new patch.

[Simon Fraser (smfr)]

Comment on attachment 254133 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:903
> +static bool latchedToFrameOrBody(ContainerNode& container)
> +{
> +    return is<HTMLFrameSetElement>(container) || is<HTMLBodyElement>(container);
> +}

I'm concerned that this isn't very future-proof, when we change which element we consider page scrolling to operate on (see bug 143609). Probably OK for now though.

[Brent Fulgham]

Comment on attachment 254133 [details]
Patch

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

>> Source/WebCore/page/mac/EventHandlerMac.mm:903
>> +}
> 
> I'm concerned that this isn't very future-proof, when we change which element we consider page scrolling to operate on (see bug 143609). Probably OK for now though.

I'll add a FIXME for Bug 106133, which I think is the underlying issue?

[Brent Fulgham]

Committed r185156: <http://trac.webkit.org/changeset/185156>