From 3ed884a6adff46cb5871508612832ab8691752ac Mon Sep 17 00:00:00 2001 From: Andy Date: Tue, 4 Aug 2020 13:54:01 -0700 Subject: Issue #1620 - Use Intrinsic Aspect Ratio for Images https://bugzilla.mozilla.org/show_bug.cgi?id=1547231 https://bugzilla.mozilla.org/show_bug.cgi?id=1559094 https://bugzilla.mozilla.org/show_bug.cgi?id=1633434 https://bugzilla.mozilla.org/show_bug.cgi?id=1565690 https://bugzilla.mozilla.org/show_bug.cgi?id=1602047 Make use of Aspect Ratios in Image frames before Images are loaded. - Check for width and height HTML properties and create a ratio with them. - Overwrite HTML size values with actual image dimensions on load. - Collapse any frames with srcless images. Comments: dom/html/nsGenericHTMLElement.cpp:1483 layout/generic/nsImageFrame.cpp:289 --- layout/generic/crashtests/1633434.html | 15 ++ layout/generic/crashtests/crashtests.list | 1 + layout/generic/nsImageFrame.cpp | 266 +++++++++++++++++------------- layout/generic/nsImageFrame.h | 20 ++- 4 files changed, 176 insertions(+), 126 deletions(-) create mode 100644 layout/generic/crashtests/1633434.html (limited to 'layout/generic') diff --git a/layout/generic/crashtests/1633434.html b/layout/generic/crashtests/1633434.html new file mode 100644 index 000000000..8a60b2072 --- /dev/null +++ b/layout/generic/crashtests/1633434.html @@ -0,0 +1,15 @@ + + + + + + diff --git a/layout/generic/crashtests/crashtests.list b/layout/generic/crashtests/crashtests.list index 1a597e51c..72872bfde 100644 --- a/layout/generic/crashtests/crashtests.list +++ b/layout/generic/crashtests/crashtests.list @@ -644,3 +644,4 @@ load 1304441.html load 1316649.html load 1381134.html load 1381134-2.html +load 1633434.html diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp index 365b7810b..3208a3233 100644 --- a/layout/generic/nsImageFrame.cpp +++ b/layout/generic/nsImageFrame.cpp @@ -14,6 +14,7 @@ #include "mozilla/gfx/2D.h" #include "mozilla/gfx/Helpers.h" #include "mozilla/gfx/PathHelpers.h" +#include "mozilla/dom/HTMLImageElement.h" #include "mozilla/MouseEvents.h" #include "mozilla/Unused.h" @@ -229,26 +230,26 @@ nsImageFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext) { nsAtomicContainerFrame::DidSetStyleContext(aOldStyleContext); - if (!mImage) { - // We'll pick this change up whenever we do get an image. - return; - } - nsStyleImageOrientation newOrientation = StyleVisibility()->mImageOrientation; // We need to update our orientation either if we had no style context before // because this is the first time it's been set, or if the image-orientation // property changed from its previous value. bool shouldUpdateOrientation = - !aOldStyleContext || - aOldStyleContext->StyleVisibility()->mImageOrientation != newOrientation; + mImage && + (!aOldStyleContext || + aOldStyleContext->StyleVisibility()->mImageOrientation != newOrientation); if (shouldUpdateOrientation) { nsCOMPtr image(mImage->Unwrap()); mImage = nsLayoutUtils::OrientImage(image, newOrientation); - UpdateIntrinsicSize(mImage); - UpdateIntrinsicRatio(mImage); + UpdateIntrinsicSize(); + UpdateIntrinsicRatio(); + } else if (!aOldStyleContext || + aOldStyleContext->StylePosition()->mAspectRatio != + StylePosition()->mAspectRatio) { + UpdateIntrinsicRatio(); } } @@ -286,50 +287,114 @@ nsImageFrame::Init(nsIContent* aContent, p->AdjustPriority(-1); } -bool -nsImageFrame::UpdateIntrinsicSize(imgIContainer* aImage) +// 2020-07-14 (RealityRipple) Firefox is doing this completely differently +// because of loading="lazy" support and the StyleDisplay()->IsContainSize() +// property. Double-check all of this for problems. + +static IntrinsicSize +ComputeIntrinsicSize(imgIContainer* aImage, + bool aUseMappedRatio, + const nsImageFrame& aFrame) { - NS_PRECONDITION(aImage, "null image"); - if (!aImage) - return false; + // When 'contain: size' is implemented, make sure to check for it. +/* + const ComputedStyle& style = *aFrame.Style(); + if (style.StyleDisplay()->IsContainSize()) { + return AspectRatio(); + } + */ + nsSize size; + IntrinsicSize intrinsicSize; + if (aImage && NS_SUCCEEDED(aImage->GetIntrinsicSize(&size))) { + if (size.width != -1) + intrinsicSize.width.SetCoordValue(size.width); + if (size.height != -1) + intrinsicSize.height.SetCoordValue(size.height); + return intrinsicSize; + } - IntrinsicSize oldIntrinsicSize = mIntrinsicSize; - mIntrinsicSize = IntrinsicSize(); - - // Set intrinsic size to match aImage's reported intrinsic width & height. - nsSize intrinsicSize; - if (NS_SUCCEEDED(aImage->GetIntrinsicSize(&intrinsicSize))) { - // If the image has no intrinsic width, intrinsicSize.width will be -1, and - // we can leave mIntrinsicSize.width at its default value of eStyleUnit_None. - // Otherwise we use intrinsicSize.width. Height works the same way. - if (intrinsicSize.width != -1) - mIntrinsicSize.width.SetCoordValue(intrinsicSize.width); - if (intrinsicSize.height != -1) - mIntrinsicSize.height.SetCoordValue(intrinsicSize.height); - } else { - // Failure means that the image hasn't loaded enough to report a result. We - // treat this case as if the image's intrinsic size was 0x0. - mIntrinsicSize.width.SetCoordValue(0); - mIntrinsicSize.height.SetCoordValue(0); + // If broken images should ever lose their size + /* + if (aFrame.ShouldShowBrokenImageIcon()) { + nscoord edgeLengthToUse = nsPresContext::CSSPixelsToAppUnits( + ICON_SIZE + (2 * (ICON_PADDING + ALT_BORDER_WIDTH))); + intrinsicSize.width.SetCoordValue(edgeLengthToUse); + intrinsicSize.height.SetCoordValue(edgeLengthToUse); + return intrinsicSize; } + */ - return mIntrinsicSize != oldIntrinsicSize; + if (aUseMappedRatio && aFrame.StylePosition()->mAspectRatio != 0.0f) { + return IntrinsicSize(); + } + + intrinsicSize.width.SetCoordValue(0); + intrinsicSize.height.SetCoordValue(0); + return intrinsicSize; +} + +// For compat reasons, see bug 1602047, we don't use the intrinsic ratio from +// width="" and height="" for images with no src attribute (no request). +// +// If ever gets implemented, this will need to check for it. +bool nsImageFrame::ShouldUseMappedAspectRatio() const { + nsCOMPtr currentRequest; + nsCOMPtr imageLoader = do_QueryInterface(mContent); + if (imageLoader) { + imageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, + getter_AddRefs(currentRequest)); + } + if (!!currentRequest) { + return true; + } + // TODO(emilio): Investigate the compat situation of the above check, maybe we + // can just check for empty src attribute or something... + auto* image = static_cast(mContent); + return image && image->IsAwaitingLoad(); } bool -nsImageFrame::UpdateIntrinsicRatio(imgIContainer* aImage) +nsImageFrame::UpdateIntrinsicSize() { - NS_PRECONDITION(aImage, "null image"); - - if (!aImage) - return false; + IntrinsicSize oldIntrinsicSize = mIntrinsicSize; + mIntrinsicSize = ComputeIntrinsicSize(mImage, ShouldUseMappedAspectRatio(), *this); + return mIntrinsicSize != oldIntrinsicSize; +} - AspectRatio oldIntrinsicRatio = mIntrinsicRatio; +static AspectRatio +ComputeAspectRatio(imgIContainer* aImage, + bool aUseMappedRatio, + const nsImageFrame& aFrame) +{ + // When 'contain: size' is implemented, make sure to check for it. +/* + const ComputedStyle& style = *aFrame.Style(); + if (style.StyleDisplay()->IsContainSize()) { + return AspectRatio(); + } + */ + if (aImage) { + AspectRatio fromImage; + if (NS_SUCCEEDED(aImage->GetIntrinsicRatio(&fromImage))) { + return fromImage; + } + } + if (aUseMappedRatio && aFrame.StylePosition()->mAspectRatio != 0.0f) { + return AspectRatio(aFrame.StylePosition()->mAspectRatio); + } + if (aFrame.ShouldShowBrokenImageIcon()) { + return AspectRatio(1.0f); + } + return AspectRatio(); +} - // Set intrinsic ratio to match aImage's reported intrinsic ratio. - if (NS_FAILED(aImage->GetIntrinsicRatio(&mIntrinsicRatio))) - mIntrinsicRatio = AspectRatio(); +bool +nsImageFrame::UpdateIntrinsicRatio() +{ + AspectRatio oldIntrinsicRatio = mIntrinsicRatio; + mIntrinsicRatio = + ComputeAspectRatio(mImage, ShouldUseMappedAspectRatio(), *this); return mIntrinsicRatio != oldIntrinsicRatio; } @@ -541,30 +606,38 @@ nsImageFrame::OnSizeAvailable(imgIRequest* aRequest, imgIContainer* aImage) return NS_OK; } - bool intrinsicSizeChanged = false; + UpdateImage(aRequest, aImage); + return NS_OK; +} + +void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) { + MOZ_ASSERT(aRequest); if (SizeIsAvailable(aRequest)) { // This is valid and for the current request, so update our stored image // container, orienting according to our style. - mImage = nsLayoutUtils::OrientImage(aImage, StyleVisibility()->mImageOrientation); - - intrinsicSizeChanged = UpdateIntrinsicSize(mImage); - intrinsicSizeChanged = UpdateIntrinsicRatio(mImage) || intrinsicSizeChanged; + mImage = nsLayoutUtils::OrientImage(aImage, + StyleVisibility()->mImageOrientation); + MOZ_ASSERT(mImage); } else { // We no longer have a valid image, so release our stored image container. mImage = mPrevImage = nullptr; - - // Have to size to 0,0 so that GetDesiredSize recalculates the size. - mIntrinsicSize.width.SetCoordValue(0); - mIntrinsicSize.height.SetCoordValue(0); - mIntrinsicRatio = AspectRatio(); - intrinsicSizeChanged = true; } + // NOTE(emilio): Intentionally using `|` instead of `||` to avoid + // short-circuiting. + bool intrinsicSizeChanged = + UpdateIntrinsicSize() | UpdateIntrinsicRatio(); + if (!(mState & IMAGE_GOTINITIALREFLOW)) { + return; + } + + // We're going to need to repaint now either way. + InvalidateFrame(); - if (intrinsicSizeChanged && (mState & IMAGE_GOTINITIALREFLOW)) { + if (intrinsicSizeChanged) { // Now we need to reflow if we have an unconstrained size and have - // already gotten the initial reflow + // already gotten the initial reflow. if (!(mState & IMAGE_SIZECONSTRAINED)) { - nsIPresShell *presShell = presContext->GetPresShell(); + nsIPresShell *presShell = PresContext()->GetPresShell(); NS_ASSERTION(presShell, "No PresShell."); if (presShell) { presShell->FrameNeedsReflow(this, nsIPresShell::eStyleChange, @@ -578,8 +651,6 @@ nsImageFrame::OnSizeAvailable(imgIRequest* aRequest, imgIContainer* aImage) mPrevImage = nullptr; } - - return NS_OK; } nsresult @@ -654,45 +725,9 @@ nsImageFrame::NotifyNewCurrentRequest(imgIRequest *aRequest, { nsCOMPtr image; aRequest->GetImage(getter_AddRefs(image)); - NS_ASSERTION(image || NS_FAILED(aStatus), "Successful load with no container?"); - - // May have to switch sizes here! - bool intrinsicSizeChanged = true; - if (NS_SUCCEEDED(aStatus) && image && SizeIsAvailable(aRequest)) { - // Update our stored image container, orienting according to our style. - mImage = nsLayoutUtils::OrientImage(image, StyleVisibility()->mImageOrientation); - - intrinsicSizeChanged = UpdateIntrinsicSize(mImage); - intrinsicSizeChanged = UpdateIntrinsicRatio(mImage) || intrinsicSizeChanged; - } else { - // We no longer have a valid image, so release our stored image container. - mImage = mPrevImage = nullptr; - - // Have to size to 0,0 so that GetDesiredSize recalculates the size - mIntrinsicSize.width.SetCoordValue(0); - mIntrinsicSize.height.SetCoordValue(0); - mIntrinsicRatio = AspectRatio(); - } - - if (mState & IMAGE_GOTINITIALREFLOW) { // do nothing if we haven't gotten the initial reflow yet - if (intrinsicSizeChanged) { - if (!(mState & IMAGE_SIZECONSTRAINED)) { - nsIPresShell *presShell = PresContext()->GetPresShell(); - if (presShell) { - presShell->FrameNeedsReflow(this, nsIPresShell::eStyleChange, - NS_FRAME_IS_DIRTY); - } - } else { - // We've already gotten the initial reflow, and our size hasn't changed, - // so we're ready to request a decode. - MaybeDecodeForPredictedSize(); - } - - mPrevImage = nullptr; - } - // Update border+content to account for image change - InvalidateFrame(); - } + NS_ASSERTION(image || NS_FAILED(aStatus), + "Successful load with no container?"); + UpdateImage(aRequest, image); } void @@ -786,32 +821,27 @@ bool nsImageFrame::ShouldShowBrokenImageIcon() const void nsImageFrame::EnsureIntrinsicSizeAndRatio() { + // When 'contain: size' is implemented, make sure to check for it. +/* + if (StyleDisplay()->IsContainSize()) { + // If we have 'contain:size', then our intrinsic size and ratio are 0,0 + // regardless of what our underlying image may think. + mIntrinsicSize = IntrinsicSize(0, 0); + mIntrinsicRatio = AspectRatio(); + return; + } + */ + // If mIntrinsicSize.width and height are 0, then we need to update from the // image container. - if (mIntrinsicSize.width.GetUnit() == eStyleUnit_Coord && + if (!(mIntrinsicSize.width.GetUnit() == eStyleUnit_Coord && mIntrinsicSize.width.GetCoordValue() == 0 && mIntrinsicSize.height.GetUnit() == eStyleUnit_Coord && - mIntrinsicSize.height.GetCoordValue() == 0) { - - if (mImage) { - UpdateIntrinsicSize(mImage); - UpdateIntrinsicRatio(mImage); - } else { - // Image request is null or image size not known. - if (!(GetStateBits() & NS_FRAME_GENERATED_CONTENT)) { - // Likely an invalid image. Check if we should display it as broken. - if (ShouldShowBrokenImageIcon()) { - // Invalid image specified. make the image big enough for the "broken" icon - nscoord edgeLengthToUse = - nsPresContext::CSSPixelsToAppUnits( - ICON_SIZE + (2 * (ICON_PADDING + ALT_BORDER_WIDTH))); - mIntrinsicSize.width.SetCoordValue(edgeLengthToUse); - mIntrinsicSize.height.SetCoordValue(edgeLengthToUse); - mIntrinsicRatio = AspectRatio(1.0f); - } - } - } + mIntrinsicSize.height.GetCoordValue() == 0)) { + return; } + UpdateIntrinsicSize(); + UpdateIntrinsicRatio(); } /* virtual */ diff --git a/layout/generic/nsImageFrame.h b/layout/generic/nsImageFrame.h index 2414d89df..5e9b67274 100644 --- a/layout/generic/nsImageFrame.h +++ b/layout/generic/nsImageFrame.h @@ -273,21 +273,19 @@ private: void GetDocumentCharacterSet(nsACString& aCharset) const; bool ShouldDisplaySelection(); + // Whether the image frame should use the mapped aspect ratio from width="" + // and height="". + bool ShouldUseMappedAspectRatio() const; + /** * Recalculate mIntrinsicSize from the image. - * - * @return whether aImage's size did _not_ - * match our previous intrinsic size. */ - bool UpdateIntrinsicSize(imgIContainer* aImage); + bool UpdateIntrinsicSize(); /** * Recalculate mIntrinsicRatio from the image. - * - * @return whether aImage's ratio did _not_ - * match our previous intrinsic ratio. */ - bool UpdateIntrinsicRatio(imgIContainer* aImage); + bool UpdateIntrinsicRatio(); /** * This function calculates the transform for converting between @@ -307,6 +305,12 @@ private: */ bool IsPendingLoad(imgIRequest* aRequest) const; + /** + * Updates mImage based on the current image request (cannot be null), and the + * image passed in (can be null), and invalidate layout and paint as needed. + */ + void UpdateImage(imgIRequest* aRequest, imgIContainer* aImage); + /** * Function to convert a dirty rect in the source image to a dirty * rect for the image frame. -- cgit v1.2.3 From 644f617e6612b3a147e50b06c244fcc9362c9605 Mon Sep 17 00:00:00 2001 From: Andy Date: Fri, 7 Aug 2020 14:29:41 -0700 Subject: Issue #1620 - Remove Development Comments --- layout/generic/nsImageFrame.cpp | 4 ---- 1 file changed, 4 deletions(-) (limited to 'layout/generic') diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp index 3208a3233..d6f47f2e4 100644 --- a/layout/generic/nsImageFrame.cpp +++ b/layout/generic/nsImageFrame.cpp @@ -287,10 +287,6 @@ nsImageFrame::Init(nsIContent* aContent, p->AdjustPriority(-1); } -// 2020-07-14 (RealityRipple) Firefox is doing this completely differently -// because of loading="lazy" support and the StyleDisplay()->IsContainSize() -// property. Double-check all of this for problems. - static IntrinsicSize ComputeIntrinsicSize(imgIContainer* aImage, bool aUseMappedRatio, -- cgit v1.2.3