Use a non-replaced inline container for image alt text

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/html/HTMLImageFallbackHelper.h"
 
 #include "core/HTMLNames.h"
 #include "core/InputTypeNames.h"
 #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"
 
 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(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);
   brokenImage->setIsFallbackImage();
   brokenImage->setAttribute(idAttr, AtomicString("alttext-image"));
   brokenImage->setAttribute(widthAttr, AtomicString("16"));
   brokenImage->setAttribute(heightAttr, AtomicString("16"));
   brokenImage->setAttribute(alignAttr, AtomicString("left"));
   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());
   altText->appendChild(text);
 }
 
 PassRefPtr<ComputedStyle> HTMLImageFallbackHelper::customStyleForAltText(
     Element& element,
     PassRefPtr<ComputedStyle> newStyle) {
   // If we have an author shadow root or have not created the UA shadow root
@@ skipping 20 matching lines @@
     else if (newStyle->height().isSpecifiedOrIntrinsic() &&
              newStyle->width().isAuto())
       newStyle->setWidth(newStyle->height());
     if (newStyle->width().isSpecifiedOrIntrinsic() &&
         newStyle->height().isSpecifiedOrIntrinsic()) {
       placeHolder->setInlineStyleProperty(CSSPropertyVerticalAlign,
                                           CSSValueBaseline);
     }
   }
 
-  // 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
   // 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::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;
 }
 
 }  // namespace blink