1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Banner (:standard8)]

I'm preparing to get the general ESLint rules we have in the eslint-plugin-mozilla recommended configuration spread around the tree more widely (bug 1359011).

This serves several aims: that we can reduce duplicate definitions, have a clearer picture of differences in the tree, and make it easier to find places where we need improvements - e.g. getting more rules enabled/fixed.

I currently have a few patches in progress to do this for accessible/

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

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

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1360166 - Clean up global handling for accessible/tests/.

Now that we inherit the recommended configurations, many definitions are handled semi-automatically.

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

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8862374 [details]
Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

https://reviewboard.mozilla.org/r/134328/#review137396

::: accessible/.eslintrc.js
(Diff revision 1)
> -    "Components": true,
> -    "console": true,
> -    "Cu": true,
> -    "dump": true,
> -    "Services": true,
> -    "XPCOMUtils": true

what is a point of removing these?

::: accessible/tests/mochitest/events/test_text.html
(Diff revision 1)
>  
>        this.invoke = function removeTextFromInput_invoke()
>        {
> -        const nsIDOMNSEditableElement =
> -          Components.interfaces.nsIDOMNSEditableElement;
> -

there's one more in this file in removeTextFromContentEditable function

::: accessible/tests/mochitest/table/test_table_1.html
(Diff revision 1)
>  
>    is(accTable.selectedRowCount, 1, "no cells selected");
>  
> -  var columnDescription = accTable.getColumnDescription(1);
> -  var rowDescription = accTable.getRowDescription(1);
> -

please put those int try/catch block, it seems those lines are about testing whether the methods throw (see bug 760878)

::: accessible/tests/mochitest/test_OuterDocAccessible.html:46
(Diff revision 1)
>            is(outerDocAcc.actionCount, 0,
>               "Wrong number of actions for internal frame!");
> -          var actionTempStr; // not really used, just needs to receive a value
> +          // Not really used, just needs to receive a value.
> +          var actionTempStr; // eslint-disable-line no-unused-vars
>            try {
>              actionTempStr = outerDocAcc.getActionName(0);

you could do
try {
  outerDocAcc.getActionName(0);
}
catch()

without having that variable

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8862374 [details]
Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

https://reviewboard.mozilla.org/r/134328/#review137396

> what is a point of removing these?

Removing them allows us to fallback on the definitions in the various environments, configs and rules that we provide. For some of them (e.g. Services/XPCOMUtils) this allows us to know if they are really likely to be in the global scope or not.

> there's one more in this file in removeTextFromContentEditable function

The only place I can find that function is in test_fromUserInput.html, where I've already removed the instance. Did I miss something, or maybe a patch was landed that removed it?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8862374 [details]
Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

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

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8862375 [details]
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

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

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

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

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8862375 [details]
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

https://reviewboard.mozilla.org/r/134330/#review139158

::: accessible/tests/browser/.eslintrc.js:73
(Diff revision 2)
> -    "func-call-spacing": "error",
>      "func-names": "off",
>      "func-style": "off",
> -    "generator-star": "off",
> +    "generator-star-spacing": "off",
> -    "global-strict": "off",
>      "handle-callback-err": ["error", "er"],

global-strict is not in recommended.js?

::: accessible/tests/browser/.eslintrc.js
(Diff revision 2)
> -    "generator-star": "off",
> +    "generator-star-spacing": "off",
> -    "global-strict": "off",
>      "handle-callback-err": ["error", "er"],
>      "indent": ["error", 2, {"SwitchCase": 1}],
> -    "key-spacing": ["error", {"beforeColon": false, "afterColon": true}],
> +    //"linebreak-style": "off",
> -    "linebreak-style": "off",

why to keep it commented out?

::: accessible/tests/browser/.eslintrc.js
(Diff revision 2)
>      "quote-props": "off",
>      "radix": "error",
>      "semi": ["error", "always"],
>      "semi-spacing": ["error", {"before": false, "after": true}],
>      "sort-vars": "off",
> -    "space-after-function-name": "off",

no in recommended.js, is this option covered by some other option?

::: accessible/tests/browser/.eslintrc.js
(Diff revision 2)
>      "semi-spacing": ["error", {"before": false, "after": true}],
>      "sort-vars": "off",
> -    "space-after-function-name": "off",
> -    "keyword-spacing": "error",
> -    "space-before-blocks": "error",
> -    "space-before-function-parentheses": "off",

same for this

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8862375 [details]
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

https://reviewboard.mozilla.org/r/134330/#review139162

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

https://reviewboard.mozilla.org/r/134332/#review139184

::: accessible/tests/browser/e10s/browser_caching_attributes.js
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  'use strict';
>  
> -/* global EVENT_FOCUS */
> +/* import-globals-from ../../mochitest/attributes.js */
> -

attributes.js doesn't define EVENT_FOCUS, do you need extra events.js?

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8862374 [details]
Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

https://reviewboard.mozilla.org/r/134328/#review137396

> The only place I can find that function is in test_fromUserInput.html, where I've already removed the instance. Did I miss something, or maybe a patch was landed that removed it?

apparentl you're right, I no longer see it, maybe I confused with this one https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/events/test_fromUserInput.html#63

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8862375 [details]
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

https://reviewboard.mozilla.org/r/134330/#review139158

> global-strict is not in recommended.js?

global-strict is now obsolete in favour of strict:

http://eslint.org/docs/rules/#removed

You've already got `"strict": ["error", "global"],`, so I removed the obsolete one.

> why to keep it commented out?

Looks like it was an error on my part, removed.

> no in recommended.js, is this option covered by some other option?

Yep, this is obsolete as well (see earlier link), `space-before-function-paren` is the replacement (already in recommended.js).

> same for this

ditto with `space-before-function-paren`

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

https://reviewboard.mozilla.org/r/134332/#review139184

> attributes.js doesn't define EVENT_FOCUS, do you need extra events.js?

The browser-test config implements a rule that automatically imports head*.js globals into the browser*.js test file scopes. In this case the e10s/head.js file is already importing events.js, so browser_caching_attributes.js gets it via that.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8862374 [details]
Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.

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

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8862375 [details]
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).

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

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

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

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

https://reviewboard.mozilla.org/r/134332/#review140204

::: accessible/tests/browser/e10s/events.js:10
(Diff revision 3)
>  'use strict';
>  
> -/* global nsIAccessibleEvent, nsIAccessibleDocument,
> -          nsIAccessibleStateChangeEvent, nsIAccessibleTextChangeEvent */
> +// This is loaded by head.js, so has the same globals, hence we import the
> +// globals from there.
> +/* import-globals-from head.js */
>  

no corresponding loadScripts call? if it's loaded by default, and no loadScripts, then why it's not included into other test files as well?

::: accessible/tests/browser/shared-head.js
(Diff revision 3)
>  /**
> - * Check if an accessible object has a defunct test.
> - * @param  {nsIAccessible}  accessible object to test defunct state for
> - * @return {Boolean}        flag indicating defunct state
> - */
> -function isDefunct(accessible) {

while I don't see a problem, but I'm not sure I see why you moved these into head.js, could you give some background on it please?

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8862376 [details]
Bug 1360166 - Clean up global handling for accessible/tests/.

https://reviewboard.mozilla.org/r/134332/#review140204

> no corresponding loadScripts call? if it's loaded by default, and no loadScripts, then why it's not included into other test files as well?

It is loaded via e10s/head.js into the same scope as that file. Unfortunately it also depends on items from head.js, so we need to indicate that fior eslint here.

> while I don't see a problem, but I'm not sure I see why you moved these into head.js, could you give some background on it please?

I thought it was because there were items used within them that were only in the e10s directory. However, looking at it again, I suspect it was because these two functions are only used for the e10s tests, and I was trying to clean up the global scopes a bit.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/298b13bed1ac
Make accessible/ ESLint rules inherit from the mozilla/recommended configuration. r=surkov
https://hg.mozilla.org/integration/autoland/rev/c609f849afc6
Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js). r=surkov
https://hg.mozilla.org/integration/autoland/rev/e231df5f9506
Clean up global handling for accessible/tests/. r=surkov

Comment 21

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/298b13bed1ac
https://hg.mozilla.org/mozilla-central/rev/c609f849afc6
https://hg.mozilla.org/mozilla-central/rev/e231df5f9506