751769 - [AccessFu] attaching AccessFu to more than one window raises havoc

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Eitan Isaacson [:eeejay]]

Currently, AccessFu is not set up to handle more than one attached window. On a desktop browser this is a problem. On Android (and I think B2G) there is only one window. In any case AccessFu should be able to handle more than one window.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Only attach to first window, throw exception if attach is called again.

This is an interim solution. This bug should remain open until we support multiple windows.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 620895 [details] [diff] [review]
Only attach to first window, throw exception if attach is called again.

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

r=me

Comment 3

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2b95bc1a64a7

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b95bc1a64a7

Comment 5

[Chris Peterson [:cpeterson]]

Eitan, will we need to fix this AccessFu bug for Klar+GeckoView?

Comment 6

[Eitan Isaacson [:eeejay]]

yes

Comment 7

[Chris Peterson [:cpeterson]]

[geckoview:klar:p1] because this is a Klar+GeckoView blocker.

Comment 8

[Chris Peterson [:cpeterson]]

Eitan, do you have time to fix this bug in GeckoView Nightly 62? We're trying to fix all the blockers for the first Klar+GeckoView release in GeckoView 62.

Comment 9

[Eitan Isaacson [:eeejay]]

Yes, this will be done. Working on some hairy text editing issues first that I hope to get into 61 for current Fennec 61.

Comment 10

[Chris Peterson [:cpeterson]]

Thanks. Assigning to Eitan for 62.

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Dynamically attach/detach windows in AccessFu. r?yzen

Comment 12

[Eitan Isaacson [:eeejay]]

Oops. Didn't flag Yura for review yet. It is breaking some tests in Mac, so I will first troubleshoot that.

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Dynamically attach/detach windows in AccessFu. r?yzen

Comment 14

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

Comment on attachment 8980112 [details] [diff] [review]
Dynamically attach/detach windows in AccessFu. r?yzen

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

This is nice, some questions below:

::: accessible/jsat/AccessFu.jsm
@@ +282,5 @@
>          }
>          this._handleMessageManager(frameLoader.messageManager);
>          break;
>        }
> +      case "domwindowopened": {

Do we care about "domwindowclosed" ? do we want to detach in that case? Or is it all GC'ed somehow?

@@ +327,5 @@
>      }
>    },
>  
>    autoMove: function autoMove(aOptions) {
> +    let mm = Utils.getMessageManager();

This is not right away clear where the message manager comes from, unless you look into what getMessageManager and then getCurrentBrowser do. What do you think about splitting getMessageManager into two distinct methods: getMessageMangerFor and getCurrentMessageManager?

Since getCurrentMessageManager does not require an argument, we can make it a getter `Utils.currentMessageManager`

@@ +332,5 @@
>      mm.sendAsyncMessage("AccessFu:AutoMove", aOptions);
>    },
>  
>    announce: function announce(aAnnouncement) {
> +    this._output(Presentation.announce(aAnnouncement), Utils.getCurrentBrowser());

Should getCurrentBrowser remain a getter (`Utils.currentBrowser`)?

@@ +356,4 @@
>        let bounds = new Rect(aJsonBounds.left, aJsonBounds.top,
>                              aJsonBounds.right - aJsonBounds.left,
>                              aJsonBounds.bottom - aJsonBounds.top);
> +      let dpr = aWindow.devicePixelRatio;

nit: let { devicePixelRatio: dpr, mozInnerScreenX, mozInnerScreenY } = aWindow;

Comment 15

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #14)
> Comment on attachment 8980112 [details] [diff] [review]
> Dynamically attach/detach windows in AccessFu. r?yzen
> 
> Review of attachment 8980112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is nice, some questions below:
> 
> ::: accessible/jsat/AccessFu.jsm
> @@ +282,5 @@
> >          }
> >          this._handleMessageManager(frameLoader.messageManager);
> >          break;
> >        }
> > +      case "domwindowopened": {
> 
> Do we care about "domwindowclosed" ? do we want to detach in that case? Or
 

Yeah, we just attach message/event listeners. Try is happy, so I am assuming there are no leaks.

> 
> @@ +327,5 @@
> >      }
> >    },
> >  
> >    autoMove: function autoMove(aOptions) {
> > +    let mm = Utils.getMessageManager();
> 
> This is not right away clear where the message manager comes from, unless
> you look into what getMessageManager and then getCurrentBrowser do. What do
> you think about splitting getMessageManager into two distinct methods:
> getMessageMangerFor and getCurrentMessageManager?
> 
> Since getCurrentMessageManager does not require an argument, we can make it
> a getter `Utils.currentMessageManager`
> 
> @@ +332,5 @@
> >      mm.sendAsyncMessage("AccessFu:AutoMove", aOptions);
> >    },
> >  
> >    announce: function announce(aAnnouncement) {
> > +    this._output(Presentation.announce(aAnnouncement), Utils.getCurrentBrowser());
> 
> Should getCurrentBrowser remain a getter (`Utils.currentBrowser`)?

No, sometimes we supply a window argument.

I thought about this a while, and I think it is fine to have two getter functions that return the x of the active window if no argument is given. Don't want to over-complicate it.

> 
> @@ +356,4 @@
> >        let bounds = new Rect(aJsonBounds.left, aJsonBounds.top,
> >                              aJsonBounds.right - aJsonBounds.left,
> >                              aJsonBounds.bottom - aJsonBounds.top);
> > +      let dpr = aWindow.devicePixelRatio;
> 
> nit: let { devicePixelRatio: dpr, mozInnerScreenX, mozInnerScreenY } =
> aWindow;

Good idea!

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Dynamically attach/detach windows in AccessFu.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb39298e432d
Dynamically attach/detach windows in AccessFu. r=yzen

Comment 18

[Andreea Pavel [:apavel]]

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