Attachment 356613 Details for Bug 192377 – Address review comments.

[patch] Address review comments.

ignore_minimum_effective_device_width.patch (text/plain), 18.99 KB, created by Yongjun Zhang on 2018-12-05 09:59:08 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yongjun Zhang

Created: 2018-12-05 09:59:08 PST

Size: 18.99 KB

patch

obsolete

>commit 70d81f7d6db8c2b39108f2d3e1e1c3f90edad1f2
>Author: Yongjun Zhang <yongjun_zhang@apple.com>
>Date:   Tue Dec 4 14:03:14 2018 -0800
>
>    We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
>    https://bugs.webkit.org/show_bug.cgi?id=192377
>    <rdar://problem/46364206>
>
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 191953785cd..99b504313b7 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-12-04  Yongjun Zhang  <yongjun_zhang@apple.com>
>+
>+        We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
>+        https://bugs.webkit.org/show_bug.cgi?id=192377
>+        <rdar://problem/46364206>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width-expected.txt: Added.
>+        * fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width.html: Added.
>+        * fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta-expected.txt: Added.
>+        * fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta.html: Added.
>+
> 2018-12-04  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r238838.
>diff --git a/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width-expected.txt b/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width-expected.txt
>new file mode 100644
>index 00000000000..823735f465d
>--- /dev/null
>+++ b/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width-expected.txt
>@@ -0,0 +1,41 @@
>+setMinimumEffectiveWidth(640.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(768.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(834.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(980.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(1024.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(1112.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(1280.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+setMinimumEffectiveWidth(1336.00)
>+window size: [320, 548]
>+square size: [32, 55]
>+zoom scale: 1.00
>+
>+
>diff --git a/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width.html b/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width.html
>new file mode 100644
>index 00000000000..8022d985510
>--- /dev/null
>+++ b/LayoutTests/fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width.html
>@@ -0,0 +1,58 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+    <meta name="viewport" content="width=device-width">
>+    <style>
>+        #square {
>+            position: absolute;
>+            width: 10vw;
>+            height: 10vh;
>+            border: 2px solid black;
>+        }
>+
>+        #output {
>+            width: 100%;
>+            height: 100%;
>+            overflow: scroll;
>+        }
>+
>+        body {
>+            margin: 0;
>+            width: 100%;
>+            height: 100%;
>+        }
>+    </style>
>+    <script src="../../../resources/ui-helper.js"></script>
>+    <script>
>+    if (window.testRunner) {
>+        testRunner.dumpAsText();
>+        testRunner.waitUntilDone();
>+    }
>+
>+    async function runTest() {
>+        const appendOutput = message => {
>+            output.appendChild(document.createTextNode(message));
>+            output.appendChild(document.createElement("br"));
>+        };
>+
>+        for (let targetMinimumEffectiveWidth of [640, 768, 834, 980, 1024, 1112, 1280, 1336]) {
>+            appendOutput(`setMinimumEffectiveWidth(${targetMinimumEffectiveWidth.toFixed(2)})`);
>+
>+            await UIHelper.setMinimumEffectiveWidth(targetMinimumEffectiveWidth);
>+            await Promise.all([UIHelper.ensureVisibleContentRectUpdate(), UIHelper.ensurePresentationUpdate()]);
>+            appendOutput(`window size: [${innerWidth}, ${innerHeight}]`);
>+            appendOutput(`square size: [${square.clientWidth}, ${square.clientHeight}]`);
>+            appendOutput(`zoom scale: ${(await UIHelper.zoomScale()).toFixed(2)}`);
>+            appendOutput("");
>+        }
>+
>+        testRunner.notifyDone();
>+    }
>+    </script>
>+</head>
>+
>+<body onload="runTest()">
>+    <div id="square"></div>
>+    <pre id="output"></pre>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta-expected.txt b/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta-expected.txt
>new file mode 100644
>index 00000000000..06d910e1050
>--- /dev/null
>+++ b/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta-expected.txt
>@@ -0,0 +1,41 @@
>+setMinimumEffectiveWidth(640.00)
>+window size: [640, 1096]
>+square size: [64, 110]
>+zoom scale: 0.50
>+
>+setMinimumEffectiveWidth(768.00)
>+window size: [768, 1315]
>+square size: [77, 132]
>+zoom scale: 0.42
>+
>+setMinimumEffectiveWidth(834.00)
>+window size: [834, 1428]
>+square size: [83, 143]
>+zoom scale: 0.38
>+
>+setMinimumEffectiveWidth(980.00)
>+window size: [980, 1678]
>+square size: [98, 168]
>+zoom scale: 0.33
>+
>+setMinimumEffectiveWidth(1024.00)
>+window size: [1024, 1754]
>+square size: [102, 175]
>+zoom scale: 0.31
>+
>+setMinimumEffectiveWidth(1112.00)
>+window size: [1112, 1904]
>+square size: [111, 190]
>+zoom scale: 0.29
>+
>+setMinimumEffectiveWidth(1280.00)
>+window size: [1280, 2192]
>+square size: [128, 219]
>+zoom scale: 0.25
>+
>+setMinimumEffectiveWidth(1336.00)
>+window size: [1336, 2288]
>+square size: [134, 229]
>+zoom scale: 0.24
>+
>+
>diff --git a/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta.html b/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta.html
>new file mode 100644
>index 00000000000..f2043d4d3ce
>--- /dev/null
>+++ b/LayoutTests/fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta.html
>@@ -0,0 +1,57 @@
>+<!DOCTYPE html> 
>+<html>
>+<head>
>+    <style>
>+        #square {
>+            position: absolute;
>+            width: 10vw;
>+            height: 10vh;
>+            border: 2px solid black;
>+        }
>+
>+        #output {
>+            width: 100%;
>+            height: 100%;
>+            overflow: scroll;
>+        }
>+
>+        body {
>+            margin: 0;
>+            width: 100%;
>+            height: 100%;
>+        }
>+    </style>
>+    <script src="../../../resources/ui-helper.js"></script>
>+    <script>
>+    if (window.testRunner) {
>+        testRunner.dumpAsText();
>+        testRunner.waitUntilDone();
>+    }
>+
>+    async function runTest() {
>+        const appendOutput = message => {
>+            output.appendChild(document.createTextNode(message));
>+            output.appendChild(document.createElement("br"));
>+        };
>+
>+        for (let targetMinimumEffectiveWidth of [640, 768, 834, 980, 1024, 1112, 1280, 1336]) {
>+            appendOutput(`setMinimumEffectiveWidth(${targetMinimumEffectiveWidth.toFixed(2)})`);
>+
>+            await UIHelper.setMinimumEffectiveWidth(targetMinimumEffectiveWidth);
>+            await Promise.all([UIHelper.ensureVisibleContentRectUpdate(), UIHelper.ensurePresentationUpdate()]);
>+            appendOutput(`window size: [${innerWidth}, ${innerHeight}]`);
>+            appendOutput(`square size: [${square.clientWidth}, ${square.clientHeight}]`);
>+            appendOutput(`zoom scale: ${(await UIHelper.zoomScale()).toFixed(2)}`);
>+            appendOutput("");
>+        }
>+
>+        testRunner.notifyDone();
>+    }
>+    </script>
>+</head>
>+
>+<body onload="runTest()">
>+    <div id="square"></div>
>+    <pre id="output"></pre>
>+</body>
>+</html>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e6ee9ed4791..3865b3d579f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2018-12-04  Yongjun Zhang  <yongjun_zhang@apple.com>
>+
>+        We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
>+        https://bugs.webkit.org/show_bug.cgi?id=192377
>+        <rdar://problem/46364206>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If the page specifies width=device-width or initial-scale=1 in the viewport meta tag, we should use the
>+        native device width and ignore the minimum effective device width in ViewportConfiguration. The patch
>+        also introduces standardWebpageParameters() which uses the device width as default and also allows the page
>+        to shrink-to-fit. If a page doesn't have viewport meta tag, or if the width argument isn't device-width
>+        and the initial scale isn't 1, we will use standardWebpageParameters() as the default configuration.
>+
>+        Tests: fast/viewport/ios/ignore-minimum-device-width-for-page-with-viewport-device-width.html
>+               fast/viewport/ios/use-minimum-device-width-for-page-without-viewport-meta.html
>+
>+        * page/ViewportConfiguration.cpp:
>+        (WebCore::ViewportConfiguration::updateDefaultConfiguration): pick the default configuration based on
>+            the page's viewport arguments. Also, we will always fall back to standardWebpageParameters() if we
>+            can ignore scaling constraints.
>+        (WebCore::ViewportConfiguration::setViewportArguments): When page sends us new ViewportArguments, pick
>+            up the correponsding default configuration before updating the configuration.
>+        (WebCore::ViewportConfiguration::setCanIgnoreScalingConstraints): When m_canIgnoreScalingConstraints is
>+            changed, try to pick up the correponsding default configuration.
>+        (WebCore::ViewportConfiguration::standardWebpageParameters): Add a new default set of viewport Parameters
>+            this is very close to nativeWebpageParameters() excpet that it allows shrink to fit and its minimum scale
>+            is 0.25. We will use this Parameters for pages that doesn't have viewport meta tag; or the width is
>+            not device-width and initial scale is not 1.
>+        (WebCore::ViewportConfiguration::updateConfiguration): If the page's viewport argument doesn't override
>+            the default width, use the m_minimumLayoutSize.width().
>+        * page/ViewportConfiguration.h:
>+        (WebCore::ViewportConfiguration::shouldIgnoreMinimumEffectiveDeviceWidth const): A helper method to tell
>+            if we should avoid using minimum effective device width.
>+        (WebCore::ViewportConfiguration::canOverrideConfigurationParameters const): If we are using a default
>+            configuration that is neither nativeWebpageParameters() nor standardWebpageParameters(), don't override
>+            it.
>+        (WebCore::ViewportConfiguration::minimumEffectiveDeviceWidth const): Add a helper method to return minimum
>+            effective device width based on shouldIgnoreMinimumEffectiveDeviceWidth().
>+        (WebCore::ViewportConfiguration::effectiveLayoutSizeScaleFactor const): Use minimumEffectiveDeviceWidth().
>+
> 2018-12-04  Chris Dumez  <cdumez@apple.com>
> 
>         Regression(r238817) PSON Page Cache API tests are failing
>diff --git a/Source/WebCore/page/ViewportConfiguration.cpp b/Source/WebCore/page/ViewportConfiguration.cpp
>index 5cc814087b6..373cdade17c 100644
>--- a/Source/WebCore/page/ViewportConfiguration.cpp
>+++ b/Source/WebCore/page/ViewportConfiguration.cpp
>@@ -143,6 +143,27 @@ bool ViewportConfiguration::setDisabledAdaptations(const OptionSet<DisabledAdapt
>     return true;
> }
> 
>+bool ViewportConfiguration::canOverrideConfigurationParameters() const
>+{
>+    return m_defaultConfiguration == ViewportConfiguration::nativeWebpageParameters() || m_defaultConfiguration == ViewportConfiguration::standardWebpageParameters();
>+}
>+
>+void ViewportConfiguration::updateDefaultConfiguration()
>+{
>+    if (!canOverrideConfigurationParameters())
>+        return;
>+
>+    if (m_canIgnoreScalingConstraints) {
>+        m_defaultConfiguration = ViewportConfiguration::standardWebpageParameters();
>+        return;
>+    }
>+
>+    if (shouldIgnoreMinimumEffectiveDeviceWidth())
>+        m_defaultConfiguration = ViewportConfiguration::nativeWebpageParameters();
>+    else
>+        m_defaultConfiguration = ViewportConfiguration::standardWebpageParameters();
>+}
>+
> bool ViewportConfiguration::setViewportArguments(const ViewportArguments& viewportArguments)
> {
>     if (m_viewportArguments == viewportArguments)
>@@ -151,6 +172,7 @@ bool ViewportConfiguration::setViewportArguments(const ViewportArguments& viewpo
>     LOG_WITH_STREAM(Viewports, stream << "ViewportConfiguration::setViewportArguments " << viewportArguments);
>     m_viewportArguments = viewportArguments;
> 
>+    updateDefaultConfiguration();
>     updateMinimumLayoutSize();
>     updateConfiguration();
>     return true;
>@@ -162,6 +184,7 @@ bool ViewportConfiguration::setCanIgnoreScalingConstraints(bool canIgnoreScaling
>         return false;
>     
>     m_canIgnoreScalingConstraints = canIgnoreScalingConstraints;
>+    updateDefaultConfiguration();
>     updateConfiguration();
>     return true;
> }
>@@ -325,6 +348,21 @@ ViewportConfiguration::Parameters ViewportConfiguration::nativeWebpageParameters
>     return parameters;
> }
> 
>+ViewportConfiguration::Parameters ViewportConfiguration::standardWebpageParameters()
>+{
>+    Parameters parameters;
>+    parameters.width = ViewportArguments::ValueDeviceWidth;
>+    parameters.widthIsSet = true;
>+    parameters.allowsUserScaling = true;
>+    parameters.allowsShrinkToFit = true;
>+    parameters.minimumScale = 0.25;
>+    parameters.maximumScale = 5;
>+    parameters.initialScale = 1;
>+    parameters.initialScaleIsSet = true;
>+    parameters.initialScaleIgnoringLayoutScaleFactor = 1;
>+    return parameters;
>+}
>+
> ViewportConfiguration::Parameters ViewportConfiguration::webpageParameters()
> {
>     Parameters parameters;
>@@ -448,6 +486,9 @@ void ViewportConfiguration::updateConfiguration()
>     else if (booleanViewportArgumentIsSet(m_viewportArguments.shrinkToFit))
>         m_configuration.allowsShrinkToFit = m_viewportArguments.shrinkToFit != 0.;
> 
>+    if (canOverrideConfigurationParameters() && !viewportArgumentsOverridesWidth)
>+        m_configuration.width = m_minimumLayoutSize.width();
>+
>     m_configuration.avoidsUnsafeArea = m_viewportArguments.viewportFit != ViewportFit::Cover;
>     m_configuration.initialScaleIgnoringLayoutScaleFactor = m_configuration.initialScale;
>     float effectiveLayoutScale = effectiveLayoutSizeScaleFactor();
>diff --git a/Source/WebCore/page/ViewportConfiguration.h b/Source/WebCore/page/ViewportConfiguration.h
>index 4987be2c645..7380b7277d3 100644
>--- a/Source/WebCore/page/ViewportConfiguration.h
>+++ b/Source/WebCore/page/ViewportConfiguration.h
>@@ -103,6 +103,7 @@ public:
> 
>     // Matches a width=device-width, initial-scale=1 viewport.
>     WEBCORE_EXPORT static Parameters nativeWebpageParameters();
>+    static Parameters standardWebpageParameters();
>     WEBCORE_EXPORT static Parameters webpageParameters();
>     WEBCORE_EXPORT static Parameters textDocumentParameters();
>     WEBCORE_EXPORT static Parameters imageDocumentParameters();
>@@ -126,6 +127,29 @@ private:
>     bool shouldIgnoreScalingConstraints() const;
>     bool shouldIgnoreVerticalScalingConstraints() const;
>     bool shouldIgnoreHorizontalScalingConstraints() const;
>+    void updateDefaultConfiguration();
>+    bool canOverrideConfigurationParameters() const;
>+
>+    constexpr bool shouldIgnoreMinimumEffectiveDeviceWidth() const
>+    {
>+        if (m_canIgnoreScalingConstraints)
>+            return true;
>+
>+        if (m_viewportArguments == ViewportArguments())
>+            return false;
>+
>+        if (m_viewportArguments.width == ViewportArguments::ValueDeviceWidth || m_viewportArguments.zoom == 1.)
>+            return true;
>+
>+        return false;
>+    }
>+
>+    constexpr double minimumEffectiveDeviceWidth() const
>+    {
>+        if (shouldIgnoreMinimumEffectiveDeviceWidth())
>+            return 0;
>+        return m_minimumEffectiveDeviceWidth;
>+    }
> 
>     constexpr double forceAlwaysUserScalableMaximumScale() const
>     {
>@@ -141,9 +165,9 @@ private:
> 
>     constexpr double effectiveLayoutSizeScaleFactor() const
>     {
>-        if (!m_viewLayoutSize.width() || !m_minimumEffectiveDeviceWidth)
>+        if (!m_viewLayoutSize.width() || !minimumEffectiveDeviceWidth())
>             return m_layoutSizeScaleFactor;
>-        return m_layoutSizeScaleFactor * m_viewLayoutSize.width() / std::max<double>(m_minimumEffectiveDeviceWidth, m_viewLayoutSize.width());
>+        return m_layoutSizeScaleFactor * m_viewLayoutSize.width() / std::max<double>(minimumEffectiveDeviceWidth(), m_viewLayoutSize.width());
>     }
> 
>     void updateMinimumLayoutSize();
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 55dff1c019a..cc92160c687 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-12-04  Yongjun Zhang  <yongjun_zhang@apple.com>
>+
>+        We should ignore minimumEffectiveDeviceWidth if the page specifies device-width in viewport meta-tag.
>+        https://bugs.webkit.org/show_bug.cgi?id=192377
>+        <rdar://problem/46364206>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Since we are using page's viewport arguments to decide the default viewport parameters and
>+        whether we can use mininum effective device width, we should always call setViewportArguments()
>+        regardless of shouldIgnoreMetaViewport settings.
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::viewportPropertiesDidChange): Always call setViewportArguments().
>+        (WebKit::WebPage::didCommitLoad): Ditto.
>+
> 2018-12-04  Chris Dumez  <cdumez@apple.com>
> 
>         Regression(r238817) PSON Page Cache API tests are failing
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index ac475152d74..6f2ca1f5093 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -1926,7 +1926,7 @@ void WebPage::disabledAdaptationsDidChange(const OptionSet<DisabledAdaptations>&
> void WebPage::viewportPropertiesDidChange(const ViewportArguments& viewportArguments)
> {
> #if PLATFORM(IOS_FAMILY)
>-    if (!m_page->settings().shouldIgnoreMetaViewport() && m_viewportConfiguration.setViewportArguments(viewportArguments))
>+    if (m_viewportConfiguration.setViewportArguments(viewportArguments))
>         viewportConfigurationChanged();
> #endif
> 
>@@ -5541,7 +5541,7 @@ void WebPage::didCommitLoad(WebFrame* frame)
>     if (m_viewportConfiguration.setContentsSize(coreFrame->view()->contentsSize()))
>         viewportChanged = true;
> 
>-    if (!m_page->settings().shouldIgnoreMetaViewport() && m_viewportConfiguration.setViewportArguments(coreFrame->document()->viewportArguments()))
>+    if (m_viewportConfiguration.setViewportArguments(coreFrame->document()->viewportArguments()))
>         viewportChanged = true;
> 
>     if (viewportChanged)

Flags:

thorton: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 192377: 356548 | 356613 | 356762