1376852 - Port some bounds mochitests to browser tests..

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Eitan Isaacson [:eeejay]]

For no particular reason, here are some tests that I ported.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Port bounds/test_zoom{,_text}.html r?yzen

Comment 2

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

Comment on attachment 8881842 [details] [diff] [review]
Port bounds/test_zoom{,_text}.html r?yzen

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

cool, just some nits

::: accessible/tests/browser/bounds/browser_test_zoom.js
@@ +6,5 @@
> +
> +/* import-globals-from ../../mochitest/layout.js */
> +
> +async function getContentBoundsForDOMElm(browser, id) {
> +  return ContentTask.spawn(browser, id, _id => {

nit: _id -> contentId

@@ +7,5 @@
> +/* import-globals-from ../../mochitest/layout.js */
> +
> +async function getContentBoundsForDOMElm(browser, id) {
> +  return ContentTask.spawn(browser, id, _id => {
> +    this.ok = ok;

is this needed?

@@ +18,5 @@
> +  let [expectedX, expectedY, expectedWidth, expectedHeight] =
> +    await getContentBoundsForDOMElm(browser, getAccessibleDOMNodeID(acc));
> +
> +  let [x, y, width, height] = getBounds(acc);
> +  is(x, expectedX, "Wrong x coordinate of " + prettyName(acc));

nit: maybe call prettyName(acc) once and save it in a variable?

@@ +36,5 @@
> +  await testContentBounds(browser, p2);
> +  await testContentBounds(browser, area);
> +
> +  await ContentTask.spawn(browser, {}, () => {
> +    zoomDocument(document, 2.0);

I believe we need content.document here instead of document (if it's current browser doc that is being zoomed).

::: accessible/tests/browser/bounds/browser_test_zoom_text.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/* import-globals-from ../../mochitest/layout.js */

nit: space in between

@@ +20,5 @@
> +  testTextNode("p1");
> +  testTextNode("p2");
> +
> +  await ContentTask.spawn(browser, {}, () => {
> +    zoomDocument(document, 2.0);

content.document

@@ +26,5 @@
> +
> +  testTextNode("p1");
> +
> +  await ContentTask.spawn(browser, {}, () => {
> +    zoomDocument(document, 1.0);

content.document

::: accessible/tests/browser/scroll/head.js
@@ -11,5 @@
>  Services.scriptloader.loadSubScript(
>    'chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js',
>    this);
>  
> -async function zoomContent(browser, zoom)

nit: jsdoc

@@ -12,5 @@
>    'chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js',
>    this);
>  
> -async function zoomContent(browser, zoom)
> -{

nit: { on the same line as function

@@ -13,5 @@
>    this);
>  
> -async function zoomContent(browser, zoom)
> -{
> -  return ContentTask.spawn(browser, zoom, _zoom => {

nit: _zoom -> contentZoom

::: accessible/tests/browser/shared-head.js
@@ +189,5 @@
>      } else {
>        // Script is a object that has { dir, name } format.
>        frameScript = `${script.dir}${script.name}`;
>      }
> +    let loadedScriptSet = LOADED_FRAMESCRIPTS.get(frameScript);

nit: space

@@ +201,1 @@
>      mm.loadFrameScript(frameScript, false, true);

nit space

Comment 3

[Eitan Isaacson [:eeejay]]

> ::: accessible/tests/browser/bounds/browser_test_zoom.js
> @@ +7,5 @@
> > +/* import-globals-from ../../mochitest/layout.js */
> > +
> > +async function getContentBoundsForDOMElm(browser, id) {
> > +  return ContentTask.spawn(browser, id, _id => {
> > +    this.ok = ok;
> 
> is this needed?

Yes. Something about JS scopes makes this necessary. Otherwise, `ok` is not found and that function fails.

> 
> I believe we need content.document here instead of document (if it's current
> browser doc that is being zoomed).
> 

In `addAccessibleTask` we run the following code in content, so `document` should work.
https://dxr.mozilla.org/mozilla-central/rev/6f8f10f48ace5692256efd91f011bd23054ee2ec/accessible/tests/browser/shared-head.js#249

> ::: accessible/tests/browser/scroll/head.js
> @@ -11,5 @@
> >  Services.scriptloader.loadSubScript(
> >    'chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js',
> >    this);
> >  
> > -async function zoomContent(browser, zoom)
> 
> nit: jsdoc

This function is being removed not added, so I dodged a jsdoc!

> 
> @@ -12,5 @@
> >    'chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js',
> >    this);
> >  
> > -async function zoomContent(browser, zoom)
> > -{
> 
> nit: { on the same line as function
> 

Same.

> @@ -13,5 @@
> >    this);
> >  
> > -async function zoomContent(browser, zoom)
> > -{
> > -  return ContentTask.spawn(browser, zoom, _zoom => {
> 
> nit: _zoom -> contentZoom
> 

Same.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Port bounds/test_zoom{,_text}.html

Added a conditional wait for REORDER because imagemaps can be populated late with no DOM notifications.

Comment 5

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

Comment on attachment 8883644 [details] [diff] [review]
Port bounds/test_zoom{,_text}.html

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

I'm assuming the change is:
```
if (!imgmap.childCount) {...}
```
Looks good

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5bd8ce9e84
Port bounds/test_zoom{,_text}.html r=yzen

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd5bd8ce9e84