1469877 - XUL accessible highlighter position is incorrect at different zoom levels

Status: RESOLVED FIXED

(DevTools :: Accessibility Tools, enhancement)

Description

[Micah [:mtigley] (she/her)]

Bounds positioning is incorrect when zoom level is manipulated while using the xul-accessible highlighter. The reason for this is the highlighter calculates zoom level in relation to the top-level window object. Instead, we should calculate the zoom based on the parent window of an accessible object.

Comment 1

[Micah [:mtigley] (she/her)]

Attachment: Bug 1469877 - XUL accessible highlighter position is incorrect at different zoom levels.

Review commit: https://reviewboard.mozilla.org/r/252120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252120/

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8986882 [details]
Bug 1469877 - XUL accessible highlighter position is incorrect at different zoom levels.

https://reviewboard.mozilla.org/r/252120/#review258820

Looks good! Just a couple of nits, see below:

::: devtools/server/actors/highlighters/utils/accessibility.js:32
(Diff revision 1)
>   * @return {Object|null} Returns, if available, positioning and bounds information for
>   *                 the accessible object.
>   */
> -function getBounds(win, { x, y, w, h }) {
> -  const { mozInnerScreenX, mozInnerScreenY, scrollX, scrollY } = win;
> -  const zoom = getCurrentZoom(win);
> +function getBounds(win, { x, y, w, h, zoom }) {
> +  let { mozInnerScreenX, mozInnerScreenY, scrollX, scrollY } = win;
> +  const zoomFactor = zoom || getCurrentZoom(win);

Perhaps in order to make it a more visible override of zoomFactor, lets just set zoomFactor here as before, e.g. ```let zoomFact = getCurrentZoom(win);``` and then inside the if statement just add ```zoomFactor = zoom;``` right before modifying inner and scroll values.

::: devtools/server/actors/highlighters/xul-accessible.js:83
(Diff revision 1)
>     *
>     * @return {Object|null} Returns, if available, positioning and bounds
>     *                       information for the accessible object.
>     */
>    get _bounds() {
> -    return getBounds(this.win, this.options);
> +    const zoom = getCurrentZoom(this.currentNode);

Lets add a comment here stating that zoom for top level browser window does not change and only inner frames do, thus our best shot is to get the zoom level of the parent window for currentNode.

::: devtools/server/actors/highlighters/xul-accessible.js:85
(Diff revision 1)
>     *                       information for the accessible object.
>     */
>    get _bounds() {
> -    return getBounds(this.win, this.options);
> +    const zoom = getCurrentZoom(this.currentNode);
> +
> +    return getBounds(this.win, {...this.options, zoom});

Niy: add a space after { and before } when using spread syntax.

Comment 3

[Micah [:mtigley] (she/her)]

Comment on attachment 8986882 [details]
Bug 1469877 - XUL accessible highlighter position is incorrect at different zoom levels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252120/diff/1-2/

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8986882 [details]
Bug 1469877 - XUL accessible highlighter position is incorrect at different zoom levels.

https://reviewboard.mozilla.org/r/252120/#review259180

Comment 5

[Micah [:mtigley] (she/her)]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2ec2f830f6cea9681826514ab8f60fe88ac860b&selectedJob=184828540

Intermittent failures that are unrelated to the bug.

Comment 6

[Eliza Balazs [:ebalazs_]]

I couldn't land your patch. mtigley: Please set the issues opened by the reviewer as fixed by commit so review board allows to land them. Thank you.

Comment 7

[Micah [:mtigley] (she/her)]

Comment on attachment 8986882 [details]
Bug 1469877 - XUL accessible highlighter position is incorrect at different zoom levels.

https://reviewboard.mozilla.org/r/252120/#review260010

Fixed issues.

Comment 8

[Micah [:mtigley] (she/her)]

Set issues on reviewboard as fixed. Thank you!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50c30489f378
XUL accessible highlighter position is incorrect at different zoom levels. r=pbro,yzen

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/50c30489f378

Comment 11

[Micah [:mtigley] (she/her)]

Clearing needinfo because it is landed.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Bug landed, clearing ni?