Closed
Bug 726097
Opened 12 years ago
Closed 12 years ago
Hit testing broken
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
VERIFIED
FIXED
mozilla13
People
(Reporter: hub, Assigned: hub)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
11.11 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
Since the fix for bug 672504 went in, the Accessibility Inspect on MacOS no longer work with Firefox. Note: the first version that landed for bug 672504 didn't cause the regression. The later one (that I finished on Linux) seems to do. I'm not sure if the impact is Mac specific either.
Assignee | ||
Comment 1•12 years ago
|
||
It seems that the bug is more generalized.
OS: Mac OS X → All
Summary: [Mac] Accessibility Inspector unable to access items → Hit testing broken
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
This patch fix the bug on Mac. It should fix it on Windows. I have a mochitest coming.
OS: All → Mac OS X
Assignee | ||
Updated•12 years ago
|
Attachment #597274 -
Flags: review?(surkov.alexander)
Comment 5•12 years ago
|
||
So that means GetFrameFromPoint walks through documents (including chrome-content boundaries taking into account bug 726520) and that means we don't need next block. (In reply to Hub Figuiere [:hub] from comment #4) > This patch fix the bug on Mac. It should fix it on Windows. I have a > mochitest coming. please make sure you test chrome-content and content-content boundaries and image map stuffs
Comment 6•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #4) > This patch fix the bug on Mac. It should fix it on Windows. I have a > mochitest coming. Sorry, missed you on IRC. Any chance of a try build so I can check on Windows? Btw, out of curiosity, why did you set the OS to Mac OS X if you think the fix applies to Windows as well?
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to James Teh [:Jamie] from comment #6) > Btw, out of curiosity, why did you set the OS to Mac OS X if you think the > fix applies to Windows as well? That's actually a bug in Firefox. I didn't change it, it just changed it back. Side effect of having stuff open in various tab / browser and session restore.
OS: Mac OS X → All
Assignee | ||
Comment 8•12 years ago
|
||
Jamie: There should be some Windows builds soon under this directory: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/hfiguiere@mozilla.com-04c1631d1219/
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to alexander :surkov from comment #5) > So that means GetFrameFromPoint walks through documents (including > chrome-content boundaries taking into account bug 726520) and that means we > don't need next block. The block that starts at line 861 ?
Comment 10•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #9) > (In reply to alexander :surkov from comment #5) > > So that means GetFrameFromPoint walks through documents (including > > chrome-content boundaries taking into account bug 726520) and that means we > > don't need next block. > > The block that starts at line 861 ? http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#859
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #597274 -
Attachment is obsolete: true
Attachment #597274 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 12•12 years ago
|
||
Comment on attachment 597569 [details] [diff] [review] Use the right doc accessible when hit testing. r= removed the unneeded code as per request.
Attachment #597569 -
Flags: review?(surkov.alexander)
Comment 13•12 years ago
|
||
Comment on attachment 597569 [details] [diff] [review] Use the right doc accessible when hit testing. r= approach looks right but I need to be sure it covers all cases, please add mochitests.
Attachment #597569 -
Flags: review?(surkov.alexander)
Comment 14•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #8) > Jamie: There should be some Windows builds soon under this directory: This build fixes the problem for me.
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #597569 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 597920 [details] [diff] [review] Use the right doc accessible when hit testing. Add proper mochi-test. here is the fix WITH the mochi-test. I have test the mochi-test separately from the fix to make sure that it detects the bug and detect it is fixed. Try build is pending.
Attachment #597920 -
Flags: review?(surkov.alexander)
Comment 17•12 years ago
|
||
1) please move test_childAtPoint.html/xul to hittest renaming them to test_general 2) add test_browserui.xul 2.1) add tests you added 2.2) test content document having two tabs (from chrome get content, and from content of inactive tab get content) 3) add iframe testing to test_general.html 4) add image map testing to test_general.html
Assignee | ||
Comment 18•12 years ago
|
||
While I have no problem in principle to make the test suite better, this bug was open because of a *regression* I introduced. I also wrote a test to make sure to catch it. I think that requesting to write more test right now will just slow down the fixing and I believe it should be a separate issue that can be tackled at a more opportune time.
Comment 19•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #18) > While I have no problem in principle to make the test suite better, this bug > was open because of a *regression* I introduced. good > I also wrote a test to make > sure to catch it. I think that requesting to write more test right now will > just slow down the fixing and I believe it should be a separate issue that > can be tackled at a more opportune time. Adding a test for this *regression* is really wanted. But I'm asking to test the code paths you touched to avoid *new* regressions. There are two options 1) reviewer downloads patch, builds and makes manual testing 2) assignee provides reasonable set of automated tests to make sure 2a) the bug is fixed 2b) no new regressions I prefer 2nd. You just was unfortunate that the code your patch removes is not well tested.
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to alexander :surkov from comment #19) > (In reply to Hub Figuiere [:hub] from comment #18) > > While I have no problem in principle to make the test suite better, this bug > > was open because of a *regression* I introduced. > > good > > > I also wrote a test to make > > sure to catch it. I think that requesting to write more test right now will > > just slow down the fixing and I believe it should be a separate issue that > > can be tackled at a more opportune time. > > Adding a test for this *regression* is really wanted. But I'm asking to test > the code paths you touched to avoid *new* regressions. Then I can backout the code removal you requested and we make that the object of a new bug and test. And live that one-line WITH mochitest that is the actual regression I checked, and that Jamie verified as fixing his issue (same bug, different platform). I really want to get that fix ASAP.
Comment 21•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #20) > I really want to get that fix ASAP. spending an extra hour to finish tests shouldn't harm anybody
Comment 22•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #20) > Then I can backout the code removal you requested and we make that the > object of a new bug and test. And live that one-line WITH mochitest that is > the actual regression I checked, and that Jamie verified as fixing his issue > (same bug, different platform). if you really feel unhappy to finish things here then please file follow up bug and fix 1), 2), 2.1) items of comment #17
Assignee | ||
Comment 23•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #597920 -
Attachment is obsolete: true
Attachment #597920 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•12 years ago
|
Attachment #598929 -
Flags: review?(surkov.alexander)
Comment 24•12 years ago
|
||
Comment on attachment 598929 [details] [diff] [review] Use the right doc accessible when hit testing. Add proper mochi-test. Review of attachment 598929 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.cpp @@ -880,5 @@ > - > - // The point is in this accessible but not in a child. We are allowed to > - // return |this| as the answer. > - return accessible; > - } you weren't supposed to remove it here? ::: accessible/tests/mochitest/hittest/Makefile.in @@ +15,5 @@ > +# The Original Code is mozilla.org code. > +# > +# The Initial Developer of the Original Code is > +# Mozilla Foundation. > +# Portions created by the Initial Developer are Copyright (C) 2010 2012 @@ +19,5 @@ > +# Portions created by the Initial Developer are Copyright (C) 2010 > +# the Initial Developer. All Rights Reserved. > +# > +# Contributor(s): > +# Fernando Herrera <fherrera@onirica.com> (original author) your name ::: accessible/tests/mochitest/hittest/test_browser.html @@ +25,5 @@ > + hititem.getBounds(hitX, hitY, hitWidth, hitHeight); > + > + var tgtX = deltaX + (hitWidth.value / 2); > + var tgtY = deltaY + (hitHeight.value / 2); > + var frame = getApplicationAccessible().firstChild; frame -> rootAcc, use getRootAccessible() @@ +27,5 @@ > + var tgtX = deltaX + (hitWidth.value / 2); > + var tgtY = deltaY + (hitHeight.value / 2); > + var frame = getApplicationAccessible().firstChild; > + var docAcc = getAccessible(document); > + var outerDocAcc = getAccessible(docAcc.parent); just do docAcc.parent, no need to wrap it by getAccessible() @@ +41,5 @@ > + ok(hitAcc == docAcc, "Hit match at " + tgtX + "," + tgtY + > + ". Found: " + prettyName(hitAcc)); > + hitAcc = docAcc.getChildAtPoint(tgtX, tgtY); > + ok(hitAcc == hittest, "Hit match at " + tgtX + "," + tgtY + > + ". Found: " + prettyName(hitAcc)); replace ok() on is()
Attachment #598929 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 25•12 years ago
|
||
Comment on attachment 598929 [details] [diff] [review] Use the right doc accessible when hit testing. Add proper mochi-test. Review of attachment 598929 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/base/nsAccessible.cpp @@ -880,5 @@ > - > - // The point is in this accessible but not in a child. We are allowed to > - // return |this| as the answer. > - return accessible; > - } My bad. putting it back.
Assignee | ||
Comment 26•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a374a3bb0bc2 I'll file a bug for the stuff that is not regression.
Target Milestone: --- → mozilla13
Assignee | ||
Comment 27•12 years ago
|
||
see bug 729006
Comment 28•12 years ago
|
||
Comment on attachment 598929 [details] [diff] [review] Use the right doc accessible when hit testing. Add proper mochi-test. Review of attachment 598929 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/tests/mochitest/hittest/test_browser.html @@ +41,5 @@ > + ok(hitAcc == docAcc, "Hit match at " + tgtX + "," + tgtY + > + ". Found: " + prettyName(hitAcc)); > + hitAcc = docAcc.getChildAtPoint(tgtX, tgtY); > + ok(hitAcc == hittest, "Hit match at " + tgtX + "," + tgtY + > + ". Found: " + prettyName(hitAcc)); btw, consider to use testChildAtPoint to keep code unified
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a374a3bb0bc2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 30•12 years ago
|
||
I can verify that tis again works for me in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120222 Firefox/13.0a1. jamie, can you verify this, too, with your steps and then set the status field to "Verifed"?
Comment 31•12 years ago
|
||
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120222 Firefox/13.0a1
Status: RESOLVED → VERIFIED
Updated•12 years ago
|
Blocks: boundsa11y
You need to log in
before you can comment on or make changes to this bug.
Description
•