Bug 1472637 - Don't display alt text while loading, to match other UAs. r=tnikkel
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 18 Mar 2019 15:15:13 +0000
changeset 523637 94682e23ba118dcfab3843a3280618b2e1686ecb
parent 523636 50fb3808da519a54d90da9fae63337b1e39c58e9
child 523638 3f1e9dae2467e54c783215189e6cb9617cee7dcd
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1472637
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1472637 - Don't display alt text while loading, to match other UAs. r=tnikkel Differential Revision: https://phabricator.services.mozilla.com/D18518
layout/base/RestyleManager.cpp
layout/generic/nsImageFrame.cpp
layout/style/res/html.css
testing/web-platform/tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-replaced-box-while-loading.html
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -471,16 +471,18 @@ static nsChangeHint ChangeForContentStat
   // need to force a reframe -- if it's needed, the HasStateDependentStyle
   // call will handle things.
   if (nsIFrame* primaryFrame = aElement.GetPrimaryFrame()) {
     // If it's generated content, ignore LOADING/etc state changes on it.
     if (!primaryFrame->IsGeneratedContentFrame() &&
         aStateMask.HasAtLeastOneOfStates(
             NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED |
             NS_EVENT_STATE_SUPPRESSED | NS_EVENT_STATE_LOADING)) {
+      // FIXME(emilio, bug 1395964): For <img> elements at least, we shouldn't
+      // reframe when LOADING changes anymore.
       return nsChangeHint_ReconstructFrame;
     }
 
     auto* disp = primaryFrame->StyleDisplay();
     if (disp->HasAppearance()) {
       nsPresContext* pc = primaryFrame->PresContext();
       nsITheme* theme = pc->GetTheme();
       if (theme &&
--- a/layout/generic/nsImageFrame.cpp
+++ b/layout/generic/nsImageFrame.cpp
@@ -549,26 +549,21 @@ nsRect nsImageFrame::SourceRectToDest(co
 
   return r;
 }
 
 // Note that we treat NS_EVENT_STATE_SUPPRESSED images as "OK".  This means
 // that we'll construct image frames for them as needed if their display is
 // toggled from "none" (though we won't paint them, unless their visibility
 // is changed too).
-#define BAD_STATES \
-  (NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED | NS_EVENT_STATE_LOADING)
-
-// This is a macro so that we don't evaluate the boolean last arg
-// unless we have to; it can be expensive
-#define IMAGE_OK(_state, _loadingOK)                                \
-  (!(_state).HasAtLeastOneOfStates(BAD_STATES) ||                   \
-   (!(_state).HasAtLeastOneOfStates(NS_EVENT_STATE_BROKEN |         \
-                                    NS_EVENT_STATE_USERDISABLED) && \
-    (_state).HasState(NS_EVENT_STATE_LOADING) && (_loadingOK)))
+#define BAD_STATES (NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED)
+
+static bool ImageOk(EventStates aState) {
+  return !aState.HasAtLeastOneOfStates(BAD_STATES);
+}
 
 static bool HasAltText(const Element& aElement) {
   // We always return some alternate text for <input>, see
   // nsCSSFrameConstructor::GetAlternateTextFor.
   if (aElement.IsHTMLElement(nsGkAtoms::input)) {
     return true;
   }
 
@@ -577,19 +572,18 @@ static bool HasAltText(const Element& aE
   return aElement.GetAttr(nsGkAtoms::alt, altText) && !altText.IsEmpty();
 }
 
 // Check if we want to use an image frame or just let the frame constructor make
 // us into an inline.
 /* static */
 bool nsImageFrame::ShouldCreateImageFrameFor(const Element& aElement,
                                              ComputedStyle& aStyle) {
-  EventStates state = aElement.State();
-  if (IMAGE_OK(state, HaveSpecifiedSize(aStyle.StylePosition()))) {
-    // Image is fine; do the image frame thing
+  if (ImageOk(aElement.State())) {
+    // Image is fine or loading; do the image frame thing
     return true;
   }
 
   if (aStyle.StyleUIReset()->mForceBrokenImageIcon) {
     return true;
   }
 
   // if our "do not show placeholders" pref is set, skip the icon
@@ -1033,18 +1027,17 @@ void nsImageFrame::Reflow(nsPresContext*
     // our desired height was greater than 0, so to avoid infinite
     // splitting, use 1 pixel as the min
     aMetrics.Height() = std::max(nsPresContext::CSSPixelsToAppUnits(1),
                                  aReflowInput.AvailableHeight());
     aStatus.SetIncomplete();
   }
 
   aMetrics.SetOverflowAreasToDesiredBounds();
-  EventStates contentState = mContent->AsElement()->State();
-  bool imageOK = IMAGE_OK(contentState, true);
+  bool imageOK = ImageOk(mContent->AsElement()->State());
 
   // Determine if the size is available
   bool haveSize = false;
   if (loadStatus & imgIRequest::STATUS_SIZE_AVAILABLE) {
     haveSize = true;
   }
 
   if (!imageOK || !haveSize) {
@@ -1345,17 +1338,17 @@ class nsDisplayAltFeedback final : publi
 
 ImgDrawResult nsImageFrame::DisplayAltFeedback(gfxContext& aRenderingContext,
                                                const nsRect& aDirtyRect,
                                                nsPoint aPt, uint32_t aFlags) {
   // We should definitely have a gIconLoad here.
   MOZ_ASSERT(gIconLoad, "How did we succeed in Init then?");
 
   // Whether we draw the broken or loading icon.
-  bool isLoading = IMAGE_OK(GetContent()->AsElement()->State(), true);
+  bool isLoading = ImageOk(mContent->AsElement()->State());
 
   // Calculate the inner area
   nsRect inner = GetInnerArea() + aPt;
 
   // Display a recessed one pixel border
   nscoord borderEdgeWidth =
       nsPresContext::CSSPixelsToAppUnits(ALT_BORDER_WIDTH);
 
@@ -1502,17 +1495,17 @@ ImgDrawResult nsImageFrame::DisplayAltFe
     mozilla::wr::IpcResourceUpdateQueue& aResources,
     const StackingContextHelper& aSc,
     mozilla::layers::RenderRootStateManager* aManager,
     nsDisplayListBuilder* aDisplayListBuilder, nsPoint aPt, uint32_t aFlags) {
   // We should definitely have a gIconLoad here.
   MOZ_ASSERT(gIconLoad, "How did we succeed in Init then?");
 
   // Whether we draw the broken or loading icon.
-  bool isLoading = IMAGE_OK(GetContent()->AsElement()->State(), true);
+  bool isLoading = ImageOk(mContent->AsElement()->State());
 
   // Calculate the inner area
   nsRect inner = GetInnerArea() + aPt;
 
   // Display a recessed one pixel border
   nscoord borderEdgeWidth =
       nsPresContext::CSSPixelsToAppUnits(ALT_BORDER_WIDTH);
 
@@ -2054,18 +2047,17 @@ void nsImageFrame::BuildDisplayList(nsDi
       nsStyleUtil::ObjectPropsMightCauseOverflow(StylePosition())
           ? 0
           : DisplayListClipState::ASSUME_DRAWING_RESTRICTED_TO_CONTENT_RECT;
 
   DisplayListClipState::AutoClipContainingBlockDescendantsToContentBox clip(
       aBuilder, this, clipFlags);
 
   if (mComputedSize.width != 0 && mComputedSize.height != 0) {
-    EventStates contentState = mContent->AsElement()->State();
-    bool imageOK = IMAGE_OK(contentState, true);
+    bool imageOK = ImageOk(mContent->AsElement()->State());
 
     nsCOMPtr<imgIRequest> currentRequest = GetCurrentRequest();
 
     // XXX(seth): The SizeIsAvailable check here should not be necessary - the
     // intention is that a non-null mImage means we have a size, but there is
     // currently some code that violates this invariant.
     if (!imageOK || !mImage || !SizeIsAvailable(currentRequest)) {
       // No image yet, or image load failed. Draw the alt-text and an icon
--- a/layout/style/res/html.css
+++ b/layout/style/res/html.css
@@ -634,18 +634,17 @@ hr {
   box-sizing: content-box;
 }
 
 hr[size="1"] {
   border-style: solid none none none;
 }
 
 img:-moz-broken::before, input:-moz-broken::before,
-img:-moz-user-disabled::before, input:-moz-user-disabled::before,
-img:-moz-loading::before, input:-moz-loading::before {
+img:-moz-user-disabled::before, input:-moz-user-disabled::before {
   content: -moz-alt-content !important;
   unicode-bidi: isolate;
 }
 
 object:-moz-any(:-moz-broken,:-moz-user-disabled) > *|* {
   /*
     Inherit in the object's alignment so that if we aren't aligned explicitly
     we'll end up in the right place vertically.  See bug 36997.  Note that this
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-replaced-box-while-loading.html
@@ -0,0 +1,29 @@
+<!doctype html>
+<title>Images don't render as a non-replaced inline while loading, even when there's no concrete size specified</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">
+<link rel="author" href="https://mozilla.org" title="Mozilla">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1472637">
+<style>
+  img {
+    min-width: 1000px;
+  }
+</style>
+<img alt="T">
+<script>
+// Do an async test off the onload handler to avoid waiting for the load even for too long unnecessarily.
+let t = async_test("Loading images should get a replaced box, even without specified size");
+onload = t.step_func(function() {
+  const image = document.querySelector("img");
+  // Use the trickle pipe to have 100 seconds until the image actually loads,
+  // that should be enough to run the test.
+  image.src = "../../../../../images/blue.png?pipe=trickle(d100)";
+  t.step_timeout(t.step_func_done(function() {
+    assert_equals(
+      image.offsetWidth,
+      1000,
+    );
+  }), 0);
+});
+</script>