Bug 158629 - Focus event dispatched in iframe causes parent document to scroll incorrectly
Summary: Focus event dispatched in iframe causes parent document to scroll incorrectly
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: Safari 9
Hardware: iPhone / iPad Unspecified
: P2 Major
Assignee: Simon Fraser (smfr)
Keywords: InRadar
Depends on: 158867 158972
  Show dependency treegraph
Reported: 2016-06-10 12:03 PDT by Aaron Chu
Modified: 2018-06-04 08:20 PDT (History)
12 users (show)

See Also:

a test case to demonstrate the bug (1.78 KB, application/zip)
2016-06-10 12:03 PDT, Aaron Chu
no flags Details
Patch (9.99 KB, patch)
2016-06-16 16:06 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (34.89 KB, patch)
2016-06-20 14:04 PDT, Simon Fraser (smfr)
thorton: review+
Details | Formatted Diff | Diff
new test case (764 bytes, application/zip)
2018-06-04 02:14 PDT, Yann Armelin
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Chu 2016-06-10 12:03:01 PDT
Created attachment 281025 [details]
a test case to demonstrate the bug

On a very long page, when the focus event is dispatched in an iframe to focus an element within the frame, the browser scrolls the page so that the iframe becomes visible when the scrollY begins at the top of the document (0). If the scrollY begins with a value > 0, the browser incorrectly scroll pass the frame.
Comment 1 Simon Fraser (smfr) 2016-06-10 12:59:15 PDT
We do have code that tries to suppress scrolling:

    // Focusing a form element triggers animation in UIKit to scroll to the right position.
    // Calling updateFocusAppearance() would generate an unnecessary call to ScrollView::setScrollPosition(),
    // which would jump us around during this animation. See <rdar://problem/6699741>.
    FrameView* view = document().view();
    bool isFormControl = view && is<HTMLFormControlElement>(*this);
    if (isFormControl)
    updateFocusAppearance(restorePreviousSelection ? SelectionRestorationMode::Restore : SelectionRestorationMode::SetDefault);
    if (isFormControl)
Comment 2 Simon Fraser (smfr) 2016-06-10 13:05:04 PDT
Ah, I think the problem is rather obvious. view->setProhibitsScrolling() is only called on the inner FrameView, not all the ancestor ones, so the ancestor ones scroll, and with incorrect offsets.
Comment 3 James Craig 2016-06-14 21:47:55 PDT
Comment 4 Simon Fraser (smfr) 2016-06-15 07:56:24 PDT
I plan to fix this but am having some problems getting a reliable testcase for iOS.
Comment 5 Simon Fraser (smfr) 2016-06-16 16:06:42 PDT
Created attachment 281498 [details]
Comment 6 Enrica Casucci 2016-06-16 16:17:47 PDT
Comment on attachment 281498 [details]

Looks good.
Comment 7 Simon Fraser (smfr) 2016-06-16 16:34:35 PDT
Comment 8 Ryan Haddad 2016-06-16 17:15:11 PDT
This change appears to have caused test failures on iso-simulator

Regressions: Unexpected text-only failures (9)
  editing/input/caret-at-the-edge-of-input.html [ Failure ]
  fast/forms/input-text-scroll-left-on-blur.html [ Failure ]
  fast/forms/textarea-no-scroll-on-blur.html [ Failure ]
  fast/forms/textarea-scrolled-type.html [ Failure ]
  fast/forms/textfield-outline.html [ Failure ]
  fast/overflow/scroll-nested-positioned-layer-in-overflow.html [ Failure ]
  fast/overflow/scrollRevealButton.html [ Failure ]
  fast/scrolling/ios/scrollTo-at-page-load.html [ Failure ]
  fast/transforms/scrollIntoView-transformed.html [ Failure ]

Comment 9 Simon Fraser (smfr) 2016-06-16 17:53:01 PDT
Thanks, I will investigate
Comment 10 Simon Fraser (smfr) 2016-06-16 18:29:08 PDT
I think these are probably because I disabled both overflow and frame scrolling, but we should really allow overflow scrolling.
Comment 11 Alexey Proskuryakov 2016-06-16 23:17:41 PDT
We can't have 9 tests failing of so long, this should have been rolled out. Will do it now.
Comment 12 WebKit Commit Bot 2016-06-16 23:20:19 PDT
Re-opened since this is blocked by bug 158867
Comment 13 Alexey Proskuryakov 2016-06-16 23:20:40 PDT
What's particularly unfortunate is that some of these tests were already flaky.
Comment 14 Simon Fraser (smfr) 2016-06-20 14:04:47 PDT
Created attachment 281675 [details]
Comment 15 Simon Fraser (smfr) 2016-06-20 14:23:32 PDT
Comment 16 Alexey Proskuryakov 2016-06-20 20:03:31 PDT
This broke Windows build:

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebView.cpp(3975): error C2664: 'void WebCore::FrameSelection::revealSelection(WebCore::SelectionRevealMode,const WebCore::ScrollAlignment &,WebCore::RevealExtentOption)': cannot convert argument 1 from 'const WebCore::ScrollAlignment' to 'WebCore::SelectionRevealMode' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebView.cpp(3975): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

And iOS tests:

Regressions: Unexpected text-only failures (1)
  fast/overflow/scrollRevealButton.html [ Failure ]

Why was this patch landed without waiting for EWS?
Comment 17 WebKit Commit Bot 2016-06-20 20:05:57 PDT
Re-opened since this is blocked by bug 158972
Comment 18 Alexey Proskuryakov 2016-06-20 20:09:04 PDT
Rolled out in https://trac.webkit.org/r202263.
Comment 19 Simon Fraser (smfr) 2016-06-21 13:58:46 PDT
Comment 20 Yann Armelin 2018-06-04 02:11:47 PDT
This issue still occurs on Safari 11 and last Webkit version, only if the target element is a non-input HTML elements (like div or span).

It can also be reproduced when element.scrollIntoView is called.
Comment 21 Yann Armelin 2018-06-04 02:14:06 PDT
Created attachment 341898 [details]
new test case
Comment 22 Jon Lee 2018-06-04 08:02:23 PDT
(In reply to Yann Armelin from comment #21)
> Created attachment 341898 [details]
> new test case

Because of the age of this bug, it's closed. Yann, would you mind filing a new bug containing your test case? Thank you.
Comment 23 Yann Armelin 2018-06-04 08:20:25 PDT
Sure, new bug : https://bugs.webkit.org/show_bug.cgi?id=186268