From 97b7562b6b5cc6069ec7681edd9b2f5a33f1651c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 2 Sep 2022 09:39:38 +0000 Subject: [PATCH] Bug 1787072 - Avoid useless reframes setting the src attribute of a broken image that already has an image frame. r=dholbert Differential Revision: https://phabricator.services.mozilla.com/D155533 --- layout/base/RestyleManager.cpp | 14 ++- layout/base/nsCSSFrameConstructor.cpp | 10 +- layout/generic/nsIFrame.cpp | 5 + layout/generic/nsIFrame.h | 6 + layout/generic/nsImageFrame.cpp | 108 +++++++++--------- layout/generic/nsImageFrame.h | 24 ++-- layout/style/res/html.css | 8 +- layout/style/test/mochitest.ini | 1 + .../test/test_img_src_causing_reflow.html | 36 ++++++ 9 files changed, 144 insertions(+), 68 deletions(-) create mode 100644 layout/style/test/test_img_src_causing_reflow.html diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index 9048febb6ce3..2925d395aad8 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -486,9 +486,15 @@ static bool StateChangeMayAffectFrame(const Element& aElement, } if (aElement.IsHTMLElement(nsGkAtoms::img)) { - // Loading state doesn't affect , see - // `nsImageFrame::ShouldCreateImageFrameFor`. - return brokenChanged; + if (!brokenChanged) { + // Loading state doesn't affect , see + // `nsImageFrame::ImageFrameTypeForElement`. + return false; + } + const bool needsImageFrame = + nsImageFrame::ImageFrameTypeFor(aElement, *aFrame.Style()) != + nsImageFrame::ImageFrameType::None; + return needsImageFrame != aFrame.IsImageFrameOrSubclass(); } if (aElement.IsSVGElement(nsGkAtoms::image)) { @@ -2705,7 +2711,7 @@ bool RestyleManager::ProcessPostTraversal(Element* aElement, // We should really fix the weird primary frame mapping for image maps // (bug 135040)... if (styleFrame && styleFrame->GetContent() != aElement) { - MOZ_ASSERT(static_cast(do_QueryFrame(styleFrame))); + MOZ_ASSERT(styleFrame->IsImageFrameOrSubclass()); styleFrame = nullptr; } diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp index 9cf2a3cbc9a9..b93a7ea73407 100644 --- a/layout/base/nsCSSFrameConstructor.cpp +++ b/layout/base/nsCSSFrameConstructor.cpp @@ -3515,7 +3515,9 @@ nsCSSFrameConstructor::FindGeneratedImageData(const Element& aElement, const nsCSSFrameConstructor::FrameConstructionData* nsCSSFrameConstructor::FindImgData(const Element& aElement, ComputedStyle& aStyle) { - if (!nsImageFrame::ShouldCreateImageFrameFor(aElement, aStyle)) { + if (nsImageFrame::ImageFrameTypeFor(aElement, aStyle) != + nsImageFrame::ImageFrameType::ForElementRequest) { + // content: url gets handled by the generic code-path. return nullptr; } @@ -3527,7 +3529,8 @@ nsCSSFrameConstructor::FindImgData(const Element& aElement, const nsCSSFrameConstructor::FrameConstructionData* nsCSSFrameConstructor::FindImgControlData(const Element& aElement, ComputedStyle& aStyle) { - if (!nsImageFrame::ShouldCreateImageFrameFor(aElement, aStyle)) { + if (nsImageFrame::ImageFrameTypeFor(aElement, aStyle) != + nsImageFrame::ImageFrameType::ForElementRequest) { return nullptr; } @@ -5356,7 +5359,8 @@ nsCSSFrameConstructor::FindElementData(const Element& aElement, // Check for 'content: ' on the element (which makes us ignore // 'display' values other than 'none' or 'contents'). - if (nsImageFrame::ShouldCreateImageFrameForContent(aElement, aStyle)) { + if (nsImageFrame::ShouldCreateImageFrameForContentProperty(aElement, + aStyle)) { static constexpr FrameConstructionData sImgData( NS_NewImageFrameForContentProperty); return &sImgData; diff --git a/layout/generic/nsIFrame.cpp b/layout/generic/nsIFrame.cpp index 246f1416b0b3..c07d7671fa09 100644 --- a/layout/generic/nsIFrame.cpp +++ b/layout/generic/nsIFrame.cpp @@ -7852,6 +7852,11 @@ bool nsIFrame::IsBlockFrameOrSubclass() const { return !!thisAsBlock; } +bool nsIFrame::IsImageFrameOrSubclass() const { + const nsImageFrame* asImage = do_QueryFrame(this); + return !!asImage; +} + static nsIFrame* GetNearestBlockContainer(nsIFrame* frame) { // The block wrappers we use to wrap blocks inside inlines aren't // described in the CSS spec. We need to make them not be containing diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h index d132336ed2dd..016b291f4644 100644 --- a/layout/generic/nsIFrame.h +++ b/layout/generic/nsIFrame.h @@ -3325,6 +3325,12 @@ class nsIFrame : public nsQueryFrame { */ bool IsBlockFrameOrSubclass() const; + /** + * Returns true if the frame is an instance of nsImageFrame or one of its + * subclasses. + */ + bool IsImageFrameOrSubclass() const; + /** * Returns true if the frame is an instance of SVGGeometryFrame or one * of its subclasses. diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp index 8dcdb60d17b8..53a77b21ebf0 100644 --- a/layout/generic/nsImageFrame.cpp +++ b/layout/generic/nsImageFrame.cpp @@ -188,8 +188,8 @@ bool nsDisplayGradient::CreateWebRenderCommands( StaticRefPtr nsImageFrame::gIconLoad; // test if the width and height are fixed, looking at the style data -// This is used by nsImageFrame::ShouldCreateImageFrameFor and should -// not be used for layout decisions. +// This is used by nsImageFrame::ImageFrameTypeFor and should not be used for +// layout decisions. static bool HaveSpecifiedSize(const nsStylePosition* aStylePosition) { // check the width and height values in the reflow input's style struct // - if width and height are specified as either coord or percentage, then @@ -834,7 +834,7 @@ static bool HasAltText(const Element& aElement) { return aElement.HasNonEmptyAttr(nsGkAtoms::alt); } -bool nsImageFrame::ShouldCreateImageFrameForContent( +bool nsImageFrame::ShouldCreateImageFrameForContentProperty( const Element& aElement, const ComputedStyle& aStyle) { if (aElement.IsRootOfNativeAnonymousSubtree()) { return false; @@ -848,39 +848,42 @@ bool nsImageFrame::ShouldCreateImageFrameForContent( } // Check if we want to use an image frame or just let the frame constructor make -// us into an inline. +// us into an inline, and if so, which kind of image frame should we create. /* static */ -bool nsImageFrame::ShouldCreateImageFrameFor(const Element& aElement, - const ComputedStyle& aStyle) { - if (ShouldCreateImageFrameForContent(aElement, aStyle)) { +auto nsImageFrame::ImageFrameTypeFor(const Element& aElement, + const ComputedStyle& aStyle) + -> ImageFrameType { + if (ShouldCreateImageFrameForContentProperty(aElement, aStyle)) { // Prefer the content property, for compat reasons, see bug 1484928. - return false; + return ImageFrameType::ForContentProperty; } if (ImageOk(aElement.State())) { // Image is fine or loading; do the image frame thing - return true; + return ImageFrameType::ForElementRequest; } if (aStyle.StyleUIReset()->mMozForceBrokenImageIcon) { - return true; + return ImageFrameType::ForElementRequest; } // if our "do not show placeholders" pref is set, skip the icon if (gIconLoad && gIconLoad->mPrefForceInlineAltText) { - return false; + return ImageFrameType::None; } if (!HasAltText(aElement)) { - return true; + return ImageFrameType::ForElementRequest; } - if (aElement.OwnerDoc()->GetCompatibilityMode() == eCompatibility_NavQuirks) { - // FIXME(emilio): We definitely don't reframe when this changes... - return HaveSpecifiedSize(aStyle.StylePosition()); + // FIXME(emilio, bug 1788767): We definitely don't reframe when + // HaveSpecifiedSize changes... + if (aElement.OwnerDoc()->GetCompatibilityMode() == eCompatibility_NavQuirks && + HaveSpecifiedSize(aStyle.StylePosition())) { + return ImageFrameType::ForElementRequest; } - return false; + return ImageFrameType::None; } void nsImageFrame::Notify(imgIRequest* aRequest, int32_t aType, @@ -936,7 +939,6 @@ void nsImageFrame::OnSizeAvailable(imgIRequest* aRequest, } 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. @@ -954,19 +956,8 @@ void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) { return; } } - bool intrinsicSizeOrRatioChanged = [&] { - // NOTE(emilio): We intentionally want to call both functions and avoid - // short-circuiting. - bool intrinsicSizeChanged = UpdateIntrinsicSize(); - bool intrinsicRatioChanged = UpdateIntrinsicRatio(); - return intrinsicSizeChanged || intrinsicRatioChanged; - }(); - if (intrinsicSizeOrRatioChanged) { - // Our aspect-ratio property value changed, and an embedding or - // might care about that. - MaybeSendIntrinsicSizeAndRatioToEmbedder(); - } + UpdateIntrinsicSizeAndRatio(); if (!GotInitialReflow()) { return; @@ -974,19 +965,6 @@ void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) { // We're going to need to repaint now either way. InvalidateFrame(); - - if (intrinsicSizeOrRatioChanged) { - // Now we need to reflow if we have an unconstrained size and have - // already gotten the initial reflow. - if (!(mState & IMAGE_SIZECONSTRAINED)) { - PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange, - NS_FRAME_IS_DIRTY); - } else if (PresShell()->IsActive()) { - // We've already gotten the initial reflow, and our size hasn't changed, - // so we're ready to request a decode. - MaybeDecodeForPredictedSize(); - } - } } void nsImageFrame::OnFrameUpdate(imgIRequest* aRequest, @@ -1085,16 +1063,32 @@ void nsImageFrame::OnLoadComplete(imgIRequest* aRequest, nsresult aStatus) { NotifyNewCurrentRequest(aRequest, aStatus); } +void nsImageFrame::ElementStateChanged(ElementState aStates) { + if (!(aStates & ElementState::BROKEN)) { + return; + } + if (mKind != Kind::ImageElement) { + return; + } + if (!ImageOk(mContent->AsElement()->State())) { + UpdateImage(nullptr, nullptr); + } +} + void nsImageFrame::ResponsiveContentDensityChanged() { - if (!GotInitialReflow()) { - return; - } + UpdateIntrinsicSizeAndRatio(); +} - if (!mImage) { - return; - } +void nsImageFrame::UpdateIntrinsicSizeAndRatio() { + bool intrinsicSizeOrRatioChanged = [&] { + // NOTE(emilio): We intentionally want to call both functions and avoid + // short-circuiting. + bool intrinsicSizeChanged = UpdateIntrinsicSize(); + bool intrinsicRatioChanged = UpdateIntrinsicRatio(); + return intrinsicSizeChanged || intrinsicRatioChanged; + }(); - if (!UpdateIntrinsicSize() && !UpdateIntrinsicRatio()) { + if (!intrinsicSizeOrRatioChanged) { return; } @@ -1102,8 +1096,20 @@ void nsImageFrame::ResponsiveContentDensityChanged() { // might care about that. MaybeSendIntrinsicSizeAndRatioToEmbedder(); - PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange, - NS_FRAME_IS_DIRTY); + if (!GotInitialReflow()) { + return; + } + + // Now we need to reflow if we have an unconstrained size and have + // already gotten the initial reflow. + if (!HasAnyStateBits(IMAGE_SIZECONSTRAINED)) { + PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange, + NS_FRAME_IS_DIRTY); + } else if (PresShell()->IsActive()) { + // We've already gotten the initial reflow, and our size hasn't changed, + // so we're ready to request a decode. + MaybeDecodeForPredictedSize(); + } } void nsImageFrame::NotifyNewCurrentRequest(imgIRequest* aRequest, diff --git a/layout/generic/nsImageFrame.h b/layout/generic/nsImageFrame.h index 8e5aa38b11a2..0534ffc90c11 100644 --- a/layout/generic/nsImageFrame.h +++ b/layout/generic/nsImageFrame.h @@ -99,6 +99,7 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback { const Maybe& aNonvisibleAction = Nothing()) final; void ResponsiveContentDensityChanged(); + void ElementStateChanged(mozilla::dom::ElementState) override; void SetupForContentURLRequest(); bool ShouldShowBrokenImageIcon() const; @@ -140,16 +141,21 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback { * Returns whether we should replace an element with an image corresponding to * its 'content' CSS property. */ - static bool ShouldCreateImageFrameForContent(const mozilla::dom::Element&, - const ComputedStyle&); + static bool ShouldCreateImageFrameForContentProperty( + const mozilla::dom::Element&, const ComputedStyle&); /** * Function to test whether given an element and its style, that element - * should get an image frame. Note that this method is only used by the - * frame constructor; it's only here because it uses gIconLoad for now. + * should get an image frame, and if so, which kind of image frame (for + * `content`, or for the element itself). */ - static bool ShouldCreateImageFrameFor(const mozilla::dom::Element&, - const ComputedStyle&); + enum class ImageFrameType { + ForContentProperty, + ForElementRequest, + None, + }; + static ImageFrameType ImageFrameTypeFor(const mozilla::dom::Element&, + const ComputedStyle&); ImgDrawResult DisplayAltFeedback(gfxContext& aRenderingContext, const nsRect& aDirtyRect, nsPoint aPt, @@ -218,6 +224,8 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback { void ReflowChildren(nsPresContext*, const ReflowInput&, const mozilla::LogicalSize& aImageSize); + void UpdateIntrinsicSizeAndRatio(); + protected: nsImageFrame(ComputedStyle* aStyle, nsPresContext* aPresContext, ClassID aID) : nsImageFrame(aStyle, aPresContext, aID, Kind::ImageElement) {} @@ -354,8 +362,8 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback { bool IsPendingLoad(imgIRequest*) 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. + * Updates mImage based on the current image request, and the image passed in + * (both can be null), and invalidate layout and paint as needed. */ void UpdateImage(imgIRequest*, imgIContainer*); diff --git a/layout/style/res/html.css b/layout/style/res/html.css index de03a75f7fab..0d2b175a297f 100644 --- a/layout/style/res/html.css +++ b/layout/style/res/html.css @@ -647,8 +647,12 @@ hr[size="1"] { border-style: solid none none none; } -input:-moz-broken::before, -img:-moz-broken::before { +/* Note that we only intend for the alt content to show up if the image is + * broken. But non-broken images/inputs will have a replaced box, and thus we + * won't we don't generate the pseudo-element anyways. This prevents + * unnecessary reframing when images become broken / non-broken. */ +input[type=image]::before, +img::before { content: -moz-alt-content !important; unicode-bidi: isolate; } diff --git a/layout/style/test/mochitest.ini b/layout/style/test/mochitest.ini index b13e88c929b6..6eba68d67a21 100644 --- a/layout/style/test/mochitest.ini +++ b/layout/style/test/mochitest.ini @@ -267,6 +267,7 @@ skip-if = verify || toolkit == 'android' # Bug 1455824 [test_hover_on_part.html] [test_html_attribute_computed_values.html] [test_ident_escaping.html] +[test_img_src_causing_reflow.html] [test_import_preload.html] support-files = slow_load.sjs # Test is slightly racy and on Android it fails frequently enough to be diff --git a/layout/style/test/test_img_src_causing_reflow.html b/layout/style/test/test_img_src_causing_reflow.html new file mode 100644 index 000000000000..e63b39f9fefa --- /dev/null +++ b/layout/style/test/test_img_src_causing_reflow.html @@ -0,0 +1,36 @@ + + +Test for bug 1787072 + + + +