Closed Bug 751769 Opened 8 years ago Closed 2 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

(Whiteboard: [geckoview:klar:p1])

Attachments

(1 file, 3 obsolete files)

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.
This is an interim solution. This bug should remain open until we support multiple windows.
Attachment #620895 - Flags: review?(dbolter)
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
Attachment #620895 - Flags: review?(dbolter) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b95bc1a64a7
Assignee: nobody → eitan
Whiteboard: [Leave open after merge]
Target Milestone: --- → mozilla15
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: eitan → nobody
Eitan, will we need to fix this AccessFu bug for Klar+GeckoView?
Flags: needinfo?(eitan)
Keywords: leave-open
Whiteboard: [Leave open after merge]
yes
Flags: needinfo?(eitan)
Whiteboard: [geckoview:klar]
[geckoview:klar:p1] because this is a Klar+GeckoView blocker.
Priority: -- → P1
Whiteboard: [geckoview:klar] → [geckoview:klar:p1]
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.
Flags: needinfo?(eitan)
Yes, this will be done. Working on some hairy text editing issues first that I hope to get into 61 for current Fennec 61.
Flags: needinfo?(eitan)
Thanks. Assigning to Eitan for 62.
Assignee: nobody → eitan
Keywords: leave-open
Oops. Didn't flag Yura for review yet. It is breaking some tests in Mac, so I will first troubleshoot that.
Attachment #8974566 - Attachment is obsolete: true
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;
Attachment #8980112 - Flags: review?(yzenevich) → review+
(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!
Attachment #8980112 - Attachment is obsolete: true
Target Milestone: mozilla15 → ---
Attachment #620895 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb39298e432d
Dynamically attach/detach windows in AccessFu. r=yzen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb39298e432d
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.