Bug 186956

Created attachment 343409 [details] patch See if all the tests would pass

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.

(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()?

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.

(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.

Comment on attachment 343409 [details] patch We can land this now and deal with any fallout later.

Comment on attachment 343409 [details] patch Clearing flags on attachment: 343409 Committed r233376: <https://trac.webkit.org/changeset/233376>

All reviewed patches have been landed. Closing bug.

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.

*** Bug 176451 has been marked as a duplicate of this bug. ***

(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.

*** Bug 164512 has been marked as a duplicate of this bug. ***

*** Bug 186268 has been marked as a duplicate of this bug. ***