Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REGRESSION (Safari 16): Dynamic viewport units (dvh) not matching viewport height #2765

Merged
merged 1 commit into from Jul 27, 2022

Conversation

dcrousso
Copy link
Member

@dcrousso dcrousso commented Jul 27, 2022

cd186d7

REGRESSION (Safari 16): Dynamic viewport units (dvh) not matching viewport height
https://bugs.webkit.org/show_bug.cgi?id=242758
<rdar://problem/97031866>

Reviewed by Simon Fraser.

This is likely a regression from two different commits.

248939@main started *always* setting an override for `sv*`/`lv*` based on `-[WKWebView setFrame:]`
whereas before they'd fall back to `FrameView::viewportConstrainedVisibleContentRect`, which takes
into account the `topContentInset`.

251232@main switched from using `RenderView::size` for `dvh` to using `FrameView::size` instead,
which does not take into account the `topContentInset`.

* Source/WebCore/page/FrameView.cpp:
(WebCore::FrameView::sizeForCSSDynamicViewportUnits const):
Use a different method that takes into account the `topContentInset`. This also matches the fallback
behavior for `sv*`/`lv*`.

* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _recalculateViewportSizesWithMinimumViewportInset:maximumViewportInset:throwOnInvalidInput:]):
Include the `_topContentInset` when calculating the viewport sizes on macOS.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::setTopContentInset):
* Source/WebKit/UIProcess/PageClient.h:
(WebKit::PageClient::topContentInsetDidChange): Added.
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::topContentInsetDidChange): Added.
Recalculate the viewport sizes whenever the `_topContentInset` changes (including `_automaticallyAdjustsContentInsets`).

* Tools/TestWebKitAPI/Tests/WebKitCocoa/CSSViewportUnits.mm:
(TEST.CSSViewportUnits.TopContentInset): Added.
(TEST.CSSViewportUnits.MinimumViewportInsetWithTopContentInset): Added.
(TEST.CSSViewportUnits.MaximumViewportInsetWithTopContentInset): Added.

Canonical link: https://commits.webkit.org/252878@main

@dcrousso dcrousso requested a review from cdumez as a code owner July 27, 2022 01:33
@dcrousso dcrousso self-assigned this Jul 27, 2022
@dcrousso dcrousso added CSS Cascading Style Sheets implementation Safari Technology Preview labels Jul 27, 2022
@dcrousso dcrousso added the merge-queue Applied to send a pull request to merge-queue label Jul 27, 2022
…wport height

https://bugs.webkit.org/show_bug.cgi?id=242758
<rdar://problem/97031866>

Reviewed by Simon Fraser.

This is likely a regression from two different commits.

248939@main started *always* setting an override for `sv*`/`lv*` based on `-[WKWebView setFrame:]`
whereas before they'd fall back to `FrameView::viewportConstrainedVisibleContentRect`, which takes
into account the `topContentInset`.

251232@main switched from using `RenderView::size` for `dvh` to using `FrameView::size` instead,
which does not take into account the `topContentInset`.

* Source/WebCore/page/FrameView.cpp:
(WebCore::FrameView::sizeForCSSDynamicViewportUnits const):
Use a different method that takes into account the `topContentInset`. This also matches the fallback
behavior for `sv*`/`lv*`.

* Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _recalculateViewportSizesWithMinimumViewportInset:maximumViewportInset:throwOnInvalidInput:]):
Include the `_topContentInset` when calculating the viewport sizes on macOS.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::setTopContentInset):
* Source/WebKit/UIProcess/PageClient.h:
(WebKit::PageClient::topContentInsetDidChange): Added.
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.h:
* Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:
(WebKit::PageClientImplCocoa::topContentInsetDidChange): Added.
Recalculate the viewport sizes whenever the `_topContentInset` changes (including `_automaticallyAdjustsContentInsets`).

* Tools/TestWebKitAPI/Tests/WebKitCocoa/CSSViewportUnits.mm:
(TEST.CSSViewportUnits.TopContentInset): Added.
(TEST.CSSViewportUnits.MinimumViewportInsetWithTopContentInset): Added.
(TEST.CSSViewportUnits.MaximumViewportInsetWithTopContentInset): Added.

Canonical link: https://commits.webkit.org/252878@main
@webkit-commit-queue
Copy link
Collaborator

Committed 252878@main (cd186d7): https://commits.webkit.org/252878@main

Reviewed commits have been landed. Closing PR #2765 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit cd186d7 into WebKit:main Jul 27, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 27, 2022
@dcrousso dcrousso deleted the eng/242758 branch July 27, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
4 participants