Use a non-replaced inline container for image alt text

Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=meacer@chromium.org,dmazzoni@chromium.org

BUG=644802
Review-Url: https://codereview.chromium.org/2593263003
Cr-Commit-Position: refs/heads/master@{#466546}
Committed: https://chromium.googlesource.com/chromium/src/+/fe767186a3dc40ea14b5f807ff5438bd78f79c95

[rhogan, 2016-12-22 13:21:11]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 13:21:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 14:31:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 14:31:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-02-28 20:37:26]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:38:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-02-28 20:40:03]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:40:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-02-28 20:55:30]

Description was changed from

==========
Use a non-replaced inline container for image alt text

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform to the spec
(https://html.spec.whatwg.org/multipage/rendering.html#images-3).

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-02-28 20:57:15]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform to the spec
(https://html.spec.whatwg.org/multipage/rendering.html#images-3).

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-02-28 20:57:25]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to the second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-02-28 20:57:40]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to the second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer to Firefox with this change. Edge doesn't
conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to the second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer, nearly identical, to Firefox with this change. Edge
doesn't
conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-02-28 20:59:53]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 21:00:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 22:33:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 22:33:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-03-01 20:08:10]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to the second bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer, nearly identical, to Firefox with this change. Edge
doesn't
conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer, nearly identical, to Firefox with this change. Edge
doesn't
conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-03-02 20:29:29]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

We still have UA-specific behaviour when it comes to displaying the broken-image
icon, but we're much closer, nearly identical, to Firefox with this change. Edge
doesn't
conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

BUG=644802
==========

[rhogan, 2017-03-02 20:29:47]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:30:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rhogan, 2017-03-02 20:31:11]

robhogan@gmail.com changed reviewers:
+ pdr@chromium.org

[rhogan, 2017-03-02 20:31:12]

Hi pdr - you the right person to take a look at this? It's a follow up to that
image fallback patch you helped shepherd through a while back.

[commit-bot: I haz the power, 2017-03-02 20:34:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 20:34:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-03-02 20:48:35]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:49:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 21:49:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 21:49:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

Overall looks good!

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (left):

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:45:
container->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInlineBlock);
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?

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:113: if
(!imageHasNoSrcAttribute && imageHasEmptyAltAttribute)
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).

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:119:
elementRepresents = ElementRepresentsText;
Can this ever be hit since imageHasEmptyAltAttribute = !imageHasNoAltAttribute?

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150:
imageSmallerThanAltImage(newStyle->width(), newStyle->height())) {
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]

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (left):

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:45:
container->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInlineBlock);
On 2017/03/02 at 21:51:11, pdr. wrote:
> 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?

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.

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:113: if
(!imageHasNoSrcAttribute && imageHasEmptyAltAttribute)
On 2017/03/02 at 21:51:11, pdr. wrote:
> 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).

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.

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:119:
elementRepresents = ElementRepresentsText;
On 2017/03/02 at 21:51:11, pdr. wrote:
> Can this ever be hit since imageHasEmptyAltAttribute =
!imageHasNoAltAttribute?

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.

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150:
imageSmallerThanAltImage(newStyle->width(), newStyle->height())) {
On 2017/03/02 at 21:51:11, pdr. wrote:
> 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?

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

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

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:113: if
(!imageHasNoSrcAttribute && imageHasEmptyAltAttribute)
On 2017/03/02 at 23:44:46, rhogan wrote:
> On 2017/03/02 at 21:51:11, pdr. wrote:
> > 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).
> 
> 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.

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;
}

https://codereview.chromium.org/2593263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:119:
elementRepresents = ElementRepresentsText;
On 2017/03/02 at 23:44:46, rhogan wrote:
> On 2017/03/02 at 21:51:11, pdr. wrote:
> > Can this ever be hit since imageHasEmptyAltAttribute =
!imageHasNoAltAttribute?
> 
> 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.

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

[rhogan, 2017-03-06 20:09:31]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 20:10:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 21:19:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 21:19:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rhogan, 2017-03-12 14:44:54]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 14:45:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-12 16:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-12 16:00:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-03-12 17:56:06]

On 2017/03/03 at 00:52:06, pdr wrote:
> 
> Yeah I think it might be a little easier to read using the positive versions
like "imageHasSrcAttribute" and "imageHasAltAttribute"

This is ready for another look!

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

Sorry about taking a day to respond. I know these expectations are a PITA to
update :/

Just a couple more questions.

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:34: static bool
imageSmallerThanAltImage(const Length width, const Length height) {
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?

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150: // "If the
element is an img element that represents nothing and the user
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?

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

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (left):

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:41:
container->setInlineStyleProperty(CSSPropertyBorderWidth, 1,
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?

[rhogan, 2017-03-18 12:12:10]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 12:12:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 12:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 12:16:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-03-23 11:12:49]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 11:13:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 11:24:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 11:24:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-03-23 11:36:05]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 11:36:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150: // "If the
element is an img element that represents nothing and the user
On 2017/03/14 at 18:19:27, pdr. wrote:
> 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?

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.

[commit-bot: I haz the power, 2017-03-23 12:35:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 12:35:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2017-03-23 17:00:33]

On 2017/03/23 at 12:24:44, robhogan wrote:
>
https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):
> 
>
https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150: // "If
the element is an img element that represents nothing and the user
> On 2017/03/14 at 18:19:27, pdr. wrote:
> > 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?
> 
> 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.

Ok, can we separate out the border part?

[rhogan, 2017-03-23 19:47:53]

On 2017/03/23 at 17:00:33, pdr wrote:
> On 2017/03/23 at 12:24:44, robhogan wrote:
> >
https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
(right):
> > 
> >
https://codereview.chromium.org/2593263003/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:150: // "If
the element is an img element that represents nothing and the user
> > On 2017/03/14 at 18:19:27, pdr. wrote:
> > > 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?
> > 
> > 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.
> 
> Ok, can we separate out the border part?

Oh yes, sure, that's what the new version of the CL does. It's not ready for
landing but it's ready for a look.

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

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (left):

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:63:
altText->setInlineStyleProperty(CSSPropertyOverflow, CSSValueHidden);
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?

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:154:
newStyle->setHeight(Length());
In tables/mozilla/bugs/bug56201.html it looks like we draw a 0px by 16px box. I
wonder if there's a bug here?

[pdr., 2017-03-24 02:14:41]

Patch is looking good! It was kinda rough rolling out the shadow dom change a
while back so I'm a little more cautious, but overall I think this patch is
great.

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

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp (right):

https://codereview.chromium.org/2593263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:104: int
pixelsForAltImage = 18;
Should this be 1 + 1 + 16 + 1 + 1 = 20 to account for the border/padding on the
left and right sides?

[rhogan, 2017-03-26 11:19:15]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 11:19:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-26 12:06:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-26 12:07:00]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rhogan, 2017-03-27 20:49:15]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:50:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 22:11:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 22:11:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-03-28 21:16:55]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:17:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 21:17:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:17:14]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[rhogan, 2017-03-28 21:20:13]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:20:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 21:20:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:20:39]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[rhogan, 2017-03-29 17:51:45]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 17:52:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 19:05:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 19:05:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-03-29 20:10:20]

Ready for another look.

On 2017/03/24 at 02:15:45, pdr wrote:
> third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp:104: int
pixelsForAltImage = 18;
> Should this be 1 + 1 + 16 + 1 + 1 = 20 to account for the border/padding on
the left and right sides?

My reasoning here was that as long as I can fit in the top and left
border/padding and the image itself then I can display it, i.e. I don't care
about clipping the bottom/right border/padding.

[pdr., 2017-04-05 18:41:24]

LGTM % one issue:
Are fast/text/whitespace/normal-after-nowrap-breaking-expected.png and
tables/mozilla/bugs/bug4527-expected.png are regressions where we should be
clipping the alt text?

[rhogan, 2017-04-05 20:29:19]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 20:29:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 21:57:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 21:57:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-06 21:33:55]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 21:35:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 23:07:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 23:07:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-08 09:44:42]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-08 09:44:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-08 11:09:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-08 11:09:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-11 22:05:17]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-04-11 22:05:20]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2593263003/#ps360001
(title: "Use a non-replaced inline container for image alt text")

[commit-bot: I haz the power, 2017-04-11 22:06:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 22:34:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 22:34:13]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rhogan, 2017-04-12 17:36:13]

robhogan@gmail.com changed reviewers:
+ avi@chromium.org, meacer@chromium.org

[rhogan, 2017-04-12 17:37:11]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=avi@chromium.org,meacer@chromium.org

BUG=644802
==========

[rhogan, 2017-04-12 17:37:26]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2017-04-12 17:38:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:48:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:48:38]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2017-04-13 14:36:23]

lgtm stamp

[rhogan, 2017-04-14 07:24:01]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 07:24:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 09:13:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 09:13:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[rhogan, 2017-04-17 09:49:43]

robhogan@gmail.com changed reviewers:
+ dmazzoni@chromium.org

[rhogan, 2017-04-17 09:49:48]

https://codereview.chromium.org/2593263003/diff/390001/content/test/data/acce...
File content/test/data/accessibility/html/a-no-text.html (right):

https://codereview.chromium.org/2593263003/diff/390001/content/test/data/acce...
content/test/data/accessibility/html/a-no-text.html:4: <html>
dmazzoni, need your input here.

The changes I'm making to the way we display fallback content affects some
assumptions in this test about the accessibility tree.

I believe this is because I am changing the shadow root used to display fallback
content from  something like:

<div id="alttext-container" style="overflow: hidden; border-width: 1px;
border-style: solid; border-color: silver; display: inline-block; box-sizing:
border-box; padding: 1px;"><img id="alttext-image" width="16" height="16"
align="left" style="margin: 0px; float: left; display: none;"><div id="alttext"
style="overflow: hidden; display: block;"></div></div>

to:

<span id="alttext-container"><img id="alttext-image" width="16" height="16"
align="left" style="margin: 0px; display: none;"><span
id="alttext"></span></span>

I'm guessing that the accessibilty tree has different expectations of what to
pull from inline versus block elements maybe?

So my question is: what would you like me to do here? Just rebaseline the test?
Or change it in a way similar to the way I've changed it here?

Here is the accessibility tree for this test before and after my CL:

Before:

AXTreeUpdate tree data: doctype=html loaded=true loading_progress=1
mimetype=text/html
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
focus_id=4 routing_id=2
AXTreeUpdate: clear node 1
AXTreeUpdate: root id 4
id=4 rootWebArea FOCUSABLE READONLY (0, 0)-(800, 600) background_color=&FFFFFFFF
color=&FF000000 scroll_x=0 scroll_y=0 scroll_x_min=0 scroll_y_min=0
scroll_x_max=0 scroll_y_max=0 value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
font-family=Times New Roman html_tag=#document font_size=16 child_ids=6
id=6 group READONLY (8, 8)-(784, 584) offset_container_id=4
background_color=&FFFFFFFF color=&FF000000 value= display=block html_tag=body
font_size=16 child_ids=7,9,11,13,15,17,19,21,23,26
id=7 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest1
display=inline html_tag=a font_size=16
id=9 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value= url=file:///dest2 display=inline
html_tag=a font_size=16
id=11 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest3/
display=inline html_tag=a font_size=16
id=13 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value= url=http://example.com/dest4
display=inline html_tag=a font_size=16
id=15 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value= url=http://example.com/dest5/
display=inline html_tag=a font_size=16
id=17 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest6.html
display=inline html_tag=a font_size=16
id=19 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value= url=http://example.com/dest7.html
display=inline html_tag=a font_size=16
id=21 link FOCUSABLE LINKED OFFSCREEN READONLY (8, 8)-(0, 0)
offset_container_id=4 background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump value= url=http://example.com/dest.8.html
display=inline html_tag=a font_size=16
id=23 link FOCUSABLE LINKED READONLY (8, 8)-(200, 103) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE text_style=underline action=jump
value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest9.html
display=inline html_tag=a font_size=16 child_ids=36
id=36 image LINKED READONLY (8, 8)-(200, 100) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/greenbox.png
display=inline html_tag=img role=img font_size=16
id=26 link FOCUSABLE LINKED READONLY (212, 8)-(200, 103) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE text_style=underline action=jump
value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest10.html
display=inline html_tag=a font_size=16 child_ids=37
id=37 image LINKED READONLY (212, 8)-(200, 100) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/greenbox.png
display=inline html_tag=img role=img font_size=16
[1:1:0417/101707.388342:228573758262:VERBOSE1:render_accessibility_impl.cc(416)]
Accessibility event: loadComplete on node id 4


After:
AXTreeUpdate tree data: doctype=html loaded=true loading_progress=1
mimetype=text/html
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
focus_id=4 routing_id=2
AXTreeUpdate: clear node 1
AXTreeUpdate: root id 4
id=4 rootWebArea FOCUSABLE READONLY (0, 0)-(800, 600) background_color=&FFFFFFFF
color=&FF000000 scroll_x=0 scroll_y=0 scroll_x_min=0 scroll_y_min=0
scroll_x_max=0 scroll_y_max=0 value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
font-family=Times New Roman html_tag=#document font_size=16 child_ids=6
id=6 group READONLY (8, 8)-(784, 584) offset_container_id=4
background_color=&FFFFFFFF color=&FF000000 value= display=block html_tag=body
font_size=16 child_ids=7,9,11,13,15,17,19,21,23,26
id=7 link FOCUSABLE LINKED READONLY (8, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest1
display=inline html_tag=a font_size=16 child_ids=28
id=28 image LINKED OFFSCREEN READONLY (8, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html

display=inline html_tag=img role=img font_size=16
id=9 link FOCUSABLE LINKED READONLY (12, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value= url=file:///dest2 display=inline
html_tag=a font_size=16 child_ids=31
id=31 image LINKED OFFSCREEN READONLY (12, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
display=inline html_tag=img role=img font_size=16
id=11 link FOCUSABLE LINKED READONLY (16, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest3/
display=inline html_tag=a font_size=16 child_ids=34
id=34 image LINKED OFFSCREEN READONLY (16, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
display=inline html_tag=img role=img font_size=16
id=13 link FOCUSABLE LINKED READONLY (20, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value= url=http://example.com/dest4
display=inline html_tag=a font_size=16 child_ids=37
id=37 image LINKED OFFSCREEN READONLY (20, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
 display=inline html_tag=img role=img font_size=16
id=15 link FOCUSABLE LINKED READONLY (24, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value= url=http://example.com/dest5/
display=inline html_tag=a font_size=16 child_ids=40
id=40 image LINKED OFFSCREEN READONLY (24, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
 display=inline html_tag=img role=img font_size=16
id=17 link FOCUSABLE LINKED READONLY (28, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest6.html
display=inline html_tag=a font_size=16 child_ids=43
id=43 image LINKED OFFSCREEN READONLY (28, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
 display=inline html_tag=img role=img font_size=16
id=19 link FOCUSABLE LINKED READONLY (32, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value=
url=http://example.com/dest7.html display=inline html_tag=a font_size=16
child_ids=46
id=46 image LINKED OFFSCREEN READONLY (32, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
 display=inline html_tag=img role=img font_size=16
id=21 link FOCUSABLE LINKED READONLY (36, 94)-(4, 17) offset_container_id=4
name_from=contents background_color=&FFFFFFFF color=&FF0000EE
text_style=underline action=jump name=   value=
url=http://example.com/dest.8.html display=inline html_tag=a font_size=16
child_ids=49
id=49 image LINKED OFFSCREEN READONLY (36, 108)-(0, 0) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/a-no-text.html
 display=inline html_tag=img role=img font_size=16
id=23 link FOCUSABLE LINKED READONLY (40, 8)-(200, 103) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE text_style=underline action=jump
value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest9.html
display=inline html_tag=a font_size=16 child_ids=52
id=52 image LINKED READONLY (40, 8)-(200, 100) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/greenbox.png
display=inline html_tag=img role=img font_size=16
id=26 link FOCUSABLE LINKED READONLY (244, 8)-(200, 103) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE text_style=underline action=jump
value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/dest10.html
display=inline html_tag=a font_size=16 child_ids=53
id=53 image LINKED READONLY (244, 8)-(200, 100) offset_container_id=4
background_color=&FFFFFFFF color=&FF0000EE action=click value=
url=file:///home/robert/Dev/blink/src/content/test/data/accessibility/html/greenbox.png
display=inline html_tag=img role=img font_size=16

[rhogan, 2017-04-19 18:27:28]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 18:28:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 21:14:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 21:14:30]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-04-19 21:25:04]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=avi@chromium.org,meacer@chromium.org

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=meacer@chromium.org,dmazzoni@chromium.org

BUG=644802
==========

[dmazzoni, 2017-04-19 23:33:54]

I'd prefer not to change the test, it doesn't look wrong.

I think we should be able to fix this. Let me try to patch it in and see.

[dmazzoni, 2017-04-20 07:03:53]

lgtm

OK, after looking it some more, the change is fine in terms of
what it's doing to the accessibility tree. I see some opportunities
for improvement but nothing that has to be done as part of this
change.

Please don't modify the test itself, just rebaseline the expectation.

Thanks!

[rhogan, 2017-04-20 19:58:48]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 19:59:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 21:09:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:09:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-20 21:34:23]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:34:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-04-20 23:31:54]

lgtm

[commit-bot: I haz the power, 2017-04-21 00:00:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 00:00:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2017-04-21 17:00:03]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-04-21 17:00:06]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
pdr@chromium.org, dmazzoni@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2593263003/#ps430001
(title: "Use a non-replaced inline container for image alt text")

[commit-bot: I haz the power, 2017-04-21 17:00:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 18:58:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 18:58:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rhogan, 2017-04-22 12:12:49]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-04-22 12:12:54]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dmazzoni@chromium.org, pdr@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2593263003/#ps440001
(title: "Use a non-replaced inline container for image alt text")

[commit-bot: I haz the power, 2017-04-22 12:13:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 19:24:49]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1492863169287290,
"parent_rev": "32d9f5075ba7f767d2c6c6d41794dc70b5ff7bf3", "commit_rev":
"fe767186a3dc40ea14b5f807ff5438bd78f79c95"}

[commit-bot: I haz the power, 2017-04-22 19:25:07]

Description was changed from

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=meacer@chromium.org,dmazzoni@chromium.org

BUG=644802
==========

to

==========
Use a non-replaced inline container for image alt text

This makes us conform more closely to 
https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element
and the third bullet point in the spec 
at https://html.spec.whatwg.org/multipage/rendering.html#images-3.

Our behaviour when it comes to displaying the broken-image icon is still
not the same as Firefox (we display it more often) but I think we're closer
to the spec in that regard. Edge doesn't conform to the spec at all.

TBR=meacer@chromium.org,dmazzoni@chromium.org

BUG=644802

Review-Url: https://codereview.chromium.org/2593263003
Cr-Commit-Position: refs/heads/master@{#466546}
Committed:
https://chromium.googlesource.com/chromium/src/+/fe767186a3dc40ea14b5f807ff54...
==========

[commit-bot: I haz the power, 2017-04-22 19:25:11]

Committed patchset #29 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/fe767186a3dc40ea14b5f807ff54...