570322 - random fail of image map (area) accessible creation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

It was caught by mochitests (bug 569425). I file new bug to avoid spamming by reports of failing mochitests.

David assumed (bug 569425 comment #10) that the bug might related with lazy frame construction. CC'ing Boris.

Boris, it sounds the real problem should be we can't create image map accessible what happens in GetAreaAccessible - http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1786. 

On the another hand we could fail early before before GetAreaAccessible is called where we use the following assumption  content->GetPrimaryFrame()->GetContent() != content (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1438). Can it fail for image map node?

Comment 1

[Timothy Nikkel (:tnikkel)]

I'm fairly certain what the problem is here, bug 569425 comment 11, it is not lazy frame construction, image maps already had their own annoying form of setting primary frame pointer asynchronously.

Comment 2

[Timothy Nikkel (:tnikkel)]

This bug is now to deal with the issue mentioned in bug 569425 comment 14, where accessible creation will fail if there has not been a mouse event over the image or the image has not been painted.

Comment 3

[alexander :surkov (:asurkov)]

actually the fix of the bug 741474 is not really connected, for bug 741474 we should mouse over the image map.

for this bug I think we should get rid mouse over hack and just wait for reorder event.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 5

[alexander :surkov (:asurkov)]

follow the wip idea to replace ensureImageMapTree on new waitForImageMap.

Comment 6

[Mark Capella [:capella]]

Attachment: Wip II

I followed surkovs example and modified all tests using the older ensureImageMapTree() function to use his new waitForImageMap() function.

All tests pass, but for the treeupdate/test_imagemap.html test, I had to leave / include the mouseover hack synthesizeMouse(), otherwise the test hangs and ... waits for a mouse over :)

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 633061 [details] [diff] [review]
Wip II

Nice! f=me.

Comment 8

[Mark Capella [:capella]]

Comment on attachment 633061 [details] [diff] [review]
Wip II

Asking for review... does this satisfy the bug with that last mouseover hack still there?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 633061 [details] [diff] [review]
Wip II

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

Definitely progress, but test_imagemap is really the main thorn (bug 745788) so I'd like to get that figured out. Image map content/layout code is not the most fun.

If you like r=me but call this part 1 and leave the bug open please.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Note I pondered (since pretest is so common) asking you to make that a util function, but since these are tests I actually personally like to see explicitly what's going on in each file anyways.

Comment 11

[Mark Capella [:capella]]

Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=d888ad9b1a5e

Orange on Linux indicating incorrect solution:

2622 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/bounds/test_zoom.html | Test timed out.
3502 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_docload.html | Different amount of expected children of [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. - got 3, expected 2
3509 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_docload.html | Different amount of expected children of [' no node info ', role: app root, name: 'Firefox', address: [xpconnect wrapped nsIAccessible]]. - got 3, expected 2

Comment 12

[alexander :surkov (:asurkov)]

You might want to keep mouse over thing for waitForImageMap function, this should keep working both treeupdate/test_imagemap.html and bounds/test_zoom.html

Comment 13

[Mark Capella [:capella]]

Modified per Surkovs suggestion. re-try ...

https://tbpl.mozilla.org/?tree=Try&rev=8d168ec70845

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v3)

Updated to show what was pushed to TRY...

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 634008 [details] [diff] [review]
Patch (v3)

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

Thanks. r=me if tests pass. (Bummer - I thought you'd run them on the earlier patch and somehow the image maps lazily instantiated on their own).

Comment 16

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=10e3d0212100

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10e3d0212100

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to David Bolter [:davidb] from comment #9)
> Comment on attachment 633061 [details] [diff] [review]
> Wip II
> 
> Review of attachment 633061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Definitely progress, but test_imagemap is really the main thorn (bug 745788)
> so I'd like to get that figured out. Image map content/layout code is not
> the most fun.
> 
> If you like r=me but call this part 1 and leave the bug open please.

I don't understand my own comment.

Mark this should fix test_imagemap too right? (If you agree, close it fixed)

Comment 19

[Mark Capella [:capella]]

It was my understanding that this change we just moved would close out this bug but not correct the issue with bug 745788. and indeed that bug had a fail last night at or around the time this patch went in. 

Note that several people have had a hand in the discussion here, and several bugs / issues are referred to over time:

   (bug 569425) can't get accessible
   (bug 741474) Different amount of expected children
   (Bug 570322) This one - random fail of image map (area) accessible creation
   (Bug 745788) Intermittent a11y/accessible/treeupdate/test_imagemap.html | Test timed out

We may have begun chasing our tail?

I'd suggest that we close this bug out, and focus fresh on what's wrong with the last remaining open bug 745788.