Eitan Isaacson [:eeejay] Assignee
	Description • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 1 • 10 years ago

Tweaked the mochitest to reproduce the issue, and fail.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 10 years ago

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 ?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 4 • 10 years ago

^

	Eitan Isaacson [:eeejay] Assignee
	Comment 5 • 10 years ago

Oops, rebased!

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 6 • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 7 • 10 years ago

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

Feedback done in patch I landed.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 10 years ago

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