965537 - [AccessFu] Refactor content-script.js

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

It is becoming a big collection of random functions.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce content controller

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: [AccessFu] Introduce ContentControl.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: [AccessFu] Introduce ContentControl.

Comment 4

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

Comment on attachment 8383152 [details] [diff] [review]
[AccessFu] Introduce ContentControl.

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

Looks very cool. r=me with questions/nits.

::: accessible/src/jsat/AccessFu.jsm
@@ +298,5 @@
>          break;
>        case 'Accessibility:Focus':
>          this._focused = JSON.parse(aData);
>          if (this._focused) {
> +	  this.autoMove({ forcePresent: true, noOpIfOnScreen: true, delay: 0 });

Delay 0 should be a default.
nit: indentation.

@@ +342,5 @@
>          if (this._focused) {
>            // We delay this for half a second so the awesomebar could close,
>            // and we could use the current coordinates for the content item.
>            // XXX TODO figure out how to avoid magic wait here.
> +	  this.autoMove({

Nit: indentation.

::: accessible/src/jsat/ContentControl.jsm
@@ +58,5 @@
> +  get vc() {
> +    return Utils.getVirtualCursor(this.document);
> +  },
> +
> +  addListenersFor: function ContentControl_addListenersFor(aMessageManager) {

I would rename to addCursorListener since it's just 1 message name we listen to and have 1 listener.

@@ +95,5 @@
> +
> +  handleMove: function ContentControl_handleMove(aMessage) {
> +    let origin = aMessage.json.origin;
> +    let action = aMessage.json.action;
> +    let rule = aMessage.json.rule;

let rule = TraversalRules[aMessage.json.rule];

@@ +98,5 @@
> +    let action = aMessage.json.action;
> +    let rule = aMessage.json.rule;
> +    let vc = this.vc;
> +
> +    if (origin != 'child' && this.sendToChild(aMessage, vc)) {

Lost a helpful comment here.

@@ +104,5 @@
> +    }
> +
> +    let moved = vc[action](TraversalRules[rule]);
> +
> +    if (moved) {

I think it the block below should look like(from what i understand):
if (moved) {
  if (origin === 'child') {
    Utils.getMessageManager(aMessage.target).sendAsyncMessage(
      'AccessFu:ClearCursor');
  } else {
    if (action === 'moveNext') {
      action = 'moveFirst';
    } else if (action === 'movePrevious') {
      action = 'moveLast';
    }
    this.sendToChild(aMessage, vc, {
      action: action
    });
  }
} else if (!this._childMessageSenders.has(aMessage.target)) {
  this.sendToParent(aMessage);
}

@@ +113,5 @@
> +        action = 'moveLast';
> +      }
> +
> +      if (origin === 'child') {
> +        Utils.getMessageManager(aMessage.target).

nit: bad line breakage.

@@ +128,5 @@
> +    }
> +  },
> +
> +  handleMoveToPoint: function ContentControl_handleMoveToPoint(aMessage) {
> +    let[x, y] = [aMessage.json.x, aMessage.json.y];

nit: space after let.

@@ +136,5 @@
> +
> +    let dpr = win.devicePixelRatio;
> +    this.vc.moveToPoint(rule, x * dpr, y * dpr, true);
> +
> +    let delta = Utils.isContentProcess ? {

no need to assign delta if not content process.
let delta;
if (Utils.isContentProcess) {
  delta = {
    x: x - win.mozInnerScreenX,
    y: y - win.mozInnerScreenY
  };
}

@@ +169,5 @@
> +
> +    return null;
> +  },
> +
> +  sendToChild: function ContentControl_sendToChild(aMessage,

I think order of args is a little confusing: perhaps [vc, aMessage, aReplacer] this way it looks clearer that aReplacer extends aMessage.json (sort of :) ).

@@ +232,5 @@
> +        return;
> +      }
> +
> +      if (aOptions.moveToFocused) {
> +        let focusedAcc =

acc = Utils.AccRetrieval.getAccessibleFor(this.document.activeElement) || acc;

@@ +264,5 @@
> +        forcePresentFunc();
> +      }
> +    };
> +
> +    if (aOptions.delay !== 0) {

If we have a default 0 we won't need 2 checks for delay.

::: accessible/src/jsat/EventManager.jsm
@@ +97,5 @@
>        switch (aEvent.type) {
>        case 'wheel':
>        {
>          let attempts = 0;
> +	let delta = aEvent.deltaX || aEvent.deltaY;

nit: indentations here and below.

@@ +102,5 @@
> +	this.contentScope.contentControl.autoMove(
> +	  null,
> +	  { moveMethod: delta > 0 ? 'moveNext' : 'movePrevious',
> +	    onScreenOnly: true,
> +	    noOpIfOnScreen: true });

noOpIfOnScreen: true should be a default.

@@ +184,5 @@
>        }
>        case Events.SCROLLING_START:
>        {
> +        this.contentScope.contentControl.autoMove(aEvent.accessible,
> +						  { delay: 0 });

Delay 0 should be a default.
nit: indentation.

@@ +273,5 @@
>          // Put vc where the focus is at
>          let acc = aEvent.accessible;
>          let doc = aEvent.accessibleDocument;
>          if (acc.role != Roles.DOCUMENT && doc.role != Roles.CHROME_WINDOW) {
> +	  this.contentScope.contentControl.autoMove(acc, { delay: 0 });

delay: 0 should be a default.

Comment 5

[Eitan Isaacson [:eeejay]]

All good feedback, and fixed. Some push back below..

(In reply to Yura Zenevich [:yzen] from comment #4)
> ::: accessible/src/jsat/ContentControl.jsm
> @@ +58,5 @@
> > +  get vc() {
> > +    return Utils.getVirtualCursor(this.document);
> > +  },
> > +
> > +  addListenersFor: function ContentControl_addListenersFor(aMessageManager) {
> 
> I would rename to addCursorListener since it's just 1 message name we listen
> to and have 1 listener.
> 

I simply removed that method.

> @@ +95,5 @@
> > +
> > +  handleMove: function ContentControl_handleMove(aMessage) {
> > +    let origin = aMessage.json.origin;
> > +    let action = aMessage.json.action;
> > +    let rule = aMessage.json.rule;
> 
> let rule = TraversalRules[aMessage.json.rule];
> 

Remove that variable.

> @@ +98,5 @@
> > +    let action = aMessage.json.action;
> > +    let rule = aMessage.json.rule;
> > +    let vc = this.vc;
> > +
> > +    if (origin != 'child' && this.sendToChild(aMessage, vc)) {
> 
> Lost a helpful comment here.
> 
> @@ +104,5 @@
> > +    }
> > +
> > +    let moved = vc[action](TraversalRules[rule]);
> > +
> > +    if (moved) {
> 
> I think it the block below should look like(from what i understand):
> if (moved) {
>   if (origin === 'child') {
>     Utils.getMessageManager(aMessage.target).sendAsyncMessage(
>       'AccessFu:ClearCursor');
>   } else {
>     if (action === 'moveNext') {
>       action = 'moveFirst';
>     } else if (action === 'movePrevious') {
>       action = 'moveLast';
>     }
>     this.sendToChild(aMessage, vc, {
>       action: action
>     });
>   }
> } else if (!this._childMessageSenders.has(aMessage.target)) {
>   this.sendToParent(aMessage);
> }
> 

Yup! Much better. Added more comments too.

> @@ +136,5 @@
> > +
> > +    let dpr = win.devicePixelRatio;
> > +    this.vc.moveToPoint(rule, x * dpr, y * dpr, true);
> > +
> > +    let delta = Utils.isContentProcess ? {
> 
> no need to assign delta if not content process.
> let delta;
> if (Utils.isContentProcess) {
>   delta = {
>     x: x - win.mozInnerScreenX,
>     y: y - win.mozInnerScreenY
>   };
> }
> 

That is functionally the same, so I am not touching this. If that is OK. My subjective opinion is that assigning variables in conditional scopes while having to declare the variable outside the scope is ugly. Also, uninitialized variables are bad.

> @@ +169,5 @@
> > +
> > +    return null;
> > +  },
> > +
> > +  sendToChild: function ContentControl_sendToChild(aMessage,
> 
> I think order of args is a little confusing: perhaps [vc, aMessage,
> aReplacer] this way it looks clearer that aReplacer extends aMessage.json
> (sort of :) ).
> 

Done!

> @@ +264,5 @@
> > +        forcePresentFunc();
> > +      }
> > +    };
> > +
> > +    if (aOptions.delay !== 0) {
> 
> If we have a default 0 we won't need 2 checks for delay.
> 

I still prefer to. Because of the weird things that could happen with this._autoMove. If the delay is 0, it should do the move immediately.

> @@ +102,5 @@
> > +	this.contentScope.contentControl.autoMove(
> > +	  null,
> > +	  { moveMethod: delta > 0 ? 'moveNext' : 'movePrevious',
> > +	    onScreenOnly: true,
> > +	    noOpIfOnScreen: true });
> 
> noOpIfOnScreen: true should be a default.
> 

I don't think so.. we usually don't want that behavior.

Comment 6

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38659c5a5c70

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38659c5a5c70