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..83c3e105d4400866f418e9f2c1a129d7ff0c905a 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) {
+  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,
-                                    CSSPrimitiveValue::UnitType::Pixels);
-  container->setInlineStyleProperty(CSSPropertyBorderStyle, CSSValueSolid);
-  container->setInlineStyleProperty(CSSPropertyBorderColor, CSSValueSilver);
-  container->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInlineBlock);

[pdr., 2017/03/02 21:51:11]

I looked at
https://chromium.googlesource.com/chromium/src/+/4d2d3e02c9c3869349c4616dea88...
and http://hixie.ch/specs/alttext... do you recall why an inline block was
picked over an inline originally?

[rhogan, 2017/03/02 23:44:46]

Because it gave the nearest rendering to the way we were doing it at the time
using raw painting, but with the benefit that text would wrap etc.

-  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,44 @@ 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 imageHasNoSrcAttribute = noImageSourceSpecified(element);
+
+  // https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
+  enum ElementRepresents { ElementRepresentsNothing, ElementRepresentsText };
+  ElementRepresents elementRepresents;
+  if (!imageHasNoSrcAttribute && imageHasEmptyAltAttribute)

[pdr., 2017/03/02 21:51:11]

This is hard to follow. I think this if-else chain can be simplified to just
test the ElementRepresentsText cases and return nothing otherwise (or,
alternatively, just the nothing cases and text otherwise).

[rhogan, 2017/03/02 23:44:46]

Agreed. In its favour though it implements the logic in the order/style that's
set out at
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element.
Maybe adding the spec text to each branch in the clause would help - because I
agree it makes for pretty close reading.

[pdr., 2017/03/03 00:52:06]

Ahh, I see what you mean. This does nicely follow the spec but I wonder if we
could just pull out the nothing cases to make it simpler?

Something like:
ElementRepresents elementRepresents = ElementRepresentsText;
// Determine whether element represents text based on the cases in
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element.
if (!imageHasNoSrcAttribute && 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 (imageHasNoSrcAttribute &&
         (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;
}

+    elementRepresents = ElementRepresentsNothing;
+  else if (!imageHasNoSrcAttribute && !imageHasEmptyAltAttribute &&
+           !imageHasNoAltAttribute)
+    elementRepresents = ElementRepresentsText;
+  else if (!imageHasNoSrcAttribute && imageHasNoAltAttribute)
+    elementRepresents = ElementRepresentsText;

[pdr., 2017/03/02 21:51:11]

Can this ever be hit since imageHasEmptyAltAttribute = !imageHasNoAltAttribute?

[rhogan, 2017/03/02 23:44:46]

Yes, I think so: where <img src="test.jpg">. The previous two are for <img
src="test.jpg" alt=""> and <img src="test.jpg" alt="text> respectively.

!imageHasNoSrcAttribute is not very readable is it. I'll change that since I use
it so much.

[pdr., 2017/03/03 00:52:06]

Yeah I think it might be a little easier to read using the positive versions
like "imageHasSrcAttribute" and "imageHasAltAttribute"

+  else if (imageHasNoSrcAttribute &&
+           (imageHasNoAltAttribute || imageHasEmptyAltAttribute))
+    elementRepresents = ElementRepresentsNothing;
+  else
+    elementRepresents = ElementRepresentsText;
+
+  // 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 +146,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())) {

[pdr., 2017/03/02 21:51:11]

Can this imageSmallerThanAltImage logic be combined into one of the bools in the
if-else chain above so we don't need more conditionals here?

[rhogan, 2017/03/02 23:44:46]

Sure, makes sense - it's blink-specific rather than implementing something in
the spec but that's no harm.

+    // "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.
     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;
 }