1077044 - [AccessFu] VC should be placed back where it was before a dialog disappeared

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Currently, it kind-of-sometimes works if the cursor was previously in an embedded (focused) iframe cursor. If the dialog is hidden purely programatically, it will go back to the iframe, and thus to the embedded vc.

But more likely, the user will tap a button on the dialog to dismiss it, and thus reset the focus in the doc away from the iframe, and then the vc placement will be less predictable.

In the case when the dialog is in the save vc-space as the content, we will always lose the position previous to the dialog.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: testcase

Tweaked the mochitest to reproduce the issue, and fail.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Store previous cursor position when dialog pops up, and restore it when it is hidden.

Comment 3

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

We have this bit of code in the event handler now: https://github.com/mozilla/gecko-dev/blob/master/accessible/jsat/EventManager.jsm#L268-L273 I wonder how that fits with the changes ?

Comment 4

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

^

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Store previous cursor position when dialog pops up, and restore it when it is hidden.

Oops, rebased!

Comment 6

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

Comment on attachment 8499682 [details] [diff] [review]
Store previous cursor position when dialog pops up, and restore it when it is hidden.

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

looks good with a couple of comments. thanks!

::: accessible/jsat/EventManager.jsm
@@ +60,5 @@
>             Ci.nsIWebProgress.NOTIFY_LOCATION));
>          this.addEventListener('wheel', this, true);
>          this.addEventListener('scroll', this, true);
>          this.addEventListener('resize', this, true);
> +        this._preDialogPosition = new WeakMap();

I know it's a WeakMap but perhaps we should call this._preDialogPosition.clear(); just in case in EventManager.stop() ?

@@ +329,5 @@
>      this.editState = editState;
>    },
>  
> +  _handleShow: function _handleShow(aEvent, aDialog) {
> +    if (aDialog) {

I think we should keep the if (aDialog) {...} code in Events.DOCUMENT_LOAD_COMPLETE case (or make a new function for it) and just add a this._preDialogPosition.set step there.

Comment 7

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fd3fa97b53

Feedback done in patch I landed.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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