Last Comment Bug 726097 - Hit testing broken
: Hit testing broken
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 All
: -- blocker (vote)
: mozilla13
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
: 726520 (view as bug list)
Depends on: 729861
Blocks: osxa11y 659589
  Show dependency treegraph
 
Reported: 2012-02-10 11:16 PST by Hubert Figuiere [:hub]
Modified: 2012-05-12 20:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use the right doc accessible when hit testing. r= (1.68 KB, patch)
2012-02-14 18:38 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Use the right doc accessible when hit testing. r= (2.56 KB, patch)
2012-02-15 14:49 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Use the right doc accessible when hit testing. Add proper mochi-test. (5.43 KB, patch)
2012-02-16 11:38 PST, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Use the right doc accessible when hit testing. Add proper mochi-test. (11.11 KB, patch)
2012-02-20 11:29 PST, Hubert Figuiere [:hub]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Hubert Figuiere [:hub] 2012-02-10 11:16:51 PST
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.
Comment 1 Hubert Figuiere [:hub] 2012-02-12 22:46:34 PST
It seems that the bug is more generalized.
Comment 2 Hubert Figuiere [:hub] 2012-02-12 22:46:56 PST
*** Bug 726520 has been marked as a duplicate of this bug. ***
Comment 3 Hubert Figuiere [:hub] 2012-02-14 18:38:08 PST
Created attachment 597274 [details] [diff] [review]
Use the right doc accessible when hit testing. r=
Comment 4 Hubert Figuiere [:hub] 2012-02-14 18:38:50 PST
This patch fix the bug on Mac. It should fix it on Windows. I have a mochitest coming.
Comment 5 alexander :surkov 2012-02-14 18:56:06 PST
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 James Teh [:Jamie] 2012-02-14 20:02:33 PST
(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?
Comment 7 Hubert Figuiere [:hub] 2012-02-14 21:23:08 PST
(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.
Comment 8 Hubert Figuiere [:hub] 2012-02-14 23:38:54 PST
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/
Comment 9 Hubert Figuiere [:hub] 2012-02-14 23:59:18 PST
(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 alexander :surkov 2012-02-15 00:31:33 PST
(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
Comment 11 Hubert Figuiere [:hub] 2012-02-15 14:49:48 PST
Created attachment 597569 [details] [diff] [review]
Use the right doc accessible when hit testing. r=
Comment 12 Hubert Figuiere [:hub] 2012-02-15 14:52:02 PST
Comment on attachment 597569 [details] [diff] [review]
Use the right doc accessible when hit testing. r=

removed the unneeded code as per request.
Comment 13 alexander :surkov 2012-02-15 18:47:41 PST
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.
Comment 14 James Teh [:Jamie] 2012-02-15 20:17:06 PST
(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.
Comment 15 Hubert Figuiere [:hub] 2012-02-16 11:38:53 PST
Created attachment 597920 [details] [diff] [review]
Use the right doc accessible when hit testing. Add proper mochi-test.
Comment 16 Hubert Figuiere [:hub] 2012-02-16 11:40:45 PST
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.
Comment 17 alexander :surkov 2012-02-16 19:15:49 PST
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
Comment 18 Hubert Figuiere [:hub] 2012-02-17 13:08:16 PST
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 alexander :surkov 2012-02-17 20:42:36 PST
(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.
Comment 20 Hubert Figuiere [:hub] 2012-02-17 21:51:00 PST
(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 alexander :surkov 2012-02-18 01:43:14 PST
(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 alexander :surkov 2012-02-18 02:19:20 PST
(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
Comment 23 Hubert Figuiere [:hub] 2012-02-20 11:29:04 PST
Created attachment 598929 [details] [diff] [review]
Use the right doc accessible when hit testing. Add proper mochi-test.
Comment 24 alexander :surkov 2012-02-20 18:26:30 PST
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()
Comment 25 Hubert Figuiere [:hub] 2012-02-20 20:13:13 PST
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.
Comment 26 Hubert Figuiere [:hub] 2012-02-20 21:19:46 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a374a3bb0bc2

I'll file a bug for the stuff that is not regression.
Comment 27 Hubert Figuiere [:hub] 2012-02-20 21:23:30 PST
see bug 729006
Comment 28 alexander :surkov 2012-02-21 01:43:54 PST
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 Ed Morley [:emorley] 2012-02-22 04:46:53 PST
https://hg.mozilla.org/mozilla-central/rev/a374a3bb0bc2
Comment 30 Marco Zehe (:MarcoZ) on PTO until August 15 2012-02-22 08:52:59 PST
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 James Teh [:Jamie] 2012-02-22 20:41:03 PST
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120222 Firefox/13.0a1

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