Bug 186956

Summary: AX: [iOS] VoiceOver scroll position is jumpy in frames
Product: WebKit Reporter: Nan Wang <n_wang>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: 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
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176615
Attachments:
Description Flags
patch none

Description Nan Wang 2018-06-22 18:15:11 PDT
iframe scrolling is still not right after the fix of 176615.

<rdar://problem/41294817>
Comment 1 Nan Wang 2018-06-22 18:29:11 PDT
Created attachment 343409 [details]
patch

See if all the tests would pass
Comment 2 Simon Fraser (smfr) 2018-06-25 12:01:19 PDT
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.
Comment 3 Nan Wang 2018-06-25 12:52:54 PDT
(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 4 Simon Fraser (smfr) 2018-06-26 11:42:56 PDT
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.
Comment 5 Nan Wang 2018-06-26 15:20:00 PDT
(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 6 Simon Fraser (smfr) 2018-06-29 15:44:38 PDT
Comment on attachment 343409 [details]
patch

We can land this now and deal with any fallout later.
Comment 7 WebKit Commit Bot 2018-06-29 16:33:36 PDT
Comment on attachment 343409 [details]
patch

Clearing flags on attachment: 343409

Committed r233376: <https://trac.webkit.org/changeset/233376>
Comment 8 WebKit Commit Bot 2018-06-29 16:33:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Frédéric Wang (:fredw) 2018-10-22 08:11:47 PDT
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.
Comment 10 Frédéric Wang (:fredw) 2018-10-22 09:36:16 PDT
*** Bug 176451 has been marked as a duplicate of this bug. ***
Comment 11 Frédéric Wang (:fredw) 2018-10-25 08:21:00 PDT
(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.
Comment 12 Simon Fraser (smfr) 2018-11-12 15:28:45 PST
*** Bug 164512 has been marked as a duplicate of this bug. ***
Comment 13 Simon Fraser (smfr) 2018-11-12 16:40:28 PST
*** Bug 186268 has been marked as a duplicate of this bug. ***