207425 2020-02-07 20:51:03 -0800 REGRESSION (r256072): ScrollViewScrollabilityTests.ScrollableWithOverflowHiddenAndInputView fails 2020-02-08 12:36:34 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 wenson_hsieh wenson_hsieh commit-queue simon.fraser thorton webkit-bug-importer oldest_to_newest 1616526 0 wenson_hsieh 2020-02-07 20:51:03 -0800 Followup to <rdar://problem/56960774>. 1616527 1 wenson_hsieh 2020-02-07 20:52:40 -0800 TestWebKitAPI.ScrollViewScrollabilityTests.ScrollableWithOverflowHiddenAndInputView Failed Ran 1 tests of 1 with 0 successful ------------------------------ Test suite failed Failed TestWebKitAPI.ScrollViewScrollabilityTests.ScrollableWithOverflowHiddenAndInputView LEAK: 1 WebProcessPool LEAK: 1 WebPageProxy /Users/whsieh/work/OpenSource/Tools/TestWebKitAPI/Tests/ios/ScrollViewScrollabilityTests.mm:100 Expected equality of these values: [[webView scrollView] isScrollEnabled] Which is: false __objc_yes Which is: true 1616528 2 wenson_hsieh 2020-02-07 21:05:00 -0800 The way this API test currently works is a little unexpected. I think it’s intended to test that after focusing an input field (and bringing up the keyboard), we scroll to reveal the focused element even when the body has `overflow: hidden`. The logic responsible for making this work is in -[WKWebView _updateScrollViewForTransaction:]... bool hasDockedInputView = !CGRectIsEmpty(_inputViewBounds); bool isZoomed = layerTreeTransaction.pageScaleFactor() > layerTreeTransaction.initialScaleFactor(); … bool scrollingEnabled = _page->scrollingCoordinatorProxy()->hasScrollableMainFrame() || hasDockedInputView || isZoomed || scrollingNeededToRevealUI; [_scrollView _setScrollEnabledInternal:scrollingEnabled]; However, since this is an API test on iOS (and therefore does not run in the context of a UIApplication), UIKit never attempts to actually bring up an input view; as a result, _inputViewBounds is always empty, so hasDockedInputView will always be NO. Instead, this test currently passes due to how -[WKWebView _zoomToFocusRect:…:] zooms the input field when running on iPhone; by zooming, the `isZoomed` flag is set, and so we end up setting scrollEnabled to YES anyways. I manually verified on device (by focusing the bottom half of https://whsieh.github.io/examples/multiple-contenteditable) that we do still scroll to reveal focused elements after my change in r256072, since _inputViewBounds is non-empty. 1616529 3 wenson_hsieh 2020-02-07 21:08:47 -0800 This also explains the need for this snippet in the test: BOOL isPhone = [[UIDevice currentDevice] userInterfaceIdiom] == UIUserInterfaceIdiomPhone; if (isPhone) EXPECT_EQ([[webView scrollView] isScrollEnabled], YES); Since we don’t zoom on iPad, the `isZoomed` flag is NO and we don’t end up making the scroll view scrollable. This also suggests that if we were to make TestWebKitAPI a proper UI application in the future, this workaround could be removed. 1616530 4 wenson_hsieh 2020-02-07 21:12:46 -0800 (In reply to Wenson Hsieh from comment #3) > This also explains the need for this snippet in the test: > > BOOL isPhone = [[UIDevice currentDevice] userInterfaceIdiom] == > UIUserInterfaceIdiomPhone; > if (isPhone) > EXPECT_EQ([[webView scrollView] isScrollEnabled], YES); > > Since we don’t zoom on iPad, the `isZoomed` flag is NO and we don’t end up > making the scroll view scrollable. This also suggests that if we were to > make TestWebKitAPI a proper UI application in the future, this workaround > could be removed. On closer inspection, maybe not, because the test also explicitly overrides the input view and input accessory views to be empty UIViews, which means that the _inputViewBounds would be empty anyways :/ auto inputView = adoptNS([[UIView alloc] init]); auto inputAccessoryView = adoptNS([[UIView alloc] init]); auto inputDelegate = adoptNS([TestInputDelegate new]); [inputDelegate setWillStartInputSessionHandler:[inputView, inputAccessoryView] (WKWebView *, id<_WKFormInputSession> session) { session.customInputView = inputView.get(); session.customInputAccessoryView = inputAccessoryView.get(); }]; 1616536 5 390167 wenson_hsieh 2020-02-07 21:30:36 -0800 Created attachment 390167 Patch 1616570 6 390167 wenson_hsieh 2020-02-08 11:52:28 -0800 Comment on attachment 390167 Patch Thanks for the review! 1616579 7 390167 commit-queue 2020-02-08 12:36:33 -0800 Comment on attachment 390167 Patch Clearing flags on attachment: 390167 Committed r256092: <https://trac.webkit.org/changeset/256092> 1616580 8 commit-queue 2020-02-08 12:36:34 -0800 All reviewed patches have been landed. Closing bug. 390167 2020-02-07 21:30:36 -0800 2020-02-08 12:36:33 -0800 Patch bug-207425-20200207213035.patch text/plain 3379 wenson_hsieh U3VidmVyc2lvbiBSZXZpc2lvbjogMjU2MDA4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDgyOTM2MGIwZTQ0MjVkZjc0 M2ZjYjgzMWFmNzUyMDQ5NzQ4OTYwZDYuLjc0MzQwNDgyZjYxMzA5OWFkOTcwOTUxNDFlN2QxYzQz YTg2ODcxYzMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzIgQEAKKzIwMjAtMDItMDcgIFdlbnNvbiBI c2llaCAgPHdlbnNvbl9oc2llaEBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjI1 NjA3Mik6IFNjcm9sbFZpZXdTY3JvbGxhYmlsaXR5VGVzdHMuU2Nyb2xsYWJsZVdpdGhPdmVyZmxv d0hpZGRlbkFuZElucHV0VmlldyBmYWlscworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA3NDI1CisgICAgICAgIEZvbGxvd3VwIHRvIDxyZGFyOi8vcHJv YmxlbS81Njk2MDc3ND4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBUaGUgZmFpbGluZyB0ZXN0IFNjcm9sbFZpZXdTY3JvbGxhYmlsaXR5VGVzdHMuU2Ny b2xsYWJsZVdpdGhPdmVyZmxvd0hpZGRlbkFuZElucHV0VmlldyByZWxpZXMgb24gLVtXS1dlYlZp ZXcKKyAgICAgICAgX3pvb21Ub0ZvY3VzUmVjdDouLi46XSB0byBtb2RpZnkgdGhlIHpvb20gc2Nh bGUgb2YgV0tTY3JvbGxWaWV3IGluIG9yZGVyIGZvciB0aGUgbmV4dCBsYXllciB0cmVlIHRyYW5z YWN0aW9uIHRvCisgICAgICAgIGhhdmUgYSB6b29tIHNjYWxlIG5vdCBlcXVhbCB0byB0aGUgaW5p dGlhbCBzY2FsZSwgd2hpY2ggaW4gdHVybiBlbnN1cmVzIHRoYXQgd2UgbWFrZSB0aGUgVUlTY3Jv bGxWaWV3IHNjcm9sbGFibGUKKyAgICAgICAgYWZ0ZXIgdGhlIG5leHQgcmVtb3RlIGxheWVyIHRy ZWUgdXBkYXRlLgorCisgICAgICAgIFRoZSBjaGFuZ2UgaW4gcjI1NjA3MiBjYXVzZWQgdXMgdG8g YmFpbCBlYXJseSBpbiAtX3pvb21Ub0ZvY3VzUmVjdDogYW5kIC1fem9vbVRvUG9pbnQ6IGluIHRo ZSBjYXNlIHdoZXJlIHRoZQorICAgICAgICBzY3JvbGwgdmlldyBpcyBub3Qgc2Nyb2xsYWJsZSAo aS5lLiB0aGUgcGFnZSBoYXMgYG92ZXJmbG93OiBoaWRkZW5gKTsgdGhpcyBtZWFucyB0aGF0IGlm IGFsbCB0aGUgZm9sbG93aW5nCisgICAgICAgIGNvbmRpdGlvbnMgYXJlIG1ldCwgd2Ugbm93IGF2 b2lkIHpvb21pbmcgaW50byBmb2N1c2VkIGVsZW1lbnRzLCB3aGVyZWFzIHdlIHdvdWxkJ3ZlIHpv b21lZCBiZWZvcmUgdGhlIGNoYW5nZToKKworICAgICAgICAxLiAgVGhlIHVzZXIgaXMgZm9jdXNp bmcgYW4gZWxlbWVudCBvbiBhbiBpUGhvbmUuCisgICAgICAgIDIuICBUaGUgZG9jdW1lbnQgYm9k eSBoYXMgYG92ZXJmbG93OiBoaWRkZW47YCwgb3IgdGhlIFdlYktpdCBjbGllbnQgaGFzIGNhbGxl ZCBgLXNldFNjcm9sbEVuYWJsZWQ6Tk9gLgorICAgICAgICAzLiAgRWl0aGVyIGFuIFNQSSBjbGll bnQgaGFzIHZlbmRlZCBhbiBlbXB0eSBpbnB1dCB2aWV3IGFuZCBpbnB1dCBhY2Nlc3Nvcnkgdmll dywgb3IgdGhlIGZvY3VzZWQgZWxlbWVudCBoYXMKKyAgICAgICAgICAgIGlucHV0bW9kZT0ibm9u ZSIuCisKKyAgICAgICAgVG8gZml4IHRoZSB0ZXN0IGFuZCByZXN0b3JlIHpvb21pbmcgYmVoYXZp b3IsIHdlIHBhcnRpYWxseSByZXZlcnQgdGhlIGNoYW5nZSBpbiByMjU2MDcyLCBzdWNoIHRoYXQg d2Ugbm8gbG9uZ2VyCisgICAgICAgIGJhaWwgZnJvbSBXS1dlYlZpZXcncyB6b29taW5nIGNvZGVw YXRocyBpZiB0aGUgc2Nyb2xsRW5hYmxlZCBwcm9wZXJ0eSBpcyBOTy4KKworICAgICAgICAqIFVJ UHJvY2Vzcy9BUEkvaW9zL1dLV2ViVmlld0lPUy5tbToKKyAgICAgICAgKC1bV0tXZWJWaWV3IF96 b29tVG9Qb2ludDphdFNjYWxlOmFuaW1hdGVkOl0pOgorICAgICAgICAoLVtXS1dlYlZpZXcgX3pv b21Ub0ZvY3VzUmVjdDpzZWxlY3Rpb25SZWN0Omluc2lkZUZpeGVkOmZvbnRTaXplOm1pbmltdW1T Y2FsZTptYXhpbXVtU2NhbGU6YWxsb3dTY2FsaW5nOmZvcmNlU2Nyb2xsOl0pOgorCiAyMDIwLTAy LTA3ICBXZW5zb24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgogCiAgICAgICAgIFtp T1NdIERvdWJsZSB0YXBwaW5nIHNob3VsZG4ndCBzY3JvbGwgdGhlIHBhZ2Ugd2hlbiB0aGUgYm9k eSBoYXMgYG92ZXJmbG93OiBoaWRkZW5gCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJv Y2Vzcy9BUEkvaW9zL1dLV2ViVmlld0lPUy5tbSBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQ SS9pb3MvV0tXZWJWaWV3SU9TLm1tCmluZGV4IGE0OWVlMTkwMGI0M2YyMjJiYzMzZDQ5YTNlOTcz ODc5YmNhMjljMDkuLmIzZDc5MWJlZWQwZThlZTZmMDAzOWVhOTUxMTQ1NmM5MDU1YmJiZGMgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9pb3MvV0tXZWJWaWV3SU9TLm1t CisrKyBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9pb3MvV0tXZWJWaWV3SU9TLm1tCkBA IC0xMDQyLDkgKzEwNDIsNiBAQCAtIChSZWZQdHI8V2ViS2l0OjpWaWV3U25hcHNob3Q+KV90YWtl Vmlld1NuYXBzaG90CiAKIC0gKHZvaWQpX3pvb21Ub1BvaW50OihXZWJDb3JlOjpGbG9hdFBvaW50 KXBvaW50IGF0U2NhbGU6KGRvdWJsZSlzY2FsZSBhbmltYXRlZDooQk9PTClhbmltYXRlZAogewot ICAgIGlmICghW19zY3JvbGxWaWV3IGlzU2Nyb2xsRW5hYmxlZF0pCi0gICAgICAgIHJldHVybjsK LQogICAgIENGVGltZUludGVydmFsIGR1cmF0aW9uID0gMDsKICAgICBDR0Zsb2F0IHpvb21TY2Fs ZSA9IGNvbnRlbnRab29tU2NhbGUoc2VsZik7CiAKQEAgLTExODcsOCArMTE4NCw2IEBAIC0gKHZv aWQpX3pvb21Ub0ZvY3VzUmVjdDooY29uc3QgV2ViQ29yZTo6RmxvYXRSZWN0Jilmb2N1c2VkRWxl bWVudFJlY3RJbkRvY3VtZW50CiB7CiAgICAgTE9HX1dJVEhfU1RSRUFNKFZpc2libGVSZWN0cywg c3RyZWFtIDw8ICJfem9vbVRvRm9jdXNSZWN0OiIgPDwgZm9jdXNlZEVsZW1lbnRSZWN0SW5Eb2N1 bWVudENvb3JkaW5hdGVzIDw8ICIgc2VsZWN0aW9uUmVjdDoiIDw8IHNlbGVjdGlvblJlY3RJbkRv Y3VtZW50Q29vcmRpbmF0ZXMpOwogICAgIFVOVVNFRF9QQVJBTShpbnNpZGVGaXhlZCk7Ci0gICAg aWYgKCFbX3Njcm9sbFZpZXcgaXNTY3JvbGxFbmFibGVkXSkKLSAgICAgICAgcmV0dXJuOwogCiAg ICAgY29uc3QgZG91YmxlIG1pbmltdW1IZWlnaHRUb1Nob3dDb250ZW50QWJvdmVLZXlib2FyZCA9 IDEwNjsKICAgICBjb25zdCBDRlRpbWVJbnRlcnZhbCBmb3JtQ29udHJvbFpvb21BbmltYXRpb25E dXJhdGlvbiA9IDAuMjU7Cg==