Bug 145574 - REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls both select and iframe
Summary: REGRESSION: (r181879): Scrolling in select/option region in iFrame scrolls bo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 142789
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-02 15:17 PDT by Brent Fulgham
Modified: 2015-06-03 23:38 PDT (History)
2 users (show)

See Also:


Attachments
Test Case (2.45 KB, application/zip)
2015-06-02 15:18 PDT, Brent Fulgham
no flags Details
Patch (11.17 KB, patch)
2015-06-02 17:03 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (12.68 KB, patch)
2015-06-02 19:04 PDT, Brent Fulgham
simon.fraser: 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 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>