Closed Bug 569425 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: tnikkel)

References

Details

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

Attachments

(3 files, 2 obsolete files)

Annotate one place in a11y that relies on the current image map primary frame setup.
(deleted), patch
Details | Diff | Splinter Review
Use a synth mouse event to force image map setup in test.
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
Don't make it look like we are shadowing a var.
(deleted), patch
surkov
: review+
Details | Diff | Splinter Review
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) (deleted) — — 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: 15 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: 15 years ago → 14 years ago
Resolution: --- → FIXED
Whiteboard: [orange][screensaver] → [screensaver]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: