177232 – Gradient on the body rendered incorrectly sometimes

Simon Fraser (smfr)

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

Radar WebKit Bug Importer

Comment 1 2017-09-20 07:41:59 PDT

<rdar://problem/34548230>

Simon Fraser (smfr)

Comment 2 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.

Zamiul Haque

Comment 3 2018-09-21 16:49:58 PDT

Created attachment 350455 [details] Patch

EWS Watchlist

Comment 4 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.

Zamiul Haque

Comment 5 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)); > } > } >

Tim Horton

Comment 6 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).

Zamiul Haque

Comment 7 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)); > } > } >

EWS Watchlist

Comment 8 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

EWS Watchlist

Comment 9 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

EWS Watchlist

Comment 10 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

EWS Watchlist

Comment 11 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

EWS Watchlist

Comment 12 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

EWS Watchlist

Comment 13 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

EWS Watchlist

Comment 14 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

EWS Watchlist

Comment 15 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

EWS Watchlist

Comment 16 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

EWS Watchlist

Comment 17 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

Zamiul Haque

Comment 18 2018-09-26 15:12:51 PDT

Created attachment 350910 [details] Patch

Zamiul Haque

Comment 19 2018-09-26 15:18:04 PDT

Created attachment 350912 [details] Patch

Tim Horton

Comment 20 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.

Zamiul Haque

Comment 21 2018-09-26 17:04:39 PDT

Created attachment 350926 [details] Patch

Zamiul Haque

Comment 22 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.

Tim Horton

Comment 23 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"

Zamiul Haque

Comment 24 2018-09-28 19:02:49 PDT

Created attachment 351159 [details] Patch

Zamiul Haque

Comment 25 2018-09-28 19:07:20 PDT

Created attachment 351160 [details] Patch

Zamiul Haque

Comment 26 2018-09-28 19:30:37 PDT

Created attachment 351162 [details] Patch

Zamiul Haque

Comment 27 2018-09-28 19:50:27 PDT

Created attachment 351165 [details] Patch

WebKit Commit Bot

Comment 28 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>

WebKit Commit Bot

Comment 29 2018-09-28 20:29:13 PDT

All reviewed patches have been landed. Closing bug.

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
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.