852129 - "ASSERTION: Text leaf parent is not hypertext" with text node child of <img>

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires accessibility, asserts)

1. Enable accessibility.
2. Load the testcase.

###!!! ASSERTION: Text leaf parent is not hypertext!: 'Error', file accessible/src/base/TextUpdater.cpp, line 49

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Trevor Saunders (:tbsaunde)]

it looks like the issue here is just that ImgeAccessible doesn't block having kids I wonder if we should make LinkbleAccessible inherit from LefAccessible?

Comment 3

[alexander :surkov (:asurkov)]

What is a purpose of text node inside img element? Does it have any semantic?

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> it looks like the issue here is just that ImgeAccessible doesn't block
> having kids I wonder if we should make LinkbleAccessible inherit from
> LefAccessible?

No, the text is rendered. I guess the image should be an ordinal hypertext (not an image accessible).

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> What is a purpose of text node inside img element? Does it have any semantic?
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > it looks like the issue here is just that ImgeAccessible doesn't block
> > having kids I wonder if we should make LinkbleAccessible inherit from
> > LefAccessible?
> 
> No, the text is rendered. I guess the image should be an ordinal hypertext
> (not an image accessible).

and the image is ignored? weird that is not what I expected, but ok.  I gues it should be ordinary HyperText then yes.  That'll make treeupdate suck even more but owe well :/

Comment 5

[Jesse Ruderman]

> What is a purpose of text node inside img element? Does it have any semantic?

It's the DOM fuzzer's way of saying hi.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Jesse Ruderman from comment #5)
> > What is a purpose of text node inside img element? Does it have any semantic?
> 
> It's the DOM fuzzer's way of saying hi.

"hi, I'm from the department of DOM fuzzing and I just wanted to make sure you were aware you can put text within a <img>" ;)

Comment 7

[alexander :surkov (:asurkov)]

it shouldn't stuck tree update we just should tweak accessible creation. I'm curious why image frame is in use.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Jesse Ruderman from comment #5)
> > What is a purpose of text node inside img element? Does it have any semantic?
> 
> It's the DOM fuzzer's way of saying hi.

DOM is usually smart. For example, if you do HTML tables wrong then DOM is built correct. I'd guess that visible elements inside of HTML img having no src is just a fallback content. But it's better to check with DOM guru

Comment 9

[Trevor Saunders (:tbsaunde)]

Boris, can you answer comments 7 and 8?

Comment 10

[Boris Zbarsky [:bzbarsky]]

So all that's going on here from the DOM/layout point of view is this:

1)  DOM doesn't prevent <img> having child nodes.  You can't get there via the HTML parser,
    but either the XML parser or direct DOM manipulation will let it happen.
2)  When layout creates frames for an <img>, if the image is judged as "broken" (and a few
    other conditions to do with quirks mode, styling, etc) it will be treated just like a
    <span>, with ::before { content: attr(alt); } styling on it or some such.  Which means
    that kids of the <img> will render at that point...

Comment 11

[alexander :surkov (:asurkov)]

how we can detect the image is "broken" and where we should add hooks to notify a11y the image changes its "brokenness" state?

Comment 12

[Boris Zbarsky [:bzbarsky]]

See what nsImageFrame::ShouldCreateImageFrameFor looks at in terms of deciding what sort of image to create?

When the brokenness state changes, the image is reframed; this should already notify a11y, right?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> See what nsImageFrame::ShouldCreateImageFrameFor looks at in terms of
> deciding what sort of image to create?

ok

> When the brokenness state changes, the image is reframed; this should
> already notify a11y, right?

then yes

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: bug 852129 - use HyperTextAccessible for invalid img

If the img is not valid then its children will be rendered, and the
sensible way to handle that is by giving the img a HyperTextAccessible
instead of a ImageAccessible.  Since the accessible name of such an img
should be the value of the alt attribute we add similar as
ImageAccessible::NativeName() to HyperTextAccessible::NativeName()
conditioned on the tag being img.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 761340 [details] [diff] [review]
bug 852129 - use HyperTextAccessible for invalid img

Review of attachment 761340 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +2111,5 @@
>  // Accessible protected
>  ENameValueFlag
>  HyperTextAccessible::NativeName(nsString& aName)
>  {
> +  // check alt attribute for invalid img elements

nit: check -> Check, alt -> @alt. Dot in the end

@@ +2112,5 @@
>  ENameValueFlag
>  HyperTextAccessible::NativeName(nsString& aName)
>  {
> +  // check alt attribute for invalid img elements
> +  bool hasAlt = false;

maybe hasImgAlt

::: accessible/tests/mochitest/tree/test_invalid_img.xhtml
@@ +16,5 @@
> +  <script>
> +  <![CDATA[
> +    function doTest()
> +    {
> +      document.getElementsByTagName("img")[0].firstChild.data = "2";

what is it for?

@@ +34,5 @@
> +</head>
> +<body>
> +
> +  <a target="_blank"
> +     title="test tree for invalid img"

maybe: use hypertext accessible for broken HTML img

@@ +44,5 @@
> +  <div id="content" style="display: none"></div>
> +  <pre id="test">
> +  </pre>
> +
> +<img id="the_img">1</img>

nit: indent it

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761340 [details] [diff] [review]
bug 852129 - use HyperTextAccessible for invalid img

Seems fine to me if we're not worrying about <svg:image> and <input type="image">.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 761340 [details] [diff] [review]
> bug 852129 - use HyperTextAccessible for invalid img
> 
> Seems fine to me if we're not worrying about <svg:image> and <input
> type="image">.

what's special about them?

Comment 18

[Boris Zbarsky [:bzbarsky]]

They behave generally just like <html:img> but are not IsHTML(nsGkAtoms::img).

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Comment on attachment 761340 [details] [diff] [review]
> bug 852129 - use HyperTextAccessible for invalid img
> 
> Seems fine to me if we're not worrying about <svg:image> and <input
> type="image">.

We don't need to worry about svg images because they don't get HyperTextAccessibles atm.  The same is true for input type=image, though I'm less clear on why they get HTMLButtonAccessibles even when the input is invalid.

I added a test for the type=image case to test_general.html since I didn't immediately find one though I thought we already had test cases for that.

Comment 20

[Trevor Saunders (:tbsaunde)]

> ::: accessible/tests/mochitest/tree/test_invalid_img.xhtml
> @@ +16,5 @@
> > +  <script>
> > +  <![CDATA[
> > +    function doTest()
> > +    {
> > +      document.getElementsByTagName("img")[0].firstChild.data = "2";
> 
> what is it for?

just coppying test case, seems safer but I'm not really sure why we need it.

Comment 21

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f243d3f45be

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f243d3f45be