[AutoSizing] Avoid making text paragraphs scroll horizontally when there is a wide...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Apr 2019 23:42:18 +0000 (23:42 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Apr 2019 23:42:18 +0000 (23:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196743
<rdar://problem/43897551>

Reviewed by Tim Horton.

Source/WebCore:

This patch changes the auto size behavior by using fixed constraint (instead of a min/max pair) to compute the content height.
Now with the initial containing block width is firmly set to auto-sizing width, the overflow content will not stretch the ICB. Instead it overflows the ICB
and triggers scrolling the same way the non-auto-sizing mode does.

* page/FrameView.cpp:
(WebCore::FrameView::autoSizeIfEnabled):
(WebCore::FrameView::enableAutoSizeMode):
* page/FrameView.h:
* testing/Internals.cpp:
(WebCore::Internals::enableAutoSizeMode):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit:

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::setViewLayoutSize):

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm: expected behavior change.
(TEST):

LayoutTests:

* css3/viewport-percentage-lengths/vh-auto-size-expected.html:
* css3/viewport-percentage-lengths/vh-auto-size.html:
* fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html:
* fast/dynamic/mail-autosize-viewport-unit.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244098 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/css3/viewport-percentage-lengths/vh-auto-size-expected.html
LayoutTests/css3/viewport-percentage-lengths/vh-auto-size.html
LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html
LayoutTests/fast/dynamic/mail-autosize-viewport-unit.html
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm

index b785385..f480b3d 100644 (file)
@@ -1,3 +1,16 @@
+2019-04-09  Zalan Bujtas  <zalan@apple.com>
+
+        [AutoSizing] Avoid making text paragraphs scroll horizontally when there is a wide table
+        https://bugs.webkit.org/show_bug.cgi?id=196743
+        <rdar://problem/43897551>
+
+        Reviewed by Tim Horton.
+
+        * css3/viewport-percentage-lengths/vh-auto-size-expected.html:
+        * css3/viewport-percentage-lengths/vh-auto-size.html:
+        * fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html:
+        * fast/dynamic/mail-autosize-viewport-unit.html:
+
 2019-04-09  Daniel Bates  <dabates@apple.com>
 
         [iPad] Should open popover when the spacebar is pressed
index eac4f17..bcaf4d9 100644 (file)
@@ -1,6 +1,6 @@
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 1000, 1, 1000000, 1000000);
+    internals.enableAutoSizeMode(true, 1000, 1);
 </script>
 text text text text text text text
 <div style="height:1000px; border:2px solid green">
index 1699155..871fd8f 100644 (file)
@@ -2,7 +2,7 @@
 <head>
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 1000, 1, 1000000, 1000000);
+    internals.enableAutoSizeMode(true, 1000, 1);
 </script>
 </head>
 <body>
index acaa3ab..dd8d47a 100644 (file)
@@ -15,7 +15,7 @@
 <div>Pass if no crash or assert.<div id=resizeThis></div></div>
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 800, 600, 1000, 1000);
+    internals.enableAutoSizeMode(true, 800, 600);
 
 if (window.testRunner) {
     testRunner.waitUntilDone();
index be083c0..5d55cae 100644 (file)
@@ -25,7 +25,7 @@
 </style>
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 2000, 600, 4000, 1000);
+    internals.enableAutoSizeMode(true, 2000, 600);
 
 if (window.testRunner)
     testRunner.dumpAsText();
index a9351bf..af2d5e3 100644 (file)
@@ -1,3 +1,24 @@
+2019-04-09  Zalan Bujtas  <zalan@apple.com>
+
+        [AutoSizing] Avoid making text paragraphs scroll horizontally when there is a wide table
+        https://bugs.webkit.org/show_bug.cgi?id=196743
+        <rdar://problem/43897551>
+
+        Reviewed by Tim Horton.
+
+        This patch changes the auto size behavior by using fixed constraint (instead of a min/max pair) to compute the content height.
+        Now with the initial containing block width is firmly set to auto-sizing width, the overflow content will not stretch the ICB. Instead it overflows the ICB
+        and triggers scrolling the same way the non-auto-sizing mode does.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::autoSizeIfEnabled):
+        (WebCore::FrameView::enableAutoSizeMode):
+        * page/FrameView.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::enableAutoSizeMode):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2019-04-09  Youenn Fablet  <youenn@apple.com>
 
         Close service worker database on network process suspension
index 40715ec..6ca57e3 100644 (file)
@@ -3458,90 +3458,34 @@ void FrameView::autoSizeIfEnabled()
     if (!renderView)
         return;
 
+    auto* firstChild = renderView->firstChild();
+    if (!firstChild)
+        return;
+
     LOG(Layout, "FrameView %p autoSizeIfEnabled", this);
     SetForScope<bool> changeInAutoSize(m_inAutoSize, true);
     if (layoutContext().subtreeLayoutRoot())
         layoutContext().convertSubtreeLayoutToFullLayout();
-    // Start from the minimum size and allow it to grow.
-    resize(m_minAutoSize.width(), m_minAutoSize.height());
-    IntSize size = frameRect().size();
-    // Do the resizing twice. The first time is basically a rough calculation using the preferred width
-    // which may result in a height change during the second iteration.
-    for (int i = 0; i < 2; i++) {
-        // Update various sizes including contentsSize, scrollHeight, etc.
-        document->updateLayoutIgnorePendingStylesheets();
-        int width = renderView->minPreferredLogicalWidth();
-        int height = renderView->documentRect().height();
-        IntSize newSize(width, height);
-
-        // Check to see if a scrollbar is needed for a given dimension and
-        // if so, increase the other dimension to account for the scrollbar.
-        // Since the dimensions are only for the view rectangle, once a
-        // dimension exceeds the maximum, there is no need to increase it further.
-        if (newSize.width() > m_maxAutoSize.width()) {
-            RefPtr<Scrollbar> localHorizontalScrollbar = horizontalScrollbar();
-            if (!localHorizontalScrollbar)
-                localHorizontalScrollbar = createScrollbar(HorizontalScrollbar);
-            newSize.expand(0, localHorizontalScrollbar->occupiedHeight());
-
-            // Don't bother checking for a vertical scrollbar because the width is at
-            // already greater the maximum.
-        } else if (newSize.height() > m_maxAutoSize.height()) {
-            RefPtr<Scrollbar> localVerticalScrollbar = verticalScrollbar();
-            if (!localVerticalScrollbar)
-                localVerticalScrollbar = createScrollbar(VerticalScrollbar);
-            newSize.expand(localVerticalScrollbar->occupiedWidth(), 0);
-
-            // Don't bother checking for a horizontal scrollbar because the height is
-            // already greater the maximum.
-        }
-
-        // Ensure the size is at least the min bounds.
-        newSize = newSize.expandedTo(m_minAutoSize);
-
-        // Bound the dimensions by the max bounds and determine what scrollbars to show.
-        ScrollbarMode horizonalScrollbarMode = ScrollbarAlwaysOff;
-        if (newSize.width() > m_maxAutoSize.width()) {
-            newSize.setWidth(m_maxAutoSize.width());
-            horizonalScrollbarMode = ScrollbarAlwaysOn;
-        }
-        ScrollbarMode verticalScrollbarMode = ScrollbarAlwaysOff;
-        if (newSize.height() > m_maxAutoSize.height()) {
-            newSize.setHeight(m_maxAutoSize.height());
-            verticalScrollbarMode = ScrollbarAlwaysOn;
-        }
-
-        if (newSize == size)
-            continue;
 
-        // While loading only allow the size to increase (to avoid twitching during intermediate smaller states)
-        // unless autoresize has just been turned on or the maximum size is smaller than the current size.
-        if (m_didRunAutosize && size.height() <= m_maxAutoSize.height() && size.width() <= m_maxAutoSize.width()
-            && !frame().loader().isComplete() && (newSize.height() < size.height() || newSize.width() < size.width()))
-            break;
+    ScrollbarMode horizonalScrollbarMode = ScrollbarAlwaysOff;
+    ScrollbarMode verticalScrollbarMode = ScrollbarAlwaysOff;
+    setVerticalScrollbarLock(false);
+    setHorizontalScrollbarLock(false);
+    setScrollbarModes(horizonalScrollbarMode, verticalScrollbarMode, true, true);
 
-        // The first time around, resize to the minimum height again; otherwise,
-        // on pages (e.g. quirks mode) where the body/document resize to the view size,
-        // we'll end up not shrinking back down after resizing to the computed preferred width.
-        resize(newSize.width(), i ? newSize.height() : m_minAutoSize.height());
-        // Force the scrollbar state to avoid the scrollbar code adding them and causing them to be needed. For example,
-        // a vertical scrollbar may cause text to wrap and thus increase the height (which is the only reason the scollbar is needed).
-        setVerticalScrollbarLock(false);
-        setHorizontalScrollbarLock(false);
-        setScrollbarModes(horizonalScrollbarMode, verticalScrollbarMode, true, true);
-    }
-    // All the resizing above may have invalidated style (for example if viewport units are being used).
+    ASSERT(is<RenderElement>(*firstChild));
+    auto& documentRenderer = downcast<RenderElement>(*firstChild);
+    documentRenderer.mutableStyle().setMaxWidth(Length(m_autoSizeConstraint.width(), Fixed));
+    resize(m_autoSizeConstraint.width(), m_autoSizeConstraint.height());
     document->updateStyleIfNeeded();
-    // FIXME: Use the final layout's result as the content size (webkit.org/b/173561).
+    document->updateLayoutIgnorePendingStylesheets();
     m_autoSizeContentSize = contentsSize();
-    if (m_autoSizeFixedMinimumHeight) {
-        auto contentsSize = this->contentsSize();
-        resize(contentsSize.width(), std::max(m_autoSizeFixedMinimumHeight, contentsSize.height()));
-        document->updateLayoutIgnorePendingStylesheets();
-    }
-    m_didRunAutosize = true;
 
-    LOG_WITH_STREAM(Layout, stream << "FrameView " << this << " autoSizeIfEnabled() changed size from " << size << " to " << frameRect().size());
+    auto finalWidth = std::max(m_autoSizeConstraint.width(), m_autoSizeContentSize.width());
+    auto finalHeight = m_autoSizeFixedMinimumHeight ? std::max(m_autoSizeFixedMinimumHeight, m_autoSizeContentSize.height()) : m_autoSizeContentSize.height();
+    resize(finalWidth, finalHeight);
+    document->updateLayoutIgnorePendingStylesheets();
+    m_didRunAutosize = true;
 }
 
 void FrameView::setAutoSizeFixedMinimumHeight(int fixedMinimumHeight)
@@ -4534,24 +4478,20 @@ bool FrameView::isViewForDocumentInFrame() const
     return &renderView->frameView() == this;
 }
 
-void FrameView::enableAutoSizeMode(bool enable, const IntSize& minSize, const IntSize& maxSize)
+void FrameView::enableAutoSizeMode(bool enable, const IntSize& viewSize)
 {
-    ASSERT(!enable || !minSize.isEmpty());
-    ASSERT(minSize.width() <= maxSize.width());
-    ASSERT(minSize.height() <= maxSize.height());
-
-    if (m_shouldAutoSize == enable && m_minAutoSize == minSize && m_maxAutoSize == maxSize)
+    ASSERT(!enable || !viewSize.isEmpty());
+    if (m_shouldAutoSize == enable && m_autoSizeConstraint == viewSize)
         return;
 
     m_shouldAutoSize = enable;
-    m_minAutoSize = minSize;
-    m_maxAutoSize = maxSize;
+    m_autoSizeConstraint = viewSize;
     m_didRunAutosize = false;
 
     setNeedsLayoutAfterViewConfigurationChange();
     layoutContext().scheduleLayout();
     if (m_shouldAutoSize) {
-        overrideViewportSizeForCSSViewportUnits({ minSize.width(), m_overrideViewportSize ? m_overrideViewportSize->height : WTF::nullopt });
+        overrideViewportSizeForCSSViewportUnits({ m_autoSizeConstraint.width(), m_overrideViewportSize ? m_overrideViewportSize->height : WTF::nullopt });
         return;
     }
 
index 55f2a67..ae16ab6 100644 (file)
@@ -399,7 +399,7 @@ public:
     void updateIsVisuallyNonEmpty();
     void updateSignificantRenderedTextMilestoneIfNeeded();
     bool isVisuallyNonEmpty() const { return m_isVisuallyNonEmpty; }
-    WEBCORE_EXPORT void enableAutoSizeMode(bool enable, const IntSize& minSize, const IntSize& maxSize);
+    WEBCORE_EXPORT void enableAutoSizeMode(bool enable, const IntSize& minSize);
     WEBCORE_EXPORT void setAutoSizeFixedMinimumHeight(int);
     IntSize autoSizingIntrinsicContentSize() const { return m_autoSizeContentSize; }
 
@@ -873,10 +873,8 @@ private:
 
     Optional<OverrideViewportSize> m_overrideViewportSize;
 
-    // The lower bound on the size when autosizing.
-    IntSize m_minAutoSize;
-    // The upper bound on the size when autosizing.
-    IntSize m_maxAutoSize;
+    // The view size when autosizing.
+    IntSize m_autoSizeConstraint;
     // The fixed height to resize the view to after autosizing is complete.
     int m_autoSizeFixedMinimumHeight { 0 };
     // The intrinsic content size decided by autosizing.
index 9916799..4fcd6a7 100644 (file)
@@ -3384,12 +3384,12 @@ void Internals::reloadExpiredOnly()
     frame()->loader().reload(ReloadOption::ExpiredOnly);
 }
 
-void Internals::enableAutoSizeMode(bool enabled, int minimumWidth, int minimumHeight, int maximumWidth, int maximumHeight)
+void Internals::enableAutoSizeMode(bool enabled, int width, int height)
 {
     auto* document = contextDocument();
     if (!document || !document->view())
         return;
-    document->view()->enableAutoSizeMode(enabled, IntSize(minimumWidth, minimumHeight), IntSize(maximumWidth, maximumHeight));
+    document->view()->enableAutoSizeMode(enabled, { width, height });
 }
 
 #if ENABLE(LEGACY_ENCRYPTED_MEDIA)
index 27c3b71..c27f268 100644 (file)
@@ -493,7 +493,7 @@ public:
     void forceReload(bool endToEnd);
     void reloadExpiredOnly();
 
-    void enableAutoSizeMode(bool enabled, int minimumWidth, int minimumHeight, int maximumWidth, int maximumHeight);
+    void enableAutoSizeMode(bool enabled, int width, int height);
 
 #if ENABLE(LEGACY_ENCRYPTED_MEDIA)
     void initializeMockCDM();
index d74e8c6..fe878f5 100644 (file)
@@ -517,7 +517,7 @@ enum CompositingPolicy {
     void forceReload(boolean endToEnd);
     void reloadExpiredOnly();
 
-    void enableAutoSizeMode(boolean enabled, long minimumWidth, long minimumHeight, long maximumWidth, long maximumHeight);
+    void enableAutoSizeMode(boolean enabled, long width, long height);
 
     [Conditional=VIDEO] sequence<DOMString> mediaResponseSources(HTMLMediaElement media);
     [Conditional=VIDEO] sequence<DOMString> mediaResponseContentRanges(HTMLMediaElement media);
index b717e80..539bad4 100644 (file)
@@ -1,3 +1,16 @@
+2019-04-09  Zalan Bujtas  <zalan@apple.com>
+
+        [AutoSizing] Avoid making text paragraphs scroll horizontally when there is a wide table
+        https://bugs.webkit.org/show_bug.cgi?id=196743
+        <rdar://problem/43897551>
+
+        Reviewed by Tim Horton.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::setViewLayoutSize):
+
 2019-04-09  Youenn Fablet  <youenn@apple.com>
 
         Close service worker database on network process suspension
index 59bb20b..f572d5e 100644 (file)
@@ -1473,8 +1473,7 @@ void WebFrameLoaderClient::transitionToCommittedForNewPage()
 
     if (int viewLayoutWidth = webPage->viewLayoutSize().width()) {
         int viewLayoutHeight = std::max(webPage->viewLayoutSize().height(), 1);
-        int maximumSize = std::numeric_limits<int>::max();
-        m_frame->coreFrame()->view()->enableAutoSizeMode(true, IntSize(viewLayoutWidth, viewLayoutHeight), IntSize(maximumSize, maximumSize));
+        m_frame->coreFrame()->view()->enableAutoSizeMode(true, { viewLayoutWidth, viewLayoutHeight });
 
         if (webPage->autoSizingShouldExpandToViewHeight())
             m_frame->coreFrame()->view()->setAutoSizeFixedMinimumHeight(webPage->size().height());
index 06233c7..abe627b 100644 (file)
@@ -5482,16 +5482,13 @@ void WebPage::setViewLayoutSize(const IntSize& viewLayoutSize)
 
     m_viewLayoutSize = viewLayoutSize;
     if (viewLayoutSize.width() <= 0) {
-        corePage()->mainFrame().view()->enableAutoSizeMode(false, IntSize(), IntSize());
+        corePage()->mainFrame().view()->enableAutoSizeMode(false, { });
         return;
     }
 
     int viewLayoutWidth = viewLayoutSize.width();
     int viewLayoutHeight = std::max(viewLayoutSize.height(), 1);
-
-    int maximumSize = std::numeric_limits<int>::max();
-
-    corePage()->mainFrame().view()->enableAutoSizeMode(true, IntSize(viewLayoutWidth, viewLayoutHeight), IntSize(maximumSize, maximumSize));
+    corePage()->mainFrame().view()->enableAutoSizeMode(true, { viewLayoutWidth, viewLayoutHeight });
 }
 
 void WebPage::setAutoSizingShouldExpandToViewHeight(bool shouldExpand)
index 1c30a68..f66868e 100644 (file)
@@ -1,3 +1,14 @@
+2019-04-09  Zalan Bujtas  <zalan@apple.com>
+
+        [AutoSizing] Avoid making text paragraphs scroll horizontally when there is a wide table
+        https://bugs.webkit.org/show_bug.cgi?id=196743
+        <rdar://problem/43897551>
+
+        Reviewed by Tim Horton.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm: expected behavior change.
+        (TEST):
+
 2019-04-09  Youenn Fablet  <youenn@apple.com>
 
         Close service worker database on network process suspension
index 71fcda4..935fb32 100644 (file)
@@ -145,8 +145,8 @@ TEST(WebKit, AutoLayoutIntegration)
     // Changing the width to 100 should result in one rows of ten; with the constraint (width >= 100) -> 100x10
     [webView layoutAtMinimumWidth:100 andExpectContentSizeChange:NSMakeSize(100, 10) resettingWidth:YES];
 
-    // One 100x100 rect and ten 10x10 rects, inline; with the constraint (width >= 20) -> 100x110
-    [webView load:@"<div class='large'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div>" withWidth:20 expectingContentSize:NSMakeSize(100, 110)];
+    // One 100x100 rect and ten 10x10 rects, inline; with the constraint (width >= 20) -> 100x150
+    [webView load:@"<div class='large'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div><div class='small inline'></div>" withWidth:20 expectingContentSize:NSMakeSize(100, 150)];
 
     // With _shouldExpandContentToViewHeightForAutoLayout off (the default), the page should lay out to the intrinsic height
     // of the content.