186956 2018-06-22 18:15:11 -0700 AX: [iOS] VoiceOver scroll position is jumpy in frames 2018-12-03 01:19:32 -0800 1 1 1 Unclassified WebKit Accessibility WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=176615 InRadar P2 Normal --- 1 n_wang webkit-unassigned aboxhall apinheiro barry cfleizach commit-queue coridyn+bugzilla dmazzoni ews-watchlist fred.wang jcraig jdiggs n_wang rniwa samuel_white simon.fraser webkit-bug-importer yann.armelin oldest_to_newest 1435731 0 n_wang 2018-06-22 18:15:11 -0700 iframe scrolling is still not right after the fix of 176615. <rdar://problem/41294817> 1435744 1 343409 n_wang 2018-06-22 18:29:11 -0700 Created attachment 343409 patch See if all the tests would pass 1436170 2 343409 simon.fraser 2018-06-25 12:01:19 -0700 Comment on attachment 343409 patch View in context: https://bugs.webkit.org/attachment.cgi?id=343409&action=review > Source/WebCore/platform/ScrollView.cpp:851 > + if (delegatesScrolling()) > + return convertToContainingView(contentsToView(rect)); This looks like it's duplicating what Frederic was trying to do in bug 182785. You need to be careful here; this change can affect both UIWebView and WKWebView on iOS, and you need to examine the callers to make sure you didn't break anything. Also bear in mind that we don't have good UIWebViewtest coverage. 1436189 3 n_wang 2018-06-25 12:52:54 -0700 (In reply to Simon Fraser (smfr) from comment #2) > Comment on attachment 343409 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343409&action=review > > > Source/WebCore/platform/ScrollView.cpp:851 > > + if (delegatesScrolling()) > > + return convertToContainingView(contentsToView(rect)); > > This looks like it's duplicating what Frederic was trying to do in bug > 182785. You need to be careful here; this change can affect both UIWebView > and WKWebView on iOS, and you need to examine the callers to make sure you > didn't break anything. Also bear in mind that we don't have good > UIWebViewtest coverage. I think that's a different issue since it's checking the window? contentsToContainingViewContents() is only being called in RenderLayer::scrollRectToVisible() Do you mean I should check all the callers of RenderLayer::scrollRectToVisible()? 1436605 4 343409 simon.fraser 2018-06-26 11:42:56 -0700 Comment on attachment 343409 patch View in context: https://bugs.webkit.org/attachment.cgi?id=343409&action=review >>> Source/WebCore/platform/ScrollView.cpp:851 >>> + return convertToContainingView(contentsToView(rect)); >> >> This looks like it's duplicating what Frederic was trying to do in bug 182785. You need to be careful here; this change can affect both UIWebView and WKWebView on iOS, and you need to examine the callers to make sure you didn't break anything. Also bear in mind that we don't have good UIWebViewtest coverage. > > I think that's a different issue since it's checking the window? > contentsToContainingViewContents() is only being called in RenderLayer::scrollRectToVisible() > > Do you mean I should check all the callers of RenderLayer::scrollRectToVisible()? Actually I think you're papering over the issue that bug 182785 is trying to fix. We always want to take scroll offsets into account when converting rects (and, anyway, with frame flattening, iframe scroll offsets should always be zero). But ScrollView::windowToContents() breaks that assumption right now, and we're trying to fix it but ScrollView::windowToContents() has lots of callers and fixing it is hard. 1436686 5 n_wang 2018-06-26 15:20:00 -0700 (In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 343409 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343409&action=review > > >>> Source/WebCore/platform/ScrollView.cpp:851 > >>> + return convertToContainingView(contentsToView(rect)); > >> > >> This looks like it's duplicating what Frederic was trying to do in bug 182785. You need to be careful here; this change can affect both UIWebView and WKWebView on iOS, and you need to examine the callers to make sure you didn't break anything. Also bear in mind that we don't have good UIWebViewtest coverage. > > > > I think that's a different issue since it's checking the window? > > contentsToContainingViewContents() is only being called in RenderLayer::scrollRectToVisible() > > > > Do you mean I should check all the callers of RenderLayer::scrollRectToVisible()? > > Actually I think you're papering over the issue that bug 182785 is trying to > fix. We always want to take scroll offsets into account when converting > rects (and, anyway, with frame flattening, iframe scroll offsets should > always be zero). But ScrollView::windowToContents() breaks that assumption > right now, and we're trying to fix it but ScrollView::windowToContents() has > lots of callers and fixing it is hard. Ok I see your concern regarding ScrollView::windowToContents(). But here we are only dealing with contentsToContainingViewContents() and RenderLayer::scrollRectToVisible() is the only caller. Do you think the fix is safe enough just for this particular case? Are you saying that we should replace the delegatesScrolling() check to be something else that covers more cases? I'm not sure if I have enough knowledge about the scrolling concept. Can you point me to what other cases we were missing? I don't think I have enough context just from this particular bug. 1437935 6 343409 simon.fraser 2018-06-29 15:44:38 -0700 Comment on attachment 343409 patch We can land this now and deal with any fallout later. 1437977 7 343409 commit-queue 2018-06-29 16:33:36 -0700 Comment on attachment 343409 patch Clearing flags on attachment: 343409 Committed r233376: <https://trac.webkit.org/changeset/233376> 1437978 8 commit-queue 2018-06-29 16:33:38 -0700 All reviewed patches have been landed. Closing bug. 1471216 9 fred.wang 2018-10-22 08:11:47 -0700 OK, I was wondering why my patch for bug 176451 did not make sense at all now and just realized about that change... Is there any reason you have to be that strict in this patch? In bug 176451, I tried to do very minimal change and I believe it is enough to make your new test pass. 1471242 10 fred.wang 2018-10-22 09:36:16 -0700 *** Bug 176451 has been marked as a duplicate of this bug. *** 1472062 11 fred.wang 2018-10-25 08:21:00 -0700 (In reply to Frédéric Wang (:fredw) from comment #9) > OK, I was wondering why my patch for bug 176451 did not make sense at all > now and just realized about that change... Is there any reason you have to > be that strict in this patch? In bug 176451, I tried to do very minimal > change and I believe it is enough to make your new test pass. The patch I uploaded on bug 176451 was safer as it only changes the behavior on WK2 and only for subframes. However, I have not been able to find a repro case where r233376 demonstrating things are worse on WK1. If people are interested I can still rebase my patch from bug 176451 to replace this fix. 1477761 12 simon.fraser 2018-11-12 15:28:45 -0800 *** Bug 164512 has been marked as a duplicate of this bug. *** 1477798 13 simon.fraser 2018-11-12 16:40:28 -0800 *** Bug 186268 has been marked as a duplicate of this bug. *** 343409 2018-06-22 18:29:11 -0700 2018-06-29 16:33:36 -0700 patch patch.txt text/plain 5204 n_wang SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzMzExNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBACisyMDE4LTA2LTIyICBOYW4gV2Fu ZyAgPG5fd2FuZ0BhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IFtpT1NdIFZvaWNlT3ZlciBzY3Jv bGwgcG9zaXRpb24gaXMganVtcHkgaW4gZnJhbWVzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODY5NTYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBpT1MgaXMgdXNpbmcgZGVsZWdhdGUgc2Nyb2xsaW5nIGFu ZCB3ZSBzaG91bGQgbm90IHRha2UgaW50byBhY2NvdW50CisgICAgICAgIHRoZSBzY3JvbGwgb2Zm c2V0IHdoZW4gY29udmVydGluZyByZWN0cy4KKworICAgICAgICBBbHNvIGZpeGVkIGEgaXNzdWUg d2hlcmUgd2Ugd2FudCB0byBzY3JvbGwgdGhlIGVsZW1lbnQgaW50byB2aWV3IGV2ZW4KKyAgICAg ICAgaWYgaXQncyBwYXJ0aWFsbHkgdmlzaWJsZS4KKworICAgICAgICBUZXN0OiBmYXN0L3Njcm9s bGluZy9pb3MvaWZyYW1lLXNjcm9sbC1pbnRvLXZpZXcuaHRtbAorCisgICAgICAgICogYWNjZXNz aWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2JqZWN0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkFjY2Vz c2liaWxpdHlPYmplY3Q6OnNjcm9sbFRvTWFrZVZpc2libGUgY29uc3QpOgorICAgICAgICAqIHBs YXRmb3JtL1Njcm9sbFZpZXcuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsVmlldzo6Y29u dGVudHNUb0NvbnRhaW5pbmdWaWV3Q29udGVudHMgY29uc3QpOgorCiAyMDE4LTA2LTIyICBUaW0g SG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgogCiAgICAgICAgIE1ha2UgaXQgcG9z c2libGUgdG8gYWRkIGEgYm9yZGVyIGFyb3VuZCBsb2FkaW5nIG9yIGZhaWxlZC10by1sb2FkIGlt YWdlcwpJbmRleDogU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9BY2Nlc3NpYmlsaXR5T2Jq ZWN0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L0FjY2Vz c2liaWxpdHlPYmplY3QuY3BwCShyZXZpc2lvbiAyMzMxMDkpCisrKyBTb3VyY2UvV2ViQ29yZS9h Y2Nlc3NpYmlsaXR5L0FjY2Vzc2liaWxpdHlPYmplY3QuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0y OTkyLDcgKzI5OTIsNyBAQCB2b2lkIEFjY2Vzc2liaWxpdHlPYmplY3Q6OnNjcm9sbFRvTWFrZVZp CiAgICAgICAgIHBhcmVudE9iamVjdCgpLT5zY3JvbGxUb01ha2VWaXNpYmxlKCk7CiAKICAgICBp ZiAoYXV0byogcmVuZGVyZXIgPSB0aGlzLT5yZW5kZXJlcigpKQotICAgICAgICByZW5kZXJlci0+ c2Nyb2xsUmVjdFRvVmlzaWJsZShTZWxlY3Rpb25SZXZlYWxNb2RlOjpSZXZlYWwsIGJvdW5kaW5n Qm94UmVjdCgpLCBmYWxzZSwgU2Nyb2xsQWxpZ25tZW50OjphbGlnbkNlbnRlcklmTm90VmlzaWJs ZSwgU2Nyb2xsQWxpZ25tZW50OjphbGlnbkNlbnRlcklmTm90VmlzaWJsZSwgU2hvdWxkQWxsb3dD cm9zc09yaWdpblNjcm9sbGluZzo6WWVzKTsKKyAgICAgICAgcmVuZGVyZXItPnNjcm9sbFJlY3RU b1Zpc2libGUoU2VsZWN0aW9uUmV2ZWFsTW9kZTo6UmV2ZWFsLCBib3VuZGluZ0JveFJlY3QoKSwg ZmFsc2UsIFNjcm9sbEFsaWdubWVudDo6YWxpZ25DZW50ZXJJZk5lZWRlZCwgU2Nyb2xsQWxpZ25t ZW50OjphbGlnbkNlbnRlcklmTmVlZGVkLCBTaG91bGRBbGxvd0Nyb3NzT3JpZ2luU2Nyb2xsaW5n OjpZZXMpOwogfQogCiB2b2lkIEFjY2Vzc2liaWxpdHlPYmplY3Q6OnNjcm9sbFRvTWFrZVZpc2li bGVXaXRoU3ViRm9jdXMoY29uc3QgSW50UmVjdCYgc3ViZm9jdXMpIGNvbnN0CkluZGV4OiBTb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcAkocmV2aXNpb24gMjMzMTA5KQorKysgU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTg0 Nyw2ICs4NDcsOSBAQCBJbnRQb2ludCBTY3JvbGxWaWV3Ojpjb250ZW50c1RvQ29udGFpbmluCiAK IEludFJlY3QgU2Nyb2xsVmlldzo6Y29udGVudHNUb0NvbnRhaW5pbmdWaWV3Q29udGVudHMoSW50 UmVjdCByZWN0KSBjb25zdAogeworICAgIGlmIChkZWxlZ2F0ZXNTY3JvbGxpbmcoKSkKKyAgICAg ICAgcmV0dXJuIGNvbnZlcnRUb0NvbnRhaW5pbmdWaWV3KGNvbnRlbnRzVG9WaWV3KHJlY3QpKTsK KyAgICAKICAgICBpZiAoY29uc3QgU2Nyb2xsVmlldyogcGFyZW50U2Nyb2xsVmlldyA9IHBhcmVu dCgpKSB7CiAgICAgICAgIEludFJlY3QgcmVjdEluQ29udGFpbmluZ1ZpZXcgPSBjb252ZXJ0VG9D b250YWluaW5nVmlldyhjb250ZW50c1RvVmlldyhyZWN0KSk7CiAgICAgICAgIHJldHVybiBwYXJl bnRTY3JvbGxWaWV3LT52aWV3VG9Db250ZW50cyhyZWN0SW5Db250YWluaW5nVmlldyk7CkluZGV4 OiBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvQ2hhbmdl TG9nCShyZXZpc2lvbiAyMzMxMDkpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtpbmcg Y29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAxOC0wNi0yMiAgTmFuIFdhbmcgIDxuX3dhbmdAYXBw bGUuY29tPgorCisgICAgICAgIEFYOiBbaU9TXSBWb2ljZU92ZXIgc2Nyb2xsIHBvc2l0aW9uIGlz IGp1bXB5IGluIGZyYW1lcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTg2OTU2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBmYXN0L3Njcm9sbGluZy9pb3MvaWZyYW1lLXNjcm9sbC1pbnRvLXZpZXctZXhw ZWN0ZWQuaHRtbDogQWRkZWQuCisgICAgICAgICogZmFzdC9zY3JvbGxpbmcvaW9zL2lmcmFtZS1z Y3JvbGwtaW50by12aWV3Lmh0bWw6IEFkZGVkLgorCiAyMDE4LTA2LTIyICBSb3NzIEtpcnNsaW5n ICA8cm9zcy5raXJzbGluZ0Bzb255LmNvbT4KIAogICAgICAgICBbV2luQ2Fpcm9dIFVucmV2aWV3 ZWQgdGVzdCBnYXJkZW5pbmcuCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L3Njcm9sbGluZy9pb3Mv aWZyYW1lLXNjcm9sbC1pbnRvLXZpZXctZXhwZWN0ZWQuaHRtbAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlv dXRUZXN0cy9mYXN0L3Njcm9sbGluZy9pb3MvaWZyYW1lLXNjcm9sbC1pbnRvLXZpZXctZXhwZWN0 ZWQuaHRtbAkobm9uZXhpc3RlbnQpCisrKyBMYXlvdXRUZXN0cy9mYXN0L3Njcm9sbGluZy9pb3Mv aWZyYW1lLXNjcm9sbC1pbnRvLXZpZXctZXhwZWN0ZWQuaHRtbAkod29ya2luZyBjb3B5KQpAQCAt MCwwICsxLDE5IEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGhlYWQ+Cis8bWV0YSBuYW1l PSJ2aWV3cG9ydCIgY29udGVudD0id2lkdGg9ZGV2aWNlLXdpZHRoLCBpbml0aWFsLXNjYWxlPTEi PgorPHN0eWxlPgorYm9keSB7CisJbWFyZ2luOiAwOworfQorLmZyYW1lIHsKKyAgICBoZWlnaHQ6 IDIwMHB4OworICAgIHdpZHRoOiAyMDBweDsKK30KKzwvc3R5bGU+Cis8L2hlYWQ+CisKKyA8Ym9k eT4KKzxkaXYgY2xhc3M9ImZyYW1lIj5UaGlzIHRlc3RzIHRoYXQgdGhlIGlmcmFtZSBzaG91bGQg YmUgc2Nyb2xsIHRvIHRoZSB0b3Agb2YgdGhlIHBhZ2Ugb24gaU9TLjwvZGl2PgorPC9ib2R5Pgor PC9odG1sPgpcIE5vIG5ld2xpbmUgYXQgZW5kIG9mIGZpbGUKSW5kZXg6IExheW91dFRlc3RzL2Zh c3Qvc2Nyb2xsaW5nL2lvcy9pZnJhbWUtc2Nyb2xsLWludG8tdmlldy5odG1sCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIExheW91dFRlc3RzL2Zhc3Qvc2Nyb2xsaW5nL2lvcy9pZnJhbWUtc2Nyb2xsLWludG8tdmll dy5odG1sCShub25leGlzdGVudCkKKysrIExheW91dFRlc3RzL2Zhc3Qvc2Nyb2xsaW5nL2lvcy9p ZnJhbWUtc2Nyb2xsLWludG8tdmlldy5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC0wLDAgKzEsMzQg QEAKKzwhRE9DVFlQRSBodG1sPgorPGh0bWw+Cis8aGVhZD4KKzxtZXRhIG5hbWU9InZpZXdwb3J0 IiBjb250ZW50PSJ3aWR0aD1kZXZpY2Utd2lkdGgsIGluaXRpYWwtc2NhbGU9MSI+Cis8c3R5bGU+ Citib2R5IHsKKyAgICBtYXJnaW46IDA7Cit9CitpZnJhbWUgeworICAgIGJvcmRlcjogMDsKKyAg ICBoZWlnaHQ6IDIwMHB4OworICAgIHdpZHRoOiAyMDBweDsKK30KKzwvc3R5bGU+Cis8L2hlYWQ+ CisKKyA8Ym9keT4KKzxkaXYgc3R5bGU9ImhlaWdodDogMjAwMHB4OyI+PC9kaXY+Cis8aWZyYW1l IHNyY2RvYz0iPGJvZHkgc3R5bGU9J21hcmdpbjogMDsnPjxkaXY+VGhpcyB0ZXN0cyB0aGF0IHRo ZSBpZnJhbWUgc2hvdWxkIGJlIHNjcm9sbCB0byB0aGUgdG9wIG9mIHRoZSBwYWdlIG9uIGlPUy48 L2Rpdj48L2JvZHk+IiBvbmxvYWQ9InJ1blRlc3QoKSI+PC9pZnJhbWU+Cis8ZGl2IHN0eWxlPSJo ZWlnaHQ6IDIwMDBweDsiPjwvZGl2PgorPC9ib2R5PgorCis8c2NyaXB0PgorICAgIGlmICh3aW5k b3cudGVzdFJ1bm5lcikKKyAgICAgICAgdGVzdFJ1bm5lci53YWl0VW50aWxEb25lKCk7CisKKyAg ICBmdW5jdGlvbiBydW5UZXN0KCkgeworICAgICAgICB3aW5kb3cuc2Nyb2xsVG8oMCwgMjAwKTsK KyAgICAgICAgd2luZG93LmZyYW1lc1swXS5kb2N1bWVudC5ib2R5LnNjcm9sbEludG9WaWV3KCk7 CisgICAgICAgIGlmICh3aW5kb3cudGVzdFJ1bm5lcikKKyAgICAgICAgICAgIHRlc3RSdW5uZXIu bm90aWZ5RG9uZSgpOworICAgIH0KKyAgICAgPC9zY3JpcHQ+Cis8L2h0bWw+ClwgTm8gbmV3bGlu ZSBhdCBlbmQgb2YgZmlsZQo=