Closed Bug 726097 Opened 12 years ago Closed 12 years ago

Hit testing broken

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla13

People

(Reporter: hub, Assigned: hub)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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: nobody → hub
Blocks: osxa11y
It seems that the bug is more generalized.
OS: Mac OS X → All
Summary: [Mac] Accessibility Inspector unable to access items → Hit testing broken
This patch fix the bug on Mac. It should fix it on Windows. I have a mochitest coming.
OS: All → Mac OS X
Attachment #597274 - Flags: review?(surkov.alexander)
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
(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?
(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
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/
(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 ?
(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
Attachment #597274 - Attachment is obsolete: true
Attachment #597274 - Flags: review?(surkov.alexander)
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 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)
(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.
Attachment #597569 - Attachment is obsolete: true
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)
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
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.
(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.
(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.
(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
(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
Attachment #597920 - Attachment is obsolete: true
Attachment #597920 - Flags: review?(surkov.alexander)
Attachment #598929 - Flags: review?(surkov.alexander)
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+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a374a3bb0bc2

I'll file a bug for the stuff that is not regression.
Target Milestone: --- → mozilla13
see bug 729006
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
https://hg.mozilla.org/mozilla-central/rev/a374a3bb0bc2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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"?
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120222 Firefox/13.0a1
Status: RESOLVED → VERIFIED
Depends on: 729861
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: