Closed Bug 569425 Opened 10 years ago Closed 9 years ago

13 intermittent failures in accessible/tests/mochitest/states/test_aria_imgmap.html on Linux

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: tnikkel)

References

Details

(Keywords: intermittent-failure, Whiteboard: [screensaver])

Attachments

(3 files, 2 obsolete files)

This morning there have been a bunch of builds with 13 failures in chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html :

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275409054.1275410937.30954.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2010/06/01 09:17:34  

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275410181.1275411906.2751.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2010/06/01 09:36:21  

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275417655.1275418971.30466.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/06/01 11:40:55  


2391 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for t1
2393 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  't1' !got '0', expected 'editable'
2395 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for t2
2397 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  't2' !got '0', expected 'editable'
2399 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for rb1
2400 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  'rb1' !got '0', expected 'checked, checkable'
2402 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for rb2
2403 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  'rb2' !got '0', expected 'checkable'
2406 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for cb1
2407 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  'cb1' !got '0', expected 'checked, checkable'
2409 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can't get accessible for cbox
2410 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  'cbox' !got '0', expected 'collapsed, haspopup'
2411 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  'cbox' !got '0', expected 'expandable'
Seems to be perfectly correlated with (later) occurrences of bug 557456.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275409330.1275411333.32758.gz
Rev3 Fedora 12 mozilla-central debug test mochitest-other on 2010/06/01 09:22:10
s: talos-r3-fed-020

And not just bug 557456, also in the neighborhood of a sudden spate of bug 569238 and bug 569237 and bug 563663.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275428093.1275429395.8455.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/06/01 14:34:53
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275466109.1275467885.2371.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/06/02 01:08:29
Blocks: 438871
Whiteboard: [orange]
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275512778.1275516813.23983.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2010/06/02 14:06:18
s: talos-r3-fed64-026
Looked this over with Ehsan... when someone (probably me) gets back to this, a good place to look is just before the call to GetAreaAccessible in nsAccessibilityService::GetAccessible -- probably bad assumptions about frames (possibly related to recent lazy frame construction?)
This is probably the same problem bug 544537. None of the conventional methods (waiting for load event, waiting for focus, flushing using offsetLeft, and even using nsIDOMWindowUtils.processUpdates to try to force a paint) can force the primary frame for the area to be setup. There are a few tricks you can do to force them to setup, outlined in bug 564063 comments 1-3. Probably the simplest is to just dynamically insert a new child under the map element (doesn't have to be an area, any type will do).

I can take this bug, but can you answer the following question for me David?

I found there are two accessibility tests dealing with imagemaps (this one and tree/test_aria_imgmap.html), are there any other that will need the same treatment?

The special casing for areas in nsAccessibilityService::GetAccessible should be annotated with bug 135040 so that we can find it when that bug finally gets fixed. Are there any other places in accessibility that also need to be annotated?

Finally, when looking at this I came across http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/common.js#199 which looks like it would shadow the elm variable declared above, but I guess it's a javascript quirk that it doesn't. We should probably fix that.
What if we just use nsComputedStyle::GetStyleContextForElement instead of querying the frame for the style context?
I'm not sure I follow. Where are you suggesting we use nsComputedStyle::GetStyleContextForElement instead of looking for a frame?
(In reply to comment #11)

> I found there are two accessibility tests dealing with imagemaps (this one and
> tree/test_aria_imgmap.html), are there any other that will need the same
> treatment?

Do you talk about fix for mochitests only?
 
> The special casing for areas in nsAccessibilityService::GetAccessible should be
> annotated with bug 135040 so that we can find it when that bug finally gets
> fixed. Are there any other places in accessibility that also need to be
> annotated?

nsAccessibilityService and nsIAssessibilityService provides several methods for accessible getting. Accessible for area happens in nsAccessibilityService::GetAreaAccessible().

What kind of annotation do you like to see?

> Finally, when looking at this I came across
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/common.js#199
> which looks like it would shadow the elm variable declared above, but I guess
> it's a javascript quirk that it doesn't. We should probably fix that.

right, it should be fixed I think.
I filed bug 570322 to figure out and fix the root problem (in contrast to a workaround of the problem) and prevent a diluting of bug discussion with failed mochitests reports.
Depends on: 570322
(In reply to comment #14)
> I'm not sure I follow. Where are you suggesting we use
> nsComputedStyle::GetStyleContextForElement instead of looking for a frame?

http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1429

We seem to be relying on frames to get the style context.  After going through the code with David, it seemed to us that the weakframe check could be failing, causing the test to fail.
(In reply to comment #19)
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1429
> 
> We seem to be relying on frames to get the style context.  After going through
> the code with David, it seemed to us that the weakframe check could be failing,
> causing the test to fail.

If I'm correct then yes !weakFrame.GetFrame() would be true. We could get around this by just getting the style context, but the call to GetAreaAccessible just below would need another way to get the image frame/content that is associated with the area.
(In reply to comment #17)
> > I found there are two accessibility tests dealing with imagemaps (this one and
> > tree/test_aria_imgmap.html), are there any other that will need the same
> > treatment?
> 
> Do you talk about fix for mochitests only?

Any kind of test that uses imagemaps, because it might need the same fix.

> What kind of annotation do you like to see?

Just add "bug 135040" in a comment somewhere, so that an mxr search for "135040" can find all the places in the code that have workaround behaviour for bug 135040, so we can fix them when bug 135040 gets fixed.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → tnikkel
Attachment #449473 - Flags: review?(dbaron)
Comment on attachment 449473 [details] [diff] [review]
patch

r=dbaron

(It's not obvious to me why bug 135040 is the only way to fix this, but this seems ok.)
Attachment #449473 - Flags: review?(dbaron) → review+
I actually have another idea for fixing this better, but I want to try it out first. So I will hold off on landing this patch.
Comment on attachment 449473 [details] [diff] [review]
patch


>+    //XXX We append a new child to the map element to force the areas to setup
>+    // their primary frames because flushing won't do it (bug 135040).
>+    var map = document.getElementById("ariaMap");
>+    map.appendChild(document.createTextNode(" "));
>+

since it's not a fix of real problem then todo() must be here.
(In reply to comment #27)
> (From update of attachment 449473 [details] [diff] [review])
> 
> >+    //XXX We append a new child to the map element to force the areas to setup
> >+    // their primary frames because flushing won't do it (bug 135040).
> >+    var map = document.getElementById("ariaMap");
> >+    map.appendChild(document.createTextNode(" "));
> >+
> 
> since it's not a fix of real problem then todo() must be here.

todo can't be used here, since it will cause a failure if the condition is true (i.e., it expects something that is always false).
Comment on attachment 449473 [details] [diff] [review]
patch

Actually, this patch won't solve the problem. Inserting a child into the map element will force the image to update if it is already hooked up to the map, but won't force the image to hook up to the map in the first place. We will just have to synthesize a mouse move instead. Still thinking if we can use a better solution though...
Attachment #449473 - Attachment is obsolete: true
I noticed that accessibility duplicates the code to find the image map for an image with usemap. We should just use the already existing nsImageMapUtils::FindImageMap.
Attachment #449557 - Flags: review?(surkov.alexander)
As I mentioned above just changing the DOM under the map element won't work. So we need to send a synth mouse event. While bug 135040 doesn't strictly cover this problem, either the fix for that bug will also fix this problem, or we can file a new bug when bug 135040 gets fixed. So I'm using bug 135040 as a tracking bug for various image map issues that could be fixed in that bug.
Attachment #449560 - Flags: review?(dbaron)
Attachment #449561 - Flags: review?(surkov.alexander)
(In reply to comment #25)
> I actually have another idea for fixing this better, but I want to try it out
> first.

My idea is more involved then I thought, so it will have to wait.
Comment on attachment 449561 [details] [diff] [review]
Don't make it look like we are shadowing a var.

r=me, thank you for fixing this
Attachment #449561 - Flags: review?(surkov.alexander) → review+
Attachment #449557 - Flags: review?(surkov.alexander) → review-
Comment on attachment 449557 [details] [diff] [review]
Make accessibility use the same method as layout and content to find imagemaps.

>+  nsAutoString mapElmName;
>+  content->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::usemap,
>+                   mapElmName);
>+  nsCOMPtr<nsIDOMHTMLMapElement> mapElm =
>+    nsImageMapUtils::FindImageMap(content->GetCurrentDoc(), mapElmName);

This is a bug 557768, it breaks builds like Thunderbird.
Attachment #449557 - Attachment is obsolete: true
Attachment #449560 - Flags: review?(dbaron) → review+
Landed the three patches here
http://hg.mozilla.org/mozilla-central/rev/213ff7652a1d
http://hg.mozilla.org/mozilla-central/rev/0f7e3b92bf6f
http://hg.mozilla.org/mozilla-central/rev/2f539cc84d97

But I backed out the work around in the mochitests
http://hg.mozilla.org/mozilla-central/rev/0bf458761ea2
http://hg.mozilla.org/mozilla-central/rev/60c2ea6c971a

I backed that out because dbaron mentioned an issue with it that is specific to a11y. Usually an image hooks up to an image map when it paints or when it gets some mouse event over it. For a11y a paint or mouse event of the image may never happen (say if the image is scrolled out of view) but something could ask for the accessible for an area.

It wouldn't be too hard to have a11y ask the image to look for an image map when it builds an accessible for the image. However if the accessible for an area is asked for and the image doesn't have an accessible we don't have any easy way to get the associated image to tell it to hook up to the image map.
Depends on: 569237
(In reply to comment #41)
> But I backed out the work around in the mochitests
> http://hg.mozilla.org/mozilla-central/rev/0bf458761ea2
> http://hg.mozilla.org/mozilla-central/rev/60c2ea6c971a
> 
> I backed that out because dbaron mentioned an issue with it that is specific to
> a11y. Usually an image hooks up to an image map when it paints or when it gets
> some mouse event over it. For a11y a paint or mouse event of the image may
> never happen (say if the image is scrolled out of view) but something could ask
> for the accessible for an area.

Yep. I thought your work around synthesized a mouse event -- I guess not soon enough? (Interesting that we wait until paint to hook the image up to the map, that explains some pain I had earlier this year)
(In reply to comment #43)
> Yep. I thought your work around synthesized a mouse event -- I guess not soon
> enough?

The workaround should make the test pass, that is not the issue. The problem is that the test may be identifying a real issue with how a11y handles areas. If an accessible for an area is asked for before the corresponding image is painted (or has a mouse event over it), the request will fail.
(In reply to comment #44)
> (In reply to comment #43)
> > Yep. I thought your work around synthesized a mouse event -- I guess not soon
> > enough?
> 
> The workaround should make the test pass, that is not the issue. The problem is
> that the test may be identifying a real issue with how a11y handles areas. If
> an accessible for an area is asked for before the corresponding image is
> painted (or has a mouse event over it), the request will fail.

Oh yes, absolutely. The workaround would be temporary.
We're ok with temporary workaround here. There's a bug 570322 for the issue. Just please make sure to add todo(false) in mochitests to being reminded. And if synthesize mouse work here then it's a way to go. If these areas are out of screen then please scroll them into visible area. Here we need to have working mochitest. Let's deal with the real problem separately.
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276146860.1276150270.4690.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2010/06/09 22:14:20

s: talos-r3-fed64-027
PROCESS-CRASH | automation.py | application crashed (minidump found)
PROCESS-CRASH | automation.py | application crashed (minidump found)
2506 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for t1
2508 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'t1\' !got \'0\', expected \'editable\'
2510 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for t2
2512 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'t2\' !got \'0\', expected \'editable\'
2514 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for rb1
2515 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'rb1\' !got \'0\', expected \'checked, checkable\'
2517 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for rb2
2518 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'rb2\' !got \'0\', expected \'checkable\'
2521 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for cb1
2522 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'cb1\' !got \'0\', expected \'checked, checkable\'
2524 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for cbox
2525 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'cbox\' !got \'0\', expected \'collapsed, haspopup\'
2526 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'cbox\' !got \'0\', expected \'expandable\'
435 ERROR TEST-UNEXPECTED-FAIL | /tests/modules/plugin/test/test_painting.html | Test timed out.
philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276148455.1276150342.4850.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/06/09 22:40:55

s: talos-r3-fed64-028
PROCESS-CRASH | automation.py | application crashed (minidump found)
PROCESS-CRASH | automation.py | application crashed (minidump found)
2506 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for t1
2508 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'t1\' !got \'0\', expected \'editable\'
2510 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for t2
2512 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'t2\' !got \'0\', expected \'editable\'
2514 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for rb1
2515 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'rb1\' !got \'0\', expected \'checked, checkable\'
2517 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for rb2
2518 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'rb2\' !got \'0\', expected \'checkable\'
2521 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for cb1
2522 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'cb1\' !got \'0\', expected \'checked, checkable\'
2524 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | Can\'t get accessible for cbox
2525 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong state bits for  \'cbox\' !got \'0\', expected \'collapsed, haspopup\'
2526 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/states/test_aria_imgmap.html | wrong extra state bits for  \'cbox\' !got \'0\', expected \'expandable\'
435 ERROR TEST-UNEXPECTED-FAIL | /tests/modules/plugin/test/test_painting.html | Test timed out.
Timothy, what's the status of the bug? If it fix the problem then I would ask to add todo(false) when accessible failed created, refer to bug 570322 and land it.
Yeah, sorry, I haven't had a chance to land yet. Someone else can make those changes and land if they want.
Landed the mochitest workaround with a todo and mention of bug 570322
http://hg.mozilla.org/mozilla-central/rev/b69e999098ce
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This is obviously not fixed. I'll have to have another look at this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hmm, actually the above 3 comments are all one log, so it only happened once since the landing.
Depends on: 585286
No longer depends on: 569237
Whiteboard: [orange] → [orange][screensaver]
Bug 585286 was fixed today, which should help this (given that it is marked [scrensaver]).
I think this is fixed.
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [orange][screensaver] → [screensaver]
You need to log in before you can comment on or make changes to this bug.