Bug 171140 - [iOS] REGRESSION (r209409): getBoundingClientRect is wrong for fixed-position elements in resize/orientationchange
Summary: [iOS] REGRESSION (r209409): getBoundingClientRect is wrong for fixed-position...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-21 13:43 PDT by Tim Horton
Modified: 2017-05-07 14:23 PDT (History)
7 users (show)

See Also:


Attachments
testcase (1.39 KB, text/html)
2017-04-21 13:43 PDT, Tim Horton
no flags Details
Patch (11.77 KB, patch)
2017-04-25 14:21 PDT, Tim Horton
simon.fraser: review-
Details | Formatted Diff | Diff
Patch for EWS (25.83 KB, patch)
2017-05-05 20:06 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (31.84 KB, patch)
2017-05-06 21:24 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (31.15 KB, patch)
2017-05-07 10:49 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2017-04-21 13:43:57 PDT
Created attachment 307789 [details]
testcase

Steps to Reproduce:

1. Open attached test page in Safari on iOS 10.3+.
2. Rotate.

Expected: all three borders should line up
Actual: the borders (green and blue) that are derived from the bounding client rect of the fixed-position element inside onresize/onorientationchange don't line up with the fixed-position element's border.
Comment 1 Radar WebKit Bug Importer 2017-04-21 13:52:24 PDT
<rdar://problem/31765167>
Comment 2 Simon Fraser (smfr) 2017-04-21 14:09:16 PDT
We need a way to test rotation.
Comment 3 Tim Horton 2017-04-21 14:41:14 PDT
I don't think testing this is blocked on "a way to test rotation", I'm pretty sure this is just a resize bug. And we have ways to test that.
Comment 4 Tim Horton 2017-04-24 12:55:10 PDT
This regressed in https://trac.webkit.org/changeset/209409/webkit.
Comment 5 Tim Horton 2017-04-24 17:42:53 PDT
(And ToT is OK with Visual Viewports disabled)
Comment 6 Tim Horton 2017-04-24 20:40:26 PDT
It turns out (after hours of debugging) that the problem here is simple: WebPage::dynamicViewportSizeUpdate was not updated to set the layoutViewportOverrideRect instead of the customFixedPositionLayoutRect like WebPage::updateVisibleContentRects was.

I have tested after making this change and things are better; I'll make some tests and post a patch.
Comment 7 Tim Horton 2017-04-25 14:21:55 PDT
Created attachment 308144 [details]
Patch
Comment 8 Tim Horton 2017-04-25 14:39:06 PDT
Comment on attachment 308144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308144&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:-2992
> -    frameView.setCustomFixedPositionLayoutRect(fixedPositionLayoutRect);

We may want to try to clean this up and share more code between dynamicViewportSizeUpdate and updateVisibleContentRects, but that is a much bigger change that I don't want to make at the moment.
Comment 9 Simon Fraser (smfr) 2017-04-26 11:05:11 PDT
Comment on attachment 308144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308144&action=review

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2994
> +        frameView.setLayoutViewportOverrideRect(LayoutRect(fixedPositionLayoutRect));

This is wrong, because fixedPositionLayoutRect comes from frameView.viewportConstrainedObjectsRect(), which is for the old-style fixed behavior, not the visual viewport behavior. We need to get this rect from _page->computeCustomFixedPositionRect().
Comment 10 Simon Fraser (smfr) 2017-05-05 20:06:37 PDT
Created attachment 309262 [details]
Patch for EWS
Comment 11 Simon Fraser (smfr) 2017-05-06 21:24:02 PDT
Created attachment 309316 [details]
Patch
Comment 12 Simon Fraser (smfr) 2017-05-07 10:49:36 PDT
Created attachment 309323 [details]
Patch
Comment 13 WebKit Commit Bot 2017-05-07 14:23:21 PDT
Comment on attachment 309323 [details]
Patch

Clearing flags on attachment: 309323

Committed r216352: <http://trac.webkit.org/changeset/216352>
Comment 14 WebKit Commit Bot 2017-05-07 14:23:23 PDT
All reviewed patches have been landed.  Closing bug.