Closed Bug 1077044 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch testcaseSplinter Review
Tweaked the mochitest to reproduce the issue, and fail.
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 ?
^
Flags: needinfo?(eitan)
Oops, rebased!
Attachment #8499122 - Attachment is obsolete: true
Attachment #8499122 - Flags: review?(yzenevich)
Attachment #8499682 - Flags: review?(yzenevich)
Flags: needinfo?(eitan)
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.
Attachment #8499682 - Flags: review?(yzenevich) → review+
Assignee: nobody → eitan
https://hg.mozilla.org/mozilla-central/rev/b5fd3fa97b53
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.