Bug 145574

Summary: REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both select and iframe
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=145637
Bug Depends on: 142789    
Bug Blocks:    
Attachments:
Description Flags
Test Case
none
Patch
none
Patch simon.fraser: review+

Description Brent Fulgham 2015-06-02 15:17:04 PDT
After <http://trac.webkit.org/changeset/181879>, scrolling in select/option region in iFrame scrolls both select and iframe.
Comment 1 Brent Fulgham 2015-06-02 15:17:24 PDT
<rdar://problem/20966828>
Comment 2 Brent Fulgham 2015-06-02 15:18:01 PDT
Created attachment 254110 [details]
Test Case
Comment 3 Brent Fulgham 2015-06-02 15:19:27 PDT
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.
Comment 4 Brent Fulgham 2015-06-02 17:03:28 PDT
Created attachment 254123 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-06-02 18:05:17 PDT
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?
Comment 6 Brent Fulgham 2015-06-02 19:04:36 PDT
Created attachment 254133 [details]
Patch
Comment 7 Brent Fulgham 2015-06-02 19:06:20 PDT
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.
Comment 8 Simon Fraser (smfr) 2015-06-03 11:14:41 PDT
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.
Comment 9 Brent Fulgham 2015-06-03 11:19:09 PDT
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?
Comment 10 Brent Fulgham 2015-06-03 11:37:45 PDT
Committed r185156: <http://trac.webkit.org/changeset/185156>