Bug 177232

Summary: Gradient on the body rendered incorrectly sometimes
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Zamiul Haque <zhaque>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, bfulgham, commit-queue, ews-watchlist, megan_gardner, rniwa, simon.fraser, thorton, timothy, webkit-bug-importer, wenson_hsieh, zalan, zhaque
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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.
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (2.85 KB, patch)
2018-09-26 15:18 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Patch (4.18 KB, patch)
2018-09-26 17:04 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Patch (4.21 KB, patch)
2018-09-28 19:02 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Patch (4.20 KB, patch)
2018-09-28 19:07 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Patch (5.19 KB, patch)
2018-09-28 19:30 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Patch (5.11 KB, patch)
2018-09-28 19:50 PDT, Zamiul Haque 		no flags
DetailsFormatted DiffDiff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.
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.
Note You need to log in before you can comment on or make changes to this bug.