The default bug view has changed. See this FAQ.

Hit testing broken

VERIFIED FIXED in mozilla13

Status

()

Core
Disability Access APIs
--
blocker
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: hub, Assigned: hub)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla13
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → hub
Blocks: 342989
(Assignee)

Comment 1

5 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)

Updated

5 years ago
Duplicate of this bug: 726520
(Assignee)

Comment 3

5 years ago
Created attachment 597274 [details] [diff] [review]
Use the right doc accessible when hit testing. r=
(Assignee)

Comment 4

5 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

5 years ago
Attachment #597274 - Flags: review?(surkov.alexander)

Comment 5

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
Created attachment 597569 [details] [diff] [review]
Use the right doc accessible when hit testing. r=
(Assignee)

Updated

5 years ago
Attachment #597274 - Attachment is obsolete: true
Attachment #597274 - Flags: review?(surkov.alexander)
(Assignee)

Comment 12

5 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

5 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

5 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

5 years ago
Created attachment 597920 [details] [diff] [review]
Use the right doc accessible when hit testing. Add proper mochi-test.
(Assignee)

Updated

5 years ago
Attachment #597569 - Attachment is obsolete: true
(Assignee)

Comment 16

5 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

5 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

5 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

5 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

5 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

5 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

5 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

5 years ago
Created attachment 598929 [details] [diff] [review]
Use the right doc accessible when hit testing. Add proper mochi-test.
(Assignee)

Updated

5 years ago
Attachment #597920 - Attachment is obsolete: true
Attachment #597920 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #598929 - Flags: review?(surkov.alexander)

Comment 24

5 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

5 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

5 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

5 years ago
see bug 729006

Comment 28

5 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
https://hg.mozilla.org/mozilla-central/rev/a374a3bb0bc2
Status: NEW → RESOLVED
Last Resolved: 5 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"?

Comment 31

5 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

5 years ago
Depends on: 729861

Updated

5 years ago
Blocks: 659589
You need to log in before you can comment on or make changes to this bug.