Bug 177232 - Gradient on the body rendered incorrectly sometimes
Summary: Gradient on the body rendered incorrectly sometimes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zamiul Haque
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-20 07:41 PDT by Simon Fraser (smfr)
Modified: 2018-09-28 20:29 PDT (History)
14 users (show)

See Also:


Attachments
Testcase (221 bytes, text/html)
2017-09-20 07:41 PDT, Simon Fraser (smfr)
no flags Details
Patch (2.93 KB, patch)
2018-09-21 16:49 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (3.06 MB, application/zip)
2018-09-21 18:53 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.71 MB, application/zip)
2018-09-21 19:35 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.64 MB, application/zip)
2018-09-21 20:06 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (12.94 MB, application/zip)
2018-09-22 02:25 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.56 MB, application/zip)
2018-09-22 03:48 PDT, EWS Watchlist
no flags Details
Patch (3.30 KB, patch)
2018-09-26 15:12 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (2.85 KB, patch)
2018-09-26 15:18 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (4.18 KB, patch)
2018-09-26 17:04 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2018-09-28 19:02 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2018-09-28 19:07 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2018-09-28 19:30 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2018-09-28 19:50 PDT, Zamiul Haque
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-09-20 07:41:05 PDT
Created attachment 321315 [details]
Testcase

If the body is zero height, but has an angled repeating gradient, then the gradient renders as if it were vertical. We should propagate it to the root background and just draw the gradient normally.
Comment 1 Radar WebKit Bug Importer 2017-09-20 07:41:59 PDT
<rdar://problem/34548230>
Comment 2 Simon Fraser (smfr) 2017-09-20 15:01:18 PDT
calculateBackgroundImageGeometry() uses the size of the <body> for positioningAreaSize which is used to compute the tile size, which ends up with zero height, but the tileSize gets expanded to one device pixel via:

            return LayoutSize(std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.width() * scaleFactor),
                std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.height() * scaleFactor));

in RenderBoxModelObject::calculateFillTileSize() which should probably never make 0 bigger.

Other browsers draw no background for this testcase.
Comment 3 Zamiul Haque 2018-09-21 16:49:58 PDT
Created attachment 350455 [details]
Patch
Comment 4 EWS Watchlist 2018-09-21 16:52:18 PDT
Attachment 350455 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/ChangeLog:26:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Zamiul Haque 2018-09-21 16:52:30 PDT
Comment on attachment 350455 [details]
Patch

>Subversion Revision: 235906
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 363bd7bab31921d9f60066a85bd2f96a936fa581..fbe8654adfe0b51b48414db2cb57db186421a2f4 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-09-21  Zamiul Haque  <zhaque@apple.com>
>+
>+        Gradient on the body rendered incorrectly sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=177232. Specifically,
>+        gradients displayed at an angle (ie. 45 degrees) are rendered
>+        as if they are vertical when the body tag containing the gradient
>+        has a height of 0. Other browsers do not render under these circumstances,
>+        so WebKit was modified to follow in suit. The problem was due to layout sizes for 
>+        fill tiles being calculated with a minimum height of 1px. A simple change of the 
>+        minimum height and width to 0px was enough to bring about the desired behavior.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * rendering/RenderBoxModelObject.cpp:
>+        (WebCore::RenderBoxModelObject::calculateFillTileSize const):
>+
> 2018-09-11  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Unreviewed, fix some -Wreturn-type warnings
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>index d0ffc3354c4b4a60b3861bc94d376b9281aabf7b..baec8a7f7048154e2b596ead90c020927f1cbefa 100644
>--- a/Source/WebCore/rendering/RenderBoxModelObject.cpp
>+++ b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>@@ -1149,9 +1149,8 @@ LayoutSize RenderBoxModelObject::calculateFillTileSize(const FillLayer& fillLaye
>         float horizontalScaleFactor = localImageIntrinsicSize.width() ? (localPositioningAreaSize.width() / localImageIntrinsicSize.width()) : 1;
>         float verticalScaleFactor = localImageIntrinsicSize.height() ? (localPositioningAreaSize.height() / localImageIntrinsicSize.height()) : 1;
>         float scaleFactor = type == FillSizeType::Contain ? std::min(horizontalScaleFactor, verticalScaleFactor) : std::max(horizontalScaleFactor, verticalScaleFactor);
>-        float deviceScaleFactor = document().deviceScaleFactor();
>-        return LayoutSize(std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.width() * scaleFactor),
>-            std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.height() * scaleFactor));
>+        return LayoutSize(std::max<LayoutUnit>(0, localImageIntrinsicSize.width() * scaleFactor),
>+            std::max<LayoutUnit>(0, localImageIntrinsicSize.height() * scaleFactor));
>     }
>     }
>
Comment 6 Tim Horton 2018-09-21 17:32:26 PDT
Comment on attachment 350455 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=177232

I think there's a radar # (with no title!) that should be listed below this line.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Probably need a test!

> Source/WebCore/ChangeLog:15
> +2018-09-21  Zamiul Haque  <zhaque@apple.com>
> +
> +        Gradient on the body rendered incorrectly sometimes

You've got duplicate changelogs here. Try to deduplicate them and make them look like a normal changelog (look at another commit for examples).
Comment 7 Zamiul Haque 2018-09-21 17:34:32 PDT
Comment on attachment 350455 [details]
Patch

>Subversion Revision: 235906
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 363bd7bab31921d9f60066a85bd2f96a936fa581..fbe8654adfe0b51b48414db2cb57db186421a2f4 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-09-21  Zamiul Haque  <zhaque@apple.com>
>+
>+        Gradient on the body rendered incorrectly sometimes
>+        https://bugs.webkit.org/show_bug.cgi?id=177232. Specifically,
>+        gradients displayed at an angle (ie. 45 degrees) are rendered
>+        as if they are vertical when the body tag containing the gradient
>+        has a height of 0. Other browsers do not render under these circumstances,
>+        so WebKit was modified to follow in suit. The problem was due to layout sizes for 
>+        fill tiles being calculated with a minimum height of 1px. A simple change of the 
>+        minimum height and width to 0px was enough to bring about the desired behavior.
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * rendering/RenderBoxModelObject.cpp:
>+        (WebCore::RenderBoxModelObject::calculateFillTileSize const):
>+
> 2018-09-11  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Unreviewed, fix some -Wreturn-type warnings
>diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>index d0ffc3354c4b4a60b3861bc94d376b9281aabf7b..baec8a7f7048154e2b596ead90c020927f1cbefa 100644
>--- a/Source/WebCore/rendering/RenderBoxModelObject.cpp
>+++ b/Source/WebCore/rendering/RenderBoxModelObject.cpp
>@@ -1149,9 +1149,8 @@ LayoutSize RenderBoxModelObject::calculateFillTileSize(const FillLayer& fillLaye
>         float horizontalScaleFactor = localImageIntrinsicSize.width() ? (localPositioningAreaSize.width() / localImageIntrinsicSize.width()) : 1;
>         float verticalScaleFactor = localImageIntrinsicSize.height() ? (localPositioningAreaSize.height() / localImageIntrinsicSize.height()) : 1;
>         float scaleFactor = type == FillSizeType::Contain ? std::min(horizontalScaleFactor, verticalScaleFactor) : std::max(horizontalScaleFactor, verticalScaleFactor);
>-        float deviceScaleFactor = document().deviceScaleFactor();
>-        return LayoutSize(std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.width() * scaleFactor),
>-            std::max<LayoutUnit>(1 / deviceScaleFactor, localImageIntrinsicSize.height() * scaleFactor));
>+        return LayoutSize(std::max<LayoutUnit>(0, localImageIntrinsicSize.width() * scaleFactor),
>+            std::max<LayoutUnit>(0, localImageIntrinsicSize.height() * scaleFactor));
>     }
>     }
>
Comment 8 EWS Watchlist 2018-09-21 18:52:58 PDT
Comment on attachment 350455 [details]
Patch

Attachment 350455 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9305905

New failing tests:
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 9 EWS Watchlist 2018-09-21 18:53:01 PDT
Created attachment 350474 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-09-21 19:35:48 PDT
Comment on attachment 350455 [details]
Patch

Attachment 350455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9306278

New failing tests:
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 11 EWS Watchlist 2018-09-21 19:35:50 PDT
Created attachment 350477 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-09-21 20:06:30 PDT
Comment on attachment 350455 [details]
Patch

Attachment 350455 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9306312

New failing tests:
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 13 EWS Watchlist 2018-09-21 20:06:32 PDT
Created attachment 350483 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2018-09-22 02:25:29 PDT
Comment on attachment 350455 [details]
Patch

Attachment 350455 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9308855

New failing tests:
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 15 EWS Watchlist 2018-09-22 02:25:31 PDT
Created attachment 350509 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 16 EWS Watchlist 2018-09-22 03:48:39 PDT
Comment on attachment 350455 [details]
Patch

Attachment 350455 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9309839

New failing tests:
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html
Comment 17 EWS Watchlist 2018-09-22 03:48:41 PDT
Created attachment 350515 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 18 Zamiul Haque 2018-09-26 15:12:51 PDT
Created attachment 350910 [details]
Patch
Comment 19 Zamiul Haque 2018-09-26 15:18:04 PDT
Created attachment 350912 [details]
Patch
Comment 20 Tim Horton 2018-09-26 15:28:29 PDT
Comment on attachment 350912 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1155
> +        if (!localImageIntrinsicSize.width() || !localImageIntrinsicSize.height())

FloatSize has an isEmpty method which we generally prefer for things like this.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1158
> +        return LayoutSize(std::max<LayoutUnit>(1.0 / document().deviceScaleFactor(), localImageIntrinsicSize.width() * scaleFactor),

Please stash deviceScaleFactor in a local, and consider using FloatSize::scaled or ::scale too.
Comment 21 Zamiul Haque 2018-09-26 17:04:39 PDT
Created attachment 350926 [details]
Patch
Comment 22 Zamiul Haque 2018-09-26 17:05:27 PDT
(In reply to Tim Horton from comment #20)
> Comment on attachment 350912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350912&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1155
> > +        if (!localImageIntrinsicSize.width() || !localImageIntrinsicSize.height())
> 
> FloatSize has an isEmpty method which we generally prefer for things like
> this.
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1158
> > +        return LayoutSize(std::max<LayoutUnit>(1.0 / document().deviceScaleFactor(), localImageIntrinsicSize.width() * scaleFactor),
> 
> Please stash deviceScaleFactor in a local, and consider using
> FloatSize::scaled or ::scale too.

Made those changes you asked for, in the new attachment.
Comment 23 Tim Horton 2018-09-28 16:59:19 PDT
Comment on attachment 350926 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Gradient on the body rendered incorrectly sometimes

The ideal title would be a bit more specific about "incorrectly" and "sometimes"
Comment 24 Zamiul Haque 2018-09-28 19:02:49 PDT
Created attachment 351159 [details]
Patch
Comment 25 Zamiul Haque 2018-09-28 19:07:20 PDT
Created attachment 351160 [details]
Patch
Comment 26 Zamiul Haque 2018-09-28 19:30:37 PDT
Created attachment 351162 [details]
Patch
Comment 27 Zamiul Haque 2018-09-28 19:50:27 PDT
Created attachment 351165 [details]
Patch
Comment 28 WebKit Commit Bot 2018-09-28 20:29:11 PDT
Comment on attachment 351165 [details]
Patch

Clearing flags on attachment: 351165

Committed r236636: <https://trac.webkit.org/changeset/236636>
Comment 29 WebKit Commit Bot 2018-09-28 20:29:13 PDT
All reviewed patches have been landed.  Closing bug.