Closed Bug 1425244 Opened 6 years ago Closed 6 years ago

Enable more ESLint rules for accessible/

Categories

(Core :: Disability Access APIs, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1425244 - Enable ESLint rule object-shorthand for accessible/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule comma-spacing for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule no-cond-assign for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule no-lonely-if for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule no-new-object for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule space-unary-ops for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule no-shadow for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1425244 - Enable ESLint rule no-redeclare for accessible/tests/mochitest/.
59 bytes, text/x-review-board-request
surkov
: review+
Details 
I've been working on enabling more rules for accessible/ - so that it is closer to the mozilla/recommended configuration.
Comment on attachment 8936820 [details]
Bug 1425244 - Enable ESLint rule object-shorthand for accessible/.

https://reviewboard.mozilla.org/r/207522/#review213430

::: accessible/tests/mochitest/autocomplete.js:109
(Diff revision 1)
>  {
>    constructor: AutoCompleteSearch,
>  
>    // nsIAutoCompleteSearch implementation
> -  startSearch: function(aSearchString, aSearchParam, aPreviousResult,
> +  startSearch(aSearchString, aSearchParam, aPreviousResult,
>                          aListener) {

nit: indentation

::: accessible/tests/mochitest/relations/test_ui_modalprompt.html:39
(Diff revision 1)
>        this.eventSeq = [
>          {
>            type: EVENT_SHOW,
> -          match: function(aEvent) {
> +          match(aEvent) {
> -            return aEvent.accessible.role == ROLE_DIALOG;
> +        return aEvent.accessible.role == ROLE_DIALOG;
> -          }
> +      }

something wrong with indentation

::: accessible/tests/mochitest/table.js:142
(Diff revision 1)
>            break;
>        }
>  
>        if (role != ROLE_NOTHING) {
>          var cellObj = {
> -          role: role
> +          role

it could be { role };
Attachment #8936820 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936821 [details]
Bug 1425244 - Enable ESLint rule comma-spacing for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207524/#review213446
Attachment #8936821 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936822 [details]
Bug 1425244 - Enable ESLint rule no-cond-assign for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207526/#review213448
Attachment #8936822 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936823 [details]
Bug 1425244 - Enable ESLint rule no-lonely-if for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207528/#review213462
Comment on attachment 8936823 [details]
Bug 1425244 - Enable ESLint rule no-lonely-if for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207528/#review213464
Attachment #8936823 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936824 [details]
Bug 1425244 - Enable ESLint rule no-new-object for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207530/#review213468
Attachment #8936824 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936825 [details]
Bug 1425244 - Enable ESLint rule space-unary-ops for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207532/#review213470
Attachment #8936825 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8936826 [details]
Bug 1425244 - Enable ESLint rule no-shadow for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207534/#review213474

::: accessible/tests/mochitest/editabletext/editabletext.js:201
(Diff revision 1)
>    };
>  
>    // ////////////////////////////////////////////////////////////////////////////
>    // Implementation details.
>  
> -  function getValue(aID) {
> +  function getValue(aID1) {

it appears you can safely remove aID argument from all these method

::: accessible/tests/mochitest/treeview.js:26
(Diff revision 1)
> -      return "Load XUL tree " + prettyName(aTreeID);
> +      return "Load XUL tree " + prettyName(aTreeID1);
>      };
>    }
>  
>    gXULTreeLoadContext.queue = new eventQueue();
>    gXULTreeLoadContext.queue.push(new loadXULTree(treeID, treeView));

it'd be nicer to replace new loadXULTree() on the object, since loadXULTree constructor is used only once
Attachment #8936826 - Flags: review?(surkov.alexander)
Comment on attachment 8936827 [details]
Bug 1425244 - Enable ESLint rule no-redeclare for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207536/#review213478
Attachment #8936827 - Flags: review?(surkov.alexander) → review+
Thanks for the comments.

(In reply to alexander :surkov from comment #16)
> ::: accessible/tests/mochitest/treeview.js:26
> (Diff revision 1)
> > -      return "Load XUL tree " + prettyName(aTreeID);
> > +      return "Load XUL tree " + prettyName(aTreeID1);
> >      };
> >    }
> >  
> >    gXULTreeLoadContext.queue = new eventQueue();
> >    gXULTreeLoadContext.queue.push(new loadXULTree(treeID, treeView));
> 
> it'd be nicer to replace new loadXULTree() on the object, since loadXULTree
> constructor is used only once

Can you explain a bit more here please? I'm not quite sure what you're thinking of.
Flags: needinfo?(surkov.alexander)
gXULTreeLoadContext.queue.push({
  treeNode: getNode(treeID),

  eventSeq: [
      new invokerChecker(EVENT_REORDER, this.treeNode)
  ],

  invoke() {
    this.treeNode.view = treeView;
  },

  getID() {
    return "Load XUL tree " + prettyName(treeID);
  }
});
Flags: needinfo?(surkov.alexander)
Thanks, that made sense. I modified it slightly (as this.treeNode wasn't valid), but it seems to now work, and I'll push it to try.
In the latest version, the changes in the final patch (no-redeclare) are showing up with a test failure:

ReferenceError: eventSeq is not defined at eventQueue_handleEvent@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:547:19

The diff from bug 1316154 looks a bit strange. This section is trying to parse an eventSeq:

https://searchfox.org/mozilla-central/diff/4dad35bd6005a6a466d026527b564669ccdb4a47/accessible/tests/mochitest/events.js#566

However, that seems to be assumed to come from:

https://searchfox.org/mozilla-central/diff/4dad35bd6005a6a466d026527b564669ccdb4a47/accessible/tests/mochitest/events.js#526

That makes it look like the lower section should be iterating over this.mScenarios, rather than just attempting to use the last item from around the loop.

I've done the change locally and it fixes the test, so I'll push an update and see how that affects try.
Blocks: 1425371
Comment on attachment 8936826 [details]
Bug 1425244 - Enable ESLint rule no-shadow for accessible/tests/mochitest/.

https://reviewboard.mozilla.org/r/207534/#review213582

thanks!
Attachment #8936826 - Flags: review?(surkov.alexander) → review+
Ok, the events.js changes didn't work and seemed to cause more issues. I've split those out to bug 1425371 and undone the changes to that file.

Assuming the next try run succeeds, I'll push these patches. Thank you for the fast reviews.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f884ff63d0c
Enable ESLint rule object-shorthand for accessible/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/899e79132bbe
Enable ESLint rule comma-spacing for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/86a980a403db
Enable ESLint rule no-cond-assign for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/f0802f46c9d9
Enable ESLint rule no-lonely-if for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/096025960071
Enable ESLint rule no-new-object for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/86ee98b5a582
Enable ESLint rule space-unary-ops for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/39cf2973f9f4
Enable ESLint rule no-shadow for accessible/tests/mochitest/. r=surkov
https://hg.mozilla.org/integration/autoland/rev/ab930b218ba8
Enable ESLint rule no-redeclare for accessible/tests/mochitest/. r=surkov
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: