Closed Bug 1360166 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(3 files)

Bug 1360166 - Make accessible/ ESLint rules inherit from the mozilla/recommended configuration.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1360166 - Remove duplicated rules in accessible/tests/browser/.eslintrc.js (already in recommended.js).
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1360166 - Clean up global handling for accessible/tests/.
59 bytes, text/x-review-board-request
surkov
: review+
Details 
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 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
Attachment #8862374 - Flags: review?(surkov.alexander) → review+
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 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
Attachment #8862375 - Flags: review?(surkov.alexander) → review+
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 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 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 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 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.
Depends on: 1363080
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?
Attachment #8862376 - Flags: review?(surkov.alexander) → review+
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.
Pushed by mbanner@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.