Changeset 244098 in webkit

Timestamp:

Apr 9, 2019 4:42:18 PM (5 years ago)

Author:

Alan Bujtas

Message:

[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.

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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/css3/viewport-percentage-lengths/vh-auto-size-expected.html (modified) (1 diff)
• LayoutTests/css3/viewport-percentage-lengths/vh-auto-size.html (modified) (1 diff)
• LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html (modified) (1 diff)
• LayoutTests/fast/dynamic/mail-autosize-viewport-unit.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (3 diffs)
• Source/WebCore/page/FrameView.h (modified) (2 diffs)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (modified) (1 diff)
• Source/WebKit/WebProcess/WebPage/WebPage.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/css3/viewport-percentage-lengths/vh-auto-size-expected.html

@@ -1 +1 @@
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 1000, 1, 1000000, 1000000);
+    internals.enableAutoSizeMode(true, 1000, 1);
 </script>
 text text text text text text text

trunk/LayoutTests/css3/viewport-percentage-lengths/vh-auto-size.html

@@ -3 +3 @@
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 1000, 1, 1000000, 1000000);
+    internals.enableAutoSizeMode(true, 1000, 1);
 </script>
 </head>

trunk/LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html

@@ -16 +16 @@
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 800, 600, 1000, 1000);
+    internals.enableAutoSizeMode(true, 800, 600);
 
 if (window.testRunner) {

trunk/LayoutTests/fast/dynamic/mail-autosize-viewport-unit.html

@@ -26 +26 @@
 <script>
 if (window.internals)
-    internals.enableAutoSizeMode(true, 2000, 600, 4000, 1000);
+    internals.enableAutoSizeMode(true, 2000, 600);
 
 if (window.testRunner)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -3459 +3459 @@
         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;
-
-        // 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).
+
+    ScrollbarMode horizonalScrollbarMode = ScrollbarAlwaysOff;
+    ScrollbarMode verticalScrollbarMode = ScrollbarAlwaysOff;
+    setVerticalScrollbarLock(false);
+    setHorizontalScrollbarLock(false);
+    setScrollbarModes(horizonalScrollbarMode, verticalScrollbarMode, true, true);
+
+    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();
-    }
+
+    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;
-
-    LOG_WITH_STREAM(Layout, stream << "FrameView " << this << " autoSizeIfEnabled() changed size from " << size << " to " << frameRect().size());
 }
 
@@ -4535 +4479 @@
 }
 
-void FrameView::enableAutoSizeMode(bool enable, const IntSize& minSize, const IntSize& maxSize)
-{
-    ASSERT(!enable || !minSize.isEmpty());
-    ASSERT(minSize.width() <= maxSize.width());
-    ASSERT(minSize.height() <= maxSize.height());
-
-    if (m_shouldAutoSize == enable && m_minAutoSize == minSize && m_maxAutoSize == maxSize)
+void FrameView::enableAutoSizeMode(bool enable, const IntSize& viewSize)
+{
+    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;
 
@@ -4552 +4492 @@
     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;
     }

trunk/Source/WebCore/page/FrameView.h

@@ -400 +400 @@
     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; }
@@ -874 +874 @@
     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 };

trunk/Source/WebCore/testing/Internals.cpp

@@ -3385 +3385 @@
 }
 
-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 });
 }
 

trunk/Source/WebCore/testing/Internals.h

@@ -494 +494 @@
     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)

trunk/Source/WebCore/testing/Internals.idl

@@ -518 +518 @@
     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);

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

@@ -1474 +1474 @@
     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())

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -5483 +5483 @@
     m_viewLayoutSize = viewLayoutSize;
     if (viewLayoutSize.width() <= 0) {
-        corePage()->mainFrame().view()->enableAutoSizeMode(false, IntSize(), IntSize());
+        corePage()->mainFrame().view()->enableAutoSizeMode(false, { });
         return;
     }
@@ -5489 +5489 @@
     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 });
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+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>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/AutoLayoutIntegration.mm

@@ -146 +146 @@
     [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