Hit testing broken on Google Search results page

RESOLVED FIXED in Firefox 36

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Jamie, Assigned: surkov)

Tracking

(Blocks: 1 bug, {regression})

35 Branch
mozilla38
x86_64
Windows 8.1
regression
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Str:
1. Perform a Google Search; e.g. foo bar.
2. Get the coordinates for the centre screen point of the link for the first search result.
3. From the top level accessible, hit test with these coordinates. Hit test down the tree until you can't go any further.
Actual: The accessible is a (fairly distant) ancestor of the link.
Expected: The accessible should be the link.

Hit testing is stopping way too early for some reason. The practical upshot is that mouse tracking is quite broken in NVDA. This is quite problematic for users with partial sight and developers.

This does not occur with all pages. However, we've had reports that it occurs elsewhere, though I don't have any specific URLs. I've no idea why it occurs on some pages and not others.

Worked fine in Firefox 34, but broken in Firefox 35.

NVDA issue ticket: http://community.nvda-project.org/ticket/4825
(Assignee)

Comment 1

3 years ago
It'd be good to have a regression range, I doubt that a11y changes were guilty in that. Is there anybody willing to help with regression finding?
(Assignee)

Updated

3 years ago
Blocks: 659589
First broken build is the 2014-08-23 build. Regression range is: 
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b9dd32d1e16&tochange=64c4ec2df3d4
The only non-GAIA accessibility-related landing in that period is bug 1047696.
OK, scrap those last two comments. I was chasing a ghost that finally turned on me with the middle finger.

Of course, the correct regression range has to be in the Firefox 35 cycle, not the 34 one, if the 35 release shows the problem. And it is!

Last good build is 2014-09-16, first bad build is 2014-09-17. Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3b7921328fc1&tochange=8252eae8278c
(Assignee)

Comment 5

3 years ago
there's a div having zero width, the alg stops on it

just in case DOMi script to show all findings:

var links = [];
function findLink(a)
{
  if (tree.mAccService.getStringRole(a.role) == "link" &&
      a.name && a.name.indexOf("Foobar - Wiki") != -1) {
    links.push(a);
  }
  return false;
}
tree.clearSearch();
tree.search(accessible, findLink);

var xObj = {}, yObj = {}, wObj = {}, hObj = {};
links[0].getBounds(xObj, yObj, wObj, hObj);

var x = xObj.value + wObj.value / 2;
var y = yObj.value + hObj.value / 2;

var acc = accessible.getChildAtPoint(x, y);
while (acc) {
  tree.search(accessible, function(a) { return acc == a });
  acc = acc.getChildAtPoint(x, y);
}
(Assignee)

Comment 6

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #4)

> Last good build is 2014-09-16, first bad build is 2014-09-17. Regression
> range:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=3b7921328fc1&tochange=8252eae8278c

Is there a chance to find exact regression? It appears we regressed because of layout stuff, a11y code simply calls nsLayoutUtils::GetAllInFlowRectsUnion to find boundaries (note, Element.offsetWidth also returns 0).
(In reply to alexander :surkov from comment #6)
> Is there a chance to find exact regression? It appears we regressed because
> of layout stuff, a11y code simply calls
> nsLayoutUtils::GetAllInFlowRectsUnion to find boundaries (note,
> Element.offsetWidth also returns 0).

Result from hg bisect session:
The first bad revision is:
changeset:   205509:627909e0506e
user:        Alexander Surkov <surkov.alexander@gmail.com>
date:        Tue Sep 16 13:30:23 2014 -0400
summary:     Bug 1064877 - dexpcomify Accessible class, r=tbsaunde

My bisection also shows that I am building with one changeset before this one along the way, which is still good, so this is definitely confirmed to be the bad revision that introduces the problem.
(Assignee)

Comment 8

3 years ago
Created attachment 8552511 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Attachment #8552511 - Flags: review?(yzenevich)
FWIW, this patch does not appear to solve the problem. With this patch applied, I no longer hear NVDA's error sound, but the whole document area no longer speaks any content when mousing over it. NVDA normally speaks the element under the mouse, but now is completely silent.
(Assignee)

Comment 10

3 years ago
Comment on attachment 8552511 [details] [diff] [review]
patch

canceling per comment #9. I'd be great to have more input what can be wrong.
Attachment #8552511 - Flags: review?(yzenevich)
Before this patch, mousing over the document area while NVDA was running would give constant error sounds (descending piano tone). Now, NVDA just sees nothing, but also doesn't give an error message any more. It feels like the whole document area is an empty area for hit testing.
(Assignee)

Comment 12

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #11)
>  It feels like
> the whole document area is an empty area for hit testing.

it's strange since I was able to find that link in DOMi following Jamie's steps. So that has to be Windows or NVDA specific.
(Assignee)

Comment 13

3 years ago
just finished a build on windows and it works fine. Can it be something wrong with your build, Marco?
Definitely confirmed with a new fresh build with today's code and your patch applied. The whole document area is blank to NVDA's mouse movement.

Can you kick off a try-server build and post the link here so both jamie and I can try that one?
(Assignee)

Comment 15

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #14)
> Definitely confirmed with a new fresh build with today's code and your patch
> applied. The whole document area is blank to NVDA's mouse movement.

and it works without patch, right?

> Can you kick off a try-server build and post the link here so both jamie and
> I can try that one?

here's the previous one I made https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-39a410f1db85/
Comment on attachment 8552511 [details] [diff] [review]
patch

OK, with the try-server build, mousing over document content works again. So I'm fine with it. :) Don't know why my local build shows such quirky behavior. Let's ignore that and proceed with the review.
Attachment #8552511 - Flags: feedback+
(Assignee)

Updated

3 years ago
Attachment #8552511 - Flags: review?(yzenevich)
Comment on attachment 8552511 [details] [diff] [review]
patch

Review of attachment 8552511 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good thank you!
Attachment #8552511 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/ec1efe1d6f42
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 19

3 years ago
Thanks!

Marco/Alex, any chance of a backport? This is pretty serious for partially sighted users.
(Assignee)

Comment 20

3 years ago
Comment on attachment 8552511 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1064877
[User impact if declined]: broken mouse reading mode in NVDA
[Describe test coverage new/current, TreeHerder]: mochitest-a11y
[Risks and why]: no risk, partial backout
[String/UUID change made/needed]:no
Attachment #8552511 - Flags: approval-mozilla-aurora?
Comment on attachment 8552511 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1064877
[User impact if declined]: broken mouse reading in NVDA, possibly magnifiers affected, too
[Describe test coverage new/current, TreeHerder]: mochitest-a11y
[Risks and why]: No risk, partial backout
[String/UUID change made/needed]: No.
Attachment #8552511 - Flags: approval-mozilla-beta?
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8552511 - Flags: approval-mozilla-beta?
Attachment #8552511 - Flags: approval-mozilla-beta+
Attachment #8552511 - Flags: approval-mozilla-aurora?
Attachment #8552511 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Keywords: checkin-needed, regression
https://hg.mozilla.org/releases/mozilla-aurora/rev/8af51e438e0e
https://hg.mozilla.org/releases/mozilla-beta/rev/e63d5e1865db
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Keywords: regression
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.