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 bc2f7718bf9fb6816e763795da395201f5ea9294..b34507f2e16637512119ee6fbe18f281194a1c9f 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"
 
@@ -21,31 +21,24 @@ namespace blink {
 
 using namespace HTMLNames;
 
-static bool noImageSourceSpecified(const Element& element) {
-  bool noSrcSpecified = !element.hasAttribute(srcAttr) ||
-                        element.getAttribute(srcAttr).isNull() ||
-                        element.getAttribute(srcAttr).isEmpty();
-  bool noSrcsetSpecified = !element.hasAttribute(srcsetAttr) ||
-                           element.getAttribute(srcsetAttr).isNull() ||
-                           element.getAttribute(srcsetAttr).isEmpty();
-  return noSrcSpecified && noSrcsetSpecified;
+static bool imageSmallerThanAltImage(int pixelsForAltImage,
+                                     const Length width,
+                                     const Length height) {
+  // We don't have a layout tree so can't compute the size of an image
+  // relative dimensions - so we just assume we should display the alt image.
+  if (!width.isFixed() && !height.isFixed())
+    return false;
+  if (height.isFixed())
+    return height.value() < pixelsForAltImage;
+  return width.value() < pixelsForAltImage;
 }
 
 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,
-                                    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 +50,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);

[pdr., 2017/03/24 02:12:50]

The change to LayoutTests/tables/mozilla_expected_failures/bugs/bug23847.html
looks like a small regression because the broken image now sticks out. Could
that be due to losing this overflow: hidden?

-  altText->setInlineStyleProperty(CSSPropertyDisplay, CSSValueBlock);
 
   Text* text =
       Text::create(element.document(), toHTMLElement(element).altText());
@@ -102,36 +93,68 @@ 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);
-  }
-
-  // Make sure the broken image icon appears on the appropriate side of the
-  // image for the element's writing direction.
-  brokenImage->setInlineStyleProperty(
-      CSSPropertyFloat,
-      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::kNone);
-
-  // This preserves legacy behaviour originally defined when alt-text was
-  // managed by LayoutImage.
-  if (noImageSourceSpecified(element))
-    brokenImage->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
-  else
+  bool imageHasIntrinsicDimensions =
+      newStyle->width().isSpecifiedOrIntrinsic() &&
+      newStyle->height().isSpecifiedOrIntrinsic();
+  bool imageHasNoAltAttribute = toHTMLElement(element).altText().isNull();
+  bool treatAsReplaced =
+      imageHasIntrinsicDimensions &&
+      (element.document().inQuirksMode() || imageHasNoAltAttribute);
+  // 16px for the image and 2px for its border/padding offset.
+  int pixelsForAltImage = 18;

[pdr., 2017/03/24 02:15:45]

Should this be 1 + 1 + 16 + 1 + 1 = 20 to account for the border/padding on the
left and right sides?

+  if (treatAsReplaced &&
+      !imageSmallerThanAltImage(pixelsForAltImage, newStyle->width(),
+                                newStyle->height())) {
+    // 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."
+    placeHolder->setInlineStyleProperty(CSSPropertyDisplay,
+                                        CSSValueInlineBlock);
+    placeHolder->setInlineStyleProperty(CSSPropertyBorderWidth, 1,
+                                        CSSPrimitiveValue::UnitType::Pixels);
+    placeHolder->setInlineStyleProperty(CSSPropertyBorderStyle, CSSValueSolid);
+    placeHolder->setInlineStyleProperty(CSSPropertyBorderColor, CSSValueSilver);
+    placeHolder->setInlineStyleProperty(CSSPropertyPadding, 1,
+                                        CSSPrimitiveValue::UnitType::Pixels);
+    placeHolder->setInlineStyleProperty(CSSPropertyBoxSizing,
+                                        CSSValueBorderBox);
+    CSSPrimitiveValue::UnitType unit =
+        newStyle->height().isPercent() ? CSSPrimitiveValue::UnitType::Percentage
+                                       : CSSPrimitiveValue::UnitType::Pixels;
+    placeHolder->setInlineStyleProperty(CSSPropertyHeight,
+                                        newStyle->height().value(), unit);
+    placeHolder->setInlineStyleProperty(CSSPropertyWidth,
+                                        newStyle->width().value(), unit);
     brokenImage->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInline);
+    // Make sure the broken image icon appears on the appropriate side of the
+    // image for the element's writing direction.
+    brokenImage->setInlineStyleProperty(
+        CSSPropertyFloat,
+        AtomicString(newStyle->direction() == TextDirection::kLtr ? "left"
+                                                                  : "right"));
+  } else {
+    // "If the element is an img element that represents nothing and the user
+    // 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.
+    // "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."
+    //  - We opt not to display an icon, like Firefox.
+    if (!treatAsReplaced && newStyle->display() == EDisplay::kInline) {
+      newStyle->setWidth(Length());
+      newStyle->setHeight(Length());

[pdr., 2017/03/24 02:12:49]

In tables/mozilla/bugs/bug56201.html it looks like we draw a 0px by 16px box. I
wonder if there's a bug here?

+    }
+    brokenImage->setInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);
+  }
 
   return newStyle;
 }