Last Comment Bug 570322 - random fail of image map (area) accessible creation
: random fail of image map (area) accessible creation
Status: RESOLVED FIXED
[leave open][mentor=marco.zehe@google...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: treea11y 569425
  Show dependency treegraph
 
Reported: 2010-06-05 09:28 PDT by alexander :surkov
Modified: 2012-06-20 00:33 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (4.35 KB, patch)
2012-04-15 23:42 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
Wip II (17.47 KB, patch)
2012-06-14 00:40 PDT, Mark Capella [:capella]
dbolter: review+
mzehe: feedback+
Details | Diff | Splinter Review
Patch (v3) (17.49 KB, patch)
2012-06-18 06:34 PDT, Mark Capella [:capella]
dbolter: review+
markcapella: checkin+
Details | Diff | Splinter Review

Description alexander :surkov 2010-06-05 09:28:05 PDT
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) 2010-06-05 13:40:19 PDT
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) 2010-06-14 12:49:09 PDT
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 2012-04-15 23:42:03 PDT
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 2012-04-15 23:42:43 PDT
Created attachment 615258 [details] [diff] [review]
wip
Comment 5 alexander :surkov 2012-04-15 23:43:27 PDT
follow the wip idea to replace ensureImageMapTree on new waitForImageMap.
Comment 6 Mark Capella [:capella] 2012-06-14 00:40:11 PDT
Created attachment 633061 [details] [diff] [review]
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) 2012-06-14 00:57:57 PDT
Comment on attachment 633061 [details] [diff] [review]
Wip II

Nice! f=me.
Comment 8 Mark Capella [:capella] 2012-06-14 01:11:23 PDT
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] 2012-06-15 06:44:42 PDT
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] 2012-06-15 06:47:05 PDT
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] 2012-06-15 21:04:57 PDT
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 2012-06-16 22:27:11 PDT
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] 2012-06-18 03:55:42 PDT
Modified per Surkovs suggestion. re-try ...

https://tbpl.mozilla.org/?tree=Try&rev=8d168ec70845
Comment 14 Mark Capella [:capella] 2012-06-18 06:34:02 PDT
Created attachment 634008 [details] [diff] [review]
Patch (v3)

Updated to show what was pushed to TRY...
Comment 15 David Bolter [:davidb] 2012-06-18 06:41:53 PDT
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] 2012-06-18 07:47:42 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=10e3d0212100
Comment 17 Ed Morley [:emorley] 2012-06-19 01:21:08 PDT
https://hg.mozilla.org/mozilla-central/rev/10e3d0212100
Comment 18 David Bolter [:davidb] 2012-06-19 08:05:49 PDT
(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] 2012-06-19 08:30:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.