| Summary: | Gradient on the body rendered incorrectly sometimes | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | 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: |
|
||||||||||||||||||||||||||||||||
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.
Created attachment 350455 [details]
Patch
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 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 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 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 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 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 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 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 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 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 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 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 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 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
Created attachment 350910 [details]
Patch
Created attachment 350912 [details]
Patch
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. Created attachment 350926 [details]
Patch
(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 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" Created attachment 351159 [details]
Patch
Created attachment 351160 [details]
Patch
Created attachment 351162 [details]
Patch
Created attachment 351165 [details]
Patch
Comment on attachment 351165 [details] Patch Clearing flags on attachment: 351165 Committed r236636: <https://trac.webkit.org/changeset/236636> All reviewed patches have been landed. Closing bug. |
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.