Revert "Disable AccessibilityANoText until crrev.com/481753002 lands"

Revert "Disable AccessibilityANoText until crrev.com/481753002 lands"

BUG=440142
Committed: https://crrev.com/d6dd565126b8582d11929b463a36103f54628bf2
Cr-Commit-Position: refs/heads/master@{#310377}

[rhogan, 2015-01-01 13:41:30]

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

[pdr., 2015-01-05 19:03:01]

On 2015/01/01 at 13:41:31, robhogan wrote:
> 

LGTM, though we'll need a content owner to rubber stamp.

[dmazzoni, 2015-01-05 19:09:24]

not lgtm

It looks like you're rebaselining the test, right? (The change description of
"Revert Disable" is a bit confusing.)

If so, the test is now broken. The previous test results were correct, and we've
lost some desirable behavior in the new test results.

Can you check if your change was the one that broke the test, or if something
else that landed in-between when you disabled this and now broke it?

[rhogan, 2015-01-05 19:29:37]

On 2015/01/05 at 19:09:24, dmazzoni wrote:
> not lgtm
> 
> It looks like you're rebaselining the test, right? (The change description of
"Revert Disable" is a bit confusing.)
> 
> If so, the test is now broken. The previous test results were correct, and
we've lost some desirable behavior in the new test results.
> 
> Can you check if your change was the one that broke the test, or if something
else that landed in-between when you disabled this and now broke it?

Hi Dominic,

In https://codereview.chromium.org/751553006#msg2 I said:

"See:

http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...

The new results will use the src as the name, rather than the href - just like
the two successful image loads at the end of the test. This is because the image
load is now asynchronous for the changed tests whereas before the images loaded
and failed immediately."

It now looks like the last two use the href. I'm not sure whether I just misread
the results for the last 2 before or whether that is something that has happened
since.

Are you saying that you expect to see the old results retained or do you
specifically have a problem with the new results for the last two?

[dmazzoni, 2015-01-05 19:34:42]

On 2015/01/05 19:29:37, rhogan wrote:
> On 2015/01/05 at 19:09:24, dmazzoni wrote:
> > not lgtm
> > 
> > It looks like you're rebaselining the test, right? (The change description
of
> "Revert Disable" is a bit confusing.)
> > 
> > If so, the test is now broken. The previous test results were correct, and
> we've lost some desirable behavior in the new test results.
> > 
> > Can you check if your change was the one that broke the test, or if
something
> else that landed in-between when you disabled this and now broke it?
> 
> Hi Dominic,
> 
> In https://codereview.chromium.org/751553006#msg2 I said:
> 
> "See:
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
> 
> The new results will use the src as the name, rather than the href - just like
> the two successful image loads at the end of the test. This is because the
image
> load is now asynchronous for the changed tests whereas before the images
loaded
> and failed immediately."
> 
> It now looks like the last two use the href. I'm not sure whether I just
misread
> the results for the last 2 before or whether that is something that has
happened
> since.
> 
> Are you saying that you expect to see the old results retained or do you
> specifically have a problem with the new results for the last two?

I'm okay with the new results for the last two.

Can you figure out what happened to the first 8?

It's quite possible it wasn't your change. If it wasn't, and it isn't obvious
what broke it, you can assign to me and I'll bisect.

[rhogan, 2015-01-05 21:20:22]

On 2015/01/05 at 19:34:42, dmazzoni wrote:
> 
> Can you figure out what happened to the first 8?

These were definitely my change. It's what I meant when I said that the new
results will use the src as the name rather than the href.

I'm looking into this a bit further to confirm my previous assertion that this
is due to the fact that the image loads now fail asynchronously.

Do you have any suggestions for debugging this accessibility stuff in linux? The
corresponding tests are disabled on linux builds.

[dmazzoni, 2015-01-05 23:21:53]

On Mon, Jan 5, 2015 at 1:20 PM, <robhogan@gmail.com> wrote:

> Do you have any suggestions for debugging this accessibility stuff in
> linux? The
> corresponding tests are disabled on linux builds.
>

You could build Clank from Linux, but if you haven't done that before it's
a bit of work.

From a debug build on Linux, run Chrome with the command-line flag
--force-renderer-accessibility and add --v=1 or use vmodule to ensure that
you're logging renderer_accessibility.cc:260. Now when you open any webpage
it will log all of the accessibility events, including a dump of the
internal representation of the accessibility tree. If you dump that before
and after your change it should be clear what changed.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rhogan, 2015-01-06 20:15:10]

On 2015/01/05 at 23:21:53, dmazzoni wrote:
> From a debug build on Linux, run Chrome with the command-line flag
> --force-renderer-accessibility and add --v=1 or use vmodule to ensure that
> you're logging renderer_accessibility.cc:260. Now when you open any webpage
> it will log all of the accessibility events, including a dump of the
> internal representation of the accessibility tree. If you dump that before
> and after your change it should be clear what changed.

Great pointer, thanks! I now know what's going on. This is what the
accessibility tree initially looks like when loading '<a href="dest1.html"><img
src="#"> </a>':

id=231 rootWebArea FOCUSABLE READONLY (0, 0)-(1615, 963) 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-tex
t.html html_tag=#document doc_title=
doc_url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html
doc_mimetype=text/html name= doc_progress=.8999999761581421 doc_loaded=false
child_ids=234
  id=234 group READONLY (8, 8)-(1599, 947) value= display=block html_tag=body
name= child_ids=235
    id=235 link FOCUSABLE LINKED OFFSCREEN READONLY (0, 0)-(0, 0) value=
action=jump display=inline
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/dest1
html_tag=a name=

But then, and this is the new part, the failed image load causes us to detach
the image renderer and add a renderer with fallback content. This prompts
childrenChanged events to post to the anchor element:

[99:99:0106/184550:INFO:renderer_accessibility.cc(260)] Accessibility event:
childrenChanged on node id 235
id=235 link FOCUSABLE LINKED READONLY (8, 8)-(20, 23) value= action=jump
display=inline
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/dest1
html_tag=a name= child_ids=239
  id=239 image LINKED READONLY (8, 8)-(20, 20) value= display=inline
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html#
html_tag=img role=img name=

It's this event that the test framework is swallowing and as a result is
assigning the value of the 'url' (a-no-text) for the image element to the 'name'
field in the result:

   android.view.View clickable focusable name='a-no-text'

So I'm not sure if this is a shortcoming in the way the test framework is
consuming events or if we want to prevent these events happening in the first
place.

Dominic - I'm hoping you can advise the right thing to do here?

[dmazzoni, 2015-01-06 21:47:42]

On 2015/01/06 20:15:10, rhogan wrote:
> On 2015/01/05 at 23:21:53, dmazzoni wrote:
> > From a debug build on Linux, run Chrome with the command-line flag
> > --force-renderer-accessibility and add --v=1 or use vmodule to ensure that
> > you're logging renderer_accessibility.cc:260. Now when you open any webpage
> > it will log all of the accessibility events, including a dump of the
> > internal representation of the accessibility tree. If you dump that before
> > and after your change it should be clear what changed.
> 
> Great pointer, thanks! I now know what's going on. This is what the
> accessibility tree initially looks like when loading '<a
href="dest1.html"><img
> src="#"> </a>':
> 
> id=231 rootWebArea FOCUSABLE READONLY (0, 0)-(1615, 963) 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-tex
> t.html html_tag=#document doc_title=
>
doc_url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html
> doc_mimetype=text/html name= doc_progress=.8999999761581421 doc_loaded=false
> child_ids=234
>   id=234 group READONLY (8, 8)-(1599, 947) value= display=block html_tag=body
> name= child_ids=235
>     id=235 link FOCUSABLE LINKED OFFSCREEN READONLY (0, 0)-(0, 0) value=
> action=jump display=inline
>
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/dest1
> html_tag=a name=
> 
> But then, and this is the new part, the failed image load causes us to detach
> the image renderer and add a renderer with fallback content. This prompts
> childrenChanged events to post to the anchor element:
> 
> [99:99:0106/184550:INFO:renderer_accessibility.cc(260)] Accessibility event:
> childrenChanged on node id 235
> id=235 link FOCUSABLE LINKED READONLY (8, 8)-(20, 23) value= action=jump
> display=inline
>
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/dest1
> html_tag=a name= child_ids=239
>   id=239 image LINKED READONLY (8, 8)-(20, 20) value= display=inline
>
url=file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html#
> html_tag=img role=img name=
> 
> It's this event that the test framework is swallowing and as a result is
> assigning the value of the 'url' (a-no-text) for the image element to the
'name'
> field in the result:
> 
>    android.view.View clickable focusable name='a-no-text'
> 
> So I'm not sure if this is a shortcoming in the way the test framework is
> consuming events or if we want to prevent these events happening in the first
> place.
> 
> Dominic - I'm hoping you can advise the right thing to do here?

Thanks! That helps a lot.

It looks like the issue might be even simpler. I think that previously the url
associated with the image was just "#" and now it's the resolved url. Is that
possible? Tracing through the logic, I'm pretty sure that we would have ignored
a url of "#" and used the link's destination url to form the AX name, but given
a full url of
"file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html#"
we parse out the filename.

In particular, AXRenderObject::url() used to return "#" and now it returns the
full url - and tracing back, that means HTMLImageElement::src() used to return
"#", etc. - does that make sense as an unintentional side effect of this change?

What happens if you edit the text file and make the src of each image ""
instead, does that give it something closer to its original behavior? If so,
that seems like a perfectly reasonable fix.

Thanks!

[rhogan, 2015-01-07 20:19:38]

On 2015/01/06 at 21:47:42, dmazzoni wrote:
> Thanks! That helps a lot.
> 
> It looks like the issue might be even simpler. I think that previously the url
associated with the image was just "#" and now it's the resolved url. Is that
possible? Tracing through the logic, I'm pretty sure that we would have ignored
a url of "#" and used the link's destination url to form the AX name, but given
a full url of
"file:///home/robert/Dev/blink/src//content/test/data/accessibility/html/a-no-text.html#"
we parse out the filename.
> 
> In particular, AXRenderObject::url() used to return "#" and now it returns the
full url - and tracing back, that means HTMLImageElement::src() used to return
"#", etc. - does that make sense as an unintentional side effect of this change?

No, I'm pretty sure (as certain as I can be given what I know) my change had no
effect on the value returned for src(). It still appears to me that the new
behaviour in these tests is the childrenChanged events and that they're getting
picked up and used for the output. Previously those childrenChanged events did
not happen in this test because image fallback content did not require the
element to detach and reattach.

> 
> What happens if you edit the text file and make the src of each image ""
instead, does that give it something closer to its original behavior? If so,
that seems like a perfectly reasonable fix.

I followed this suggestion and posted a new patch. The new results are like the
original behaviour.

[dmazzoni, 2015-01-07 20:55:56]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-01-07 20:55:57]

lgtm

Thanks!

[commit-bot: I haz the power, 2015-01-07 20:56:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/805403003/40001

[commit-bot: I haz the power, 2015-01-07 21:56:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-07 21:58:30]

Patchset 3 (id:??) landed as
https://crrev.com/d6dd565126b8582d11929b463a36103f54628bf2
Cr-Commit-Position: refs/heads/master@{#310377}