Closed Bug 890940 Opened 7 years ago Closed 7 years ago

[AccessFu] Visual bounds box is very wrong

Categories

(Core :: Disability Access APIs, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: maxli, Assigned: maxli)

Details

Attachments

(1 file, 7 obsolete files)

The visual box we draw around the element with current accessibility focus is very wrong. The bounds we report to TalkBack (and thus its rectangle) are correct.

This works fine in the July 4th nightly, but doesn't starting from the 5th.
This was regressed with changeset b11787e3544b of bug 866265.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → maxli
Attachment #772620 - Flags: review?(eitan)
Comment on attachment 772620 [details] [diff] [review]
Patch

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

Besides that thing below, this looks right.

::: accessible/src/jsat/content-script.js
@@ +66,5 @@
>      case 'moveToPoint':
>        if (!this._ppcp) {
>          this._ppcp = Utils.getPixelsPerCSSPixel(content);
>        }
> +      let vp = Utils.getViewport(Utils.win) || { zoom: 1.0 };

I think the viewport is a chrome js Android thing, so this translation should be in AccessFu.jsm.
Attachment #772620 - Flags: review?(eitan)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #772620 - Attachment is obsolete: true
Attachment #772636 - Flags: review?(eitan)
Comment on attachment 772636 [details] [diff] [review]
Patch v2

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

cool beans.
Attachment #772636 - Flags: review?(eitan) → review+
https://hg.mozilla.org/mozilla-central/rev/e8eded157cf9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
This is still broken on some sites (e.g. news.ycombinator.com)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch (obsolete) — Splinter Review
The last patch mostly happened to work by coincidence. But I now understand better what changed with bug 866265, so this should be right.
Attachment #772636 - Attachment is obsolete: true
Attachment #775122 - Flags: review?(eitan)
Comment on attachment 775122 [details] [diff] [review]
Patch

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

Yep, this stuff is tricky :)

I have a suspicion that your change simply cancels out the Utils.getPixelsPerCSSPixel() change I put in a while back (in some conditions).
If this is the case, I would remove that method and go with window.devicePixelRatio that you use here. Then I would invert the aToCSSPixels argument.

The important part -
This needs to be tested in:
1. Android.
2. A high-DPI display like rMBP with the screen reader simulator extension.
3. A non-high-DPI display (I could do that).

::: accessible/src/jsat/AccessFu.jsm
@@ +582,3 @@
>  
> +    if (aToCSSPixels) {
> +      newBounds = newBounds.scale(1 / root.devicePixelRatio, 1 / root.devicePixelRatio);

Is this similar to Utils.getPixelsPerCSSPixel? Should we be scaling for both? If they are the same, perhaps you are simply scaling it back to the original ratio...

::: accessible/src/jsat/content-script.js
@@ +68,5 @@
>          this._ppcp = Utils.getPixelsPerCSSPixel(content);
>        }
>        moved = vc.moveToPoint(rule,
> +                             details.x * this._ppcp * content.devicePixelRatio,
> +                             details.y * this._ppcp * content.devicePixelRatio,

I think this._ppcp is the multiplicative inverse of content.devicePixelRatio. So they are actually just canceling each other out, no?
Attachment #775122 - Flags: review?(eitan)
Attached patch Patch fixed (obsolete) — Splinter Review
The above patch didn't work on a high DPI display. But here's a version that does. devicePixelRatio should be the same value as Utils.getPixelsPerCSSPixel, so for simplicity I used that instead.

I also tried this on a normal display, and it seemed to work too (which probably makes sense since all these values we're multiplying/dividing by should be 1 in that case).
Attachment #775122 - Attachment is obsolete: true
Attachment #776514 - Flags: review?(eitan)
Comment on attachment 776514 [details] [diff] [review]
Patch fixed

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

Still not 100% clear on the issues below. But it sounds like it works on all devices/platforms? How does Android use devicePixelRatio?

::: accessible/src/jsat/AccessFu.jsm
@@ +566,1 @@
>      let root = Utils.win;

nit, could you rename this in this patch? I don't know what I was thinking. Maybe 'win' instead..

@@ +566,2 @@
>      let root = Utils.win;
> +    let vp = Utils.getViewport(root) || { zoom: root.devicePixelRatio };

so if zoom is devicePixelRatio, and then scale below is the inverse, don't we end up in the same place? I'm not clear how this works.

@@ +620,5 @@
>        case 'mozAccessFuGesture':
> +        if (Utils.MozBuildApp === 'mobile/android') {
> +          aEvent.detail.x *= Utils.win.devicePixelRatio;
> +          aEvent.detail.y *= Utils.win.devicePixelRatio;
> +        }

The gesture info should remain as device pixels. Transform this when we go back to interrogating content, as far as I can tell this is only needed in moveToPoint.

::: accessible/src/jsat/content-script.js
@@ +69,4 @@
>        }
>        moved = vc.moveToPoint(rule,
> +                             details.x * this._ppcp,
> +                             details.y * this._ppcp,

So you already multiplied this in handleGesture in AccessFu.jsm. How is this working?
Attachment #776514 - Flags: review?(eitan)
Attached patch Patch v4 (obsolete) — Splinter Review
(In reply to Eitan Isaacson [:eeejay] from comment #12)

This should work on all platforms.

> so if zoom is devicePixelRatio, and then scale below is the inverse, don't
> we end up in the same place? I'm not clear how this works.

zoom is only devicePixelRatio if there's no viewport, i.e. on the desktop where there's no pinch-zooming. Otherwise the calculation will scale by the resolution change due to the pinch-zoom. I suppose this could be written slightly differently, but I think this way actually makes more intuitive sense with respect to how all these coordinates work (of course, all this coordinate business isn't too intuitive)

> So you already multiplied this in handleGesture in AccessFu.jsm. How is this
> working?

The event x and y coordinates are now affected by the widget scaling (on Android). So we actually need to multiply it twice for it work properly. Because of this, I think it makes more sense to move this multiplication to TouchAdapter (and have done so).
Attachment #776514 - Attachment is obsolete: true
Attachment #776727 - Flags: review?(eitan)
(In reply to Max Li [:maxli] from comment #13)
> Created attachment 776727 [details] [diff] [review]
> Patch v4
> 
> (In reply to Eitan Isaacson [:eeejay] from comment #12)
> 
> This should work on all platforms.
> 
> > so if zoom is devicePixelRatio, and then scale below is the inverse, don't
> > we end up in the same place? I'm not clear how this works.
> 
> zoom is only devicePixelRatio if there's no viewport, i.e. on the desktop
> where there's no pinch-zooming. Otherwise the calculation will scale by the
> resolution change due to the pinch-zoom.

That is why it was set to always be 1.

> I suppose this could be written
> slightly differently, but I think this way actually makes more intuitive
> sense with respect to how all these coordinates work (of course, all this
> coordinate business isn't too intuitive)
> 

I don't think this is intuitive:

>  let newBounds = bounds.scale(scale, scale).translate(offset.left, offset.top).
> 		scale(vp.zoom, vp.zoom);

You are doing: ((bounds * scale) + offset) * zoom
And since scale and zoom are inverse, in the non-Android case, you are simply doing:
bounds + (offset * zoom). Very clever, but also hardly readable :)

I would break this up into multiple lines with clear comments in each case about what is going on, for example:

// Bounds are in device pixels.
let bounds = ...
...
let dpr = win.devicePixelRatio;
let vp = Utils.getViewport(win);

// Add the browser offset in device pixels
bounds = bounds.translate(offset.left*dpr, offset.top*dpr). 

if (vp) {
  // Account for viewport zoom.
  bounds = bounds.scale(vp.zoom, vp.zoom);
}

if (aToCSSPixels) {
  // Convert from device pixels to CSS pixels
  bounds = bounds.scale(1/dpr, 1/dpr);
}

Not sure if that is exactly equivalent to what you were doing, but please guide us through every step of the way. Also, get rid of bad variable choices I made, like 'scale'. Thanks for getting rid of 'root. newBounds is redundant, etc.
Attached patch More descriptive patch (obsolete) — Splinter Review
Attachment #776727 - Attachment is obsolete: true
Attachment #776727 - Flags: review?(eitan)
Attachment #777116 - Flags: review?(eitan)
Fixed coordinate translation in TouchAdapter
Attachment #777116 - Attachment is obsolete: true
Attachment #777116 - Flags: review?(eitan)
Attachment #777178 - Flags: review?(eitan)
Comment on attachment 777178 [details] [diff] [review]
Patch with input parameters fixed

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

This looks more correct. Some parts are still puzzling to me like in Android where you take zoom into account on output, but not input. But if you tested this and it works in the 3 cases above, it is a good start. I'll need to test this further with b2g devices, but it doesn't have to wait for that. At least now I understand more whats going on, so b2g will be easier to account for later.

Please repost with the changes below, after that we should be good.

::: accessible/src/jsat/AccessFu.jsm
@@ +810,3 @@
>        Services.obs.notifyObservers(null, 'Gesture:LongPress',
> +                                   JSON.stringify({x: aMessage.x / dpr,
> +                                                   y: aMessage.y / dpr}));

This works fine in Android for now, but we should be accounting for content offset like we do in _adjustBounds. That is, of course, theoretical in Android since there is only one content process, and its offset is always 0. But I could see this being useful for other kinds of interactions like scroll that won't be Android specific. So we should unify the code path to go through _adjustBounds in all cases where we take content coordinates and translate them to top-chrome coordinates.

So this is what I think should happen:
1. _adjustBounds should go into the AccessFu object, and renamed adjustContentBounds.
2. It should take an argument telling if we want to sale with vp zoom or not.
3. AccessFu:ActivateContextMenu should provide the bounds of the current object.
4. activateContextMenu() should adjust the bounds and then get the x/y by calling center() on the bounds.

::: accessible/src/jsat/TouchAdapter.jsm
@@ +348,5 @@
>    getDistanceToCoord: function TouchPoint_getDistanceToCoord(aX, aY) {
>      return Math.sqrt(Math.pow(this.x - aX, 2) + Math.pow(this.y - aY, 2));
>    },
>  
> +  get scaleFactor() {

you could replace the getter with the dpr (or 1) after the first get.

::: accessible/src/jsat/content-script.js
@@ +97,5 @@
>    let rule = TraversalRules[details.rule];
>  
>    try {
>      if (!this._ppcp) {
> +      this._ppcp = content.devicePixelRatio;

No need to cache a simple attribute. Just use it directly like you do elsewhere.
Attachment #777178 - Flags: review?(eitan)
Attached patch Patch v6Splinter Review
Attachment #777178 - Attachment is obsolete: true
Attachment #777342 - Flags: review?(eitan)
Comment on attachment 777342 [details] [diff] [review]
Patch v6

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

This looks good!
Attachment #777342 - Flags: review?(eitan) → review+
https://hg.mozilla.org/mozilla-central/rev/843d948e138b
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.