Use a non-replaced inline container for image alt text

third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp

Index: third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp b/third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
index cbf51fab17b89ccfaa1729635cf7c3415b19a1bc..e024aa190c18db57aa0b1c9fca924c9783d559e3 100644
--- a/third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
@@ -9,11 +9,11 @@
 #include "core/dom/ElementRareData.h"
 #include "core/dom/Text.h"
 #include "core/dom/shadow/ShadowRoot.h"
-#include "core/html/HTMLDivElement.h"
 #include "core/html/HTMLElement.h"
 #include "core/html/HTMLImageElement.h"
 #include "core/html/HTMLImageLoader.h"
 #include "core/html/HTMLInputElement.h"
+#include "core/html/HTMLSpanElement.h"
 #include "core/html/HTMLStyleElement.h"
 #include "wtf/text/StringBuilder.h"
 
@@ -31,21 +31,20 @@ static bool noImageSourceSpecified(const Element& element) {
   return noSrcSpecified && noSrcsetSpecified;
 }
 
+static bool imageSmallerThanAltImage(const Length width, const Length height) {

[pdr., 2017/03/14 18:19:27]

Can you add a link to the spec text that defines 16px? Also, does this handle
the relative-sized em stuff in the spec text?

+  if (!width.isFixed() && !height.isFixed())
+    return false;
+  if (height.isFixed())
+    return height.value() < 16;
+  return width.value() < 16;
+}
+
 void HTMLImageFallbackHelper::createAltTextShadowTree(Element& element) {
   ShadowRoot& root = element.ensureUserAgentShadowRoot();
 
-  HTMLDivElement* container = HTMLDivElement::create(element.document());
+  HTMLSpanElement* container = HTMLSpanElement::create(element.document());
   root.appendChild(container);
   container->setAttribute(idAttr, AtomicString("alttext-container"));
-  container->setInlineStyleProperty(CSSPropertyOverflow, CSSValueHidden);
-  container->setInlineStyleProperty(CSSPropertyBorderWidth, 1,

[pdr., 2017/03/14 18:23:27]

Does this change match firefox--I still see a border on their missing images?
Most of the expectation changes seem to be due to losing this border. Similar to
my other question, can we defer this to a followup patch?

-                                    CSSPrimitiveValue::UnitType::Pixels);
-  container->setInlineStyleProperty(CSSPropertyBorderStyle, CSSValueSolid);
-  container->setInlineStyleProperty(CSSPropertyBorderColor, CSSValueSilver);
-  container->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInlineBlock);
-  container->setInlineStyleProperty(CSSPropertyBoxSizing, CSSValueBorderBox);
-  container->setInlineStyleProperty(CSSPropertyPadding, 1,
-                                    CSSPrimitiveValue::UnitType::Pixels);
 
   HTMLImageElement* brokenImage = HTMLImageElement::create(element.document());
   container->appendChild(brokenImage);
@@ -57,11 +56,9 @@ void HTMLImageFallbackHelper::createAltTextShadowTree(Element& element) {
   brokenImage->setInlineStyleProperty(CSSPropertyMargin, 0,
                                       CSSPrimitiveValue::UnitType::Pixels);
 
-  HTMLDivElement* altText = HTMLDivElement::create(element.document());
+  HTMLSpanElement* altText = HTMLSpanElement::create(element.document());
   container->appendChild(altText);
   altText->setAttribute(idAttr, AtomicString("alttext"));
-  altText->setInlineStyleProperty(CSSPropertyOverflow, CSSValueHidden);
-  altText->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock);
 
   Text* text =
       Text::create(element.document(), toHTMLElement(element).altText());
@@ -102,14 +99,43 @@ PassRefPtr<ComputedStyle> HTMLImageFallbackHelper::customStyleForAltText(
     }
   }
 
-  // If the image has specified dimensions allow the alt-text container expand
-  // to fill them.
-  if (newStyle->width().isSpecifiedOrIntrinsic() &&
-      newStyle->height().isSpecifiedOrIntrinsic()) {
-    placeHolder->setInlineStyleProperty(
-        CSSPropertyWidth, 100, CSSPrimitiveValue::UnitType::Percentage);
-    placeHolder->setInlineStyleProperty(
-        CSSPropertyHeight, 100, CSSPrimitiveValue::UnitType::Percentage);
+  bool imageHasIntrinsicDimensions =
+      newStyle->width().isSpecifiedOrIntrinsic() &&
+      newStyle->height().isSpecifiedOrIntrinsic();
+  bool imageHasNoAltAttribute = toHTMLElement(element).altText().isNull();
+  bool imageHasEmptyAltAttribute =
+      !imageHasNoAltAttribute && toHTMLElement(element).altText().isEmpty();
+  bool imageHasSrcAttribute = !noImageSourceSpecified(element);
+
+  // https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
+  enum ElementRepresents { ElementRepresentsNothing, ElementRepresentsText };
+  ElementRepresents elementRepresents = ElementRepresentsText;
+  if (imageHasSrcAttribute && imageHasEmptyAltAttribute) {
+    // First case: "If the src attribute is set and the alt attribute is set to
+    // the empty string ... the element represents nothing ..."
+    elementRepresents = ElementRepresentsNothing;
+  } else if (!imageHasSrcAttribute &&
+             (imageHasNoAltAttribute || imageHasEmptyAltAttribute)) {
+    // Fourth case: "If the src attribute is not set and either the alt
+    // attribute is set to the empty string or the alt attribute is not set at
+    // all ... The element represents nothing."
+    elementRepresents = ElementRepresentsNothing;
+  }
+
+  // https://html.spec.whatwg.org/multipage/rendering.html#images-3:
+  // "If the element does not represent an image, but the element already has
+  // intrinsic dimensions (e.g. from the dimension attributes or CSS rules), and
+  // either: the user agent has reason to believe that the image will become
+  // available and be rendered in due course, or the element has no alt
+  // attribute, or the Document is in quirks mode The user agent is expected to
+  // treat the element as a replaced element whose content is the text that the
+  // element represents, if any."
+  bool treatAsReplaced =
+      imageHasIntrinsicDimensions &&
+      (element.document().inQuirksMode() || imageHasNoAltAttribute);
+  if (!treatAsReplaced && newStyle->display() == EDisplay::Inline) {
+    newStyle->setWidth(Length());
+    newStyle->setHeight(Length());
   }
 
   // Make sure the broken image icon appears on the appropriate side of the
@@ -119,19 +145,22 @@ PassRefPtr<ComputedStyle> HTMLImageFallbackHelper::customStyleForAltText(
       AtomicString(newStyle->direction() == TextDirection::kLtr ? "left"
                                                                 : "right"));
 
-  // This is an <img> with no attributes, so don't display anything.
-  if (noImageSourceSpecified(element) &&
-      !newStyle->width().isSpecifiedOrIntrinsic() &&
-      !newStyle->height().isSpecifiedOrIntrinsic() &&
-      toHTMLElement(element).altText().isEmpty())
-    newStyle->setDisplay(EDisplay::None);
-
-  // This preserves legacy behaviour originally defined when alt-text was
-  // managed by LayoutImage.
-  if (noImageSourceSpecified(element))
+  if (elementRepresents == ElementRepresentsNothing ||
+      imageSmallerThanAltImage(newStyle->width(), newStyle->height())) {
+    // "If the element is an img element that represents nothing and the user

[pdr., 2017/03/14 18:19:27]

This seems like a significant change. I think this is reasonable, but can it be
deferred to a followup patch or does it need to be in this one?

[rhogan, 2017/03/23 12:24:44]

Do you mean not displaying the alt image icon if the image is smaller than it?
I'd prefer to do that now, it's one of the things that prompted me to revisit
this - we get bugs from people complaining about 1x1px little squares where they
expect to see nothing at all.

+    // agent does not expect this to change the user agent is expected to treat
+    // the element as an empty inline element."
+    // We achieve this by hiding the broken image so that the span is empty.
     brokenImage->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
-  else
+  } else {
+    // "If the element is an img element that represents some text and the user
+    // agent does not expect this to change the user agent is expected to treat
+    // the element as a non-replaced phrasing element whose content is the text,
+    // optionally with an icon indicating that an image is missing, so that the
+    // user can request the image be displayed or investigate why it is not
+    // rendering."
     brokenImage->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInline);
+  }
 
   return newStyle;
 }