869280 - Add temporary modal subtree to pivot API

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

An occurrence in all sorts of webapps and sites are modal dialogs that pop up and make the rest of the application insensitive to input. The user should be confined to the dialog and should not be allowed to interact with the rest of the UI.

In addition to detection of such dialogs (which is not the scope of this bug), the pivot API should make it easy to limit interaction to the modal subtree.

Instead of modifying all the move methods to take a temporary root that limits the traversal, I chose to add a modalRoot attribute to the pivot interface. This is for a few reasons:

- The modal state has the scope of a document, and needs to be tracked in association with that document. Amending the pivot to hold that state makes things easier. But the control of the modal state should be left to the AT.
- The pivot.position setter can't take arguments.
- The moveNext/movePrevious already have optional nsIAccessible arguments, adding more after them changes the interpretation of providing null as optional argument values.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Add modalRoot to pivot API

Comment 2

[alexander :surkov (:asurkov)]

I'm curious why does making nsIAccessiblePrivot::root not readonly not work?

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> I'm curious why does making nsIAccessiblePrivot::root not readonly not work?

Are you suggesting that the virtual cursor pivot that is attached to the document have a writable root attribute? That would break with the current norm where the vc's root is the document. That might not be too bad, but it could get confusing if the new root is not even in the associated document. I guess we could throw an error in that case, but that means holding a reference to the document node anyway.

So with the caveat would be "pivot.root is writable, unless it is a vc pivot, and in that case the root cannot be outside of the owning document".

Also, the advantage of having a temporary root is that the state and necessary info is stored in the pivot. For example, if the we go with a writable root, we would need to externally hold a reference to the original root, and restore it when the modal dialog goes away (this is less of an issue with a doc associated vc). Or another example, it is easy to determine without any prior context whether the pivot is trapped in a modal subtree (pivot.modalRoot != null).

Bottom line, it is possible. But I think modalRoot would work better. Do you see any downsides to modalRoot?

Comment 4

[alexander :surkov (:asurkov)]

what about to add 

nsIAccessibleCursorable::getVirtualCursorFor(nsINode* aNode)
{
  return new nsAccessiblePivot(GetAccessible(aNode));
}

> Do you see any downsides to modalRoot?

It adds extra complicity to API and actually it has nothing in common with modal dialogs (except of the use case).

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #4)
> what about to add 
> 
> nsIAccessibleCursorable::getVirtualCursorFor(nsINode* aNode)
> {
>   return new nsAccessiblePivot(GetAccessible(aNode));
> }
> 

That doesn't help. We already have nsIAccessibleRetrieval::createAccessiblePivot(). And even if we didn't this requires doing move*() with this pivot, and then assigning the position to the main vc.

> > Do you see any downsides to modalRoot?
> 
> It adds extra complicity to API and actually it has nothing in common with
> modal dialogs (except of the use case).

We could call it something else, I think a temporary root could be useful for other use cases where you are only interested in traversing inside a certain subtree.

But here is the dialog example, and why it is useful:
on DOCUMENT_LOAD_COMPLETE event - if it is a modal dialog:
vc.modalRoot = dialog;
on HIDE - if it is equal to vc.modalRoot:
vc.modalRoot = null;

That is all that is needed, everything else should just work. Of course, detecting the appearance and disappearance of a dialog is not necessarily that trivial.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

> > nsIAccessibleCursorable::getVirtualCursorFor(nsINode* aNode)
> > {
> >   return new nsAccessiblePivot(GetAccessible(aNode));
> > }
> > 
> 
> That doesn't help. We already have
> nsIAccessibleRetrieval::createAccessiblePivot().

ok, good it should work for modal dialog handling.

> And even if we didn't this
> requires doing move*() with this pivot, and then assigning the position to
> the main vc.

why would you need to assign the position? these pivots are unrelated. Modal dialog should be treated as separate document and thus should have own pivot. No? 

> But here is the dialog example, and why it is useful:
> on DOCUMENT_LOAD_COMPLETE event - if it is a modal dialog:
> vc.modalRoot = dialog;
> on HIDE - if it is equal to vc.modalRoot:
> vc.modalRoot = null;

> That is all that is needed, everything else should just work. Of course,
> detecting the appearance and disappearance of a dialog is not necessarily
> that trivial.

moveNext fails after changing the modalRoot. So after you open/close a modal dialog then you start the document reading from beging/end. Is that wanted behavior?

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #6)
> (In reply to Eitan Isaacson [:eeejay] from comment #5)
> 
> > > nsIAccessibleCursorable::getVirtualCursorFor(nsINode* aNode)
> > > {
> > >   return new nsAccessiblePivot(GetAccessible(aNode));
> > > }
> > > 
> > 
> > That doesn't help. We already have
> > nsIAccessibleRetrieval::createAccessiblePivot().
> 
> ok, good it should work for modal dialog handling.
> 
> > And even if we didn't this
> > requires doing move*() with this pivot, and then assigning the position to
> > the main vc.
> 
> why would you need to assign the position? these pivots are unrelated. Modal
> dialog should be treated as separate document and thus should have own
> pivot. No? 
> 

That departs from the design of having a way of knowing where the canonical locus-of-focus/virtusl-cursor-position/point-of-regard is. This is done by using the virtual cursor associated with a document. If an AT has another pivot that is temporarily used as the vc controller instead of the document's, the true state gets obscured.

You should be able to universally observe the vc change via an a11y event. Other ATs or modules would need to be aware of a temporary pivot to do this.

> > But here is the dialog example, and why it is useful:
> > on DOCUMENT_LOAD_COMPLETE event - if it is a modal dialog:
> > vc.modalRoot = dialog;
> > on HIDE - if it is equal to vc.modalRoot:
> > vc.modalRoot = null;
> 
> > That is all that is needed, everything else should just work. Of course,
> > detecting the appearance and disappearance of a dialog is not necessarily
> > that trivial.
> 
> moveNext fails after changing the modalRoot. So after you open/close a modal
> dialog then you start the document reading from beging/end. Is that wanted
> behavior?

No, you are right. Ideally, we would be back where we were before the modal dialog popped up. But disappearing nodes is not a unique problem to modal dialogs, it exists in popup menus and in other arbitrarily added and removed content. So while a temporary pivot could work well for this in a modal dialog, it does not make sense in other cases where we have the same problem. It is a separate issue that needs to be addressed.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)

> > why would you need to assign the position? these pivots are unrelated. Modal
> > dialog should be treated as separate document and thus should have own
> > pivot. No? 
> > 
> 
> That departs from the design of having a way of knowing where the canonical
> locus-of-focus/virtusl-cursor-position/point-of-regard is. This is done by
> using the virtual cursor associated with a document. If an AT has another
> pivot that is temporarily used as the vc controller instead of the
> document's, the true state gets obscured.

it'd be nice to keep a modal dialog open by window.openDialog and a HTML created modal dialog the same way. window.openDialog is always about separate pivot.

> You should be able to universally observe the vc change via an a11y event.
> Other ATs or modules would need to be aware of a temporary pivot to do this.

so the problem is in events that document's pivot has a support.

> > moveNext fails after changing the modalRoot. So after you open/close a modal
> > dialog then you start the document reading from beging/end. Is that wanted
> > behavior?
> 
> No, you are right. Ideally, we would be back where we were before the modal
> dialog popped up. But disappearing nodes is not a unique problem to modal
> dialogs, it exists in popup menus and in other arbitrarily added and removed
> content. So while a temporary pivot could work well for this in a modal
> dialog, it does not make sense in other cases where we have the same
> problem. It is a separate issue that needs to be addressed.

so you need to store old position, it seems pivot is a good container for this. What about to change pivots for the document like:

interface nsICusorable
{
  pivot virtualCursor;
}

var modalPivot = newPivot(modalDialogRoot); // modal dialog open
var docPivot = document.virtualCursor;
document.virtualCursor = modalPivot;
// bla bla
document.virtualCurosr = docPivot; // modal dialog closed

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #8)
> (In reply to Eitan Isaacson [:eeejay] from comment #7)
> 
> > > why would you need to assign the position? these pivots are unrelated. Modal
> > > dialog should be treated as separate document and thus should have own
> > > pivot. No? 
> > > 
> > 
> > That departs from the design of having a way of knowing where the canonical
> > locus-of-focus/virtusl-cursor-position/point-of-regard is. This is done by
> > using the virtual cursor associated with a document. If an AT has another
> > pivot that is temporarily used as the vc controller instead of the
> > document's, the true state gets obscured.
> 
> it'd be nice to keep a modal dialog open by window.openDialog and a HTML
> created modal dialog the same way. window.openDialog is always about
> separate pivot.
> 

It is similar, but not identical. AFAIK, openDialog() is not standard, and is mostly used for XUL desktop. It will probably not be encountered, in most cases going forward, so I don't think we need to keep things 100% consistent with it.

Behaviorally, I agree that the user should have the same experience. Whether that is achieved by one pivot with two roots, or two pivots does not matter.

> > You should be able to universally observe the vc change via an a11y event.
> > Other ATs or modules would need to be aware of a temporary pivot to do this.
> 
> so the problem is in events that document's pivot has a support.
> 

right.

> > > moveNext fails after changing the modalRoot. So after you open/close a modal
> > > dialog then you start the document reading from beging/end. Is that wanted
> > > behavior?
> > 
> > No, you are right. Ideally, we would be back where we were before the modal
> > dialog popped up. But disappearing nodes is not a unique problem to modal
> > dialogs, it exists in popup menus and in other arbitrarily added and removed
> > content. So while a temporary pivot could work well for this in a modal
> > dialog, it does not make sense in other cases where we have the same
> > problem. It is a separate issue that needs to be addressed.
> 
> so you need to store old position, it seems pivot is a good container for
> this. 

It may be, but like I said, this needs a solution in other places besides modal dialogs. Solving it here is not enough.

> What about to change pivots for the document like:
> 
> interface nsICusorable
> {
>   pivot virtualCursor;
> }
> 
> var modalPivot = newPivot(modalDialogRoot); // modal dialog open
> var docPivot = document.virtualCursor;
> document.virtualCursor = modalPivot;
> // bla bla
> document.virtualCurosr = docPivot; // modal dialog closed

This could be achieved in a very similar manner with the interface I proposed:

var oldPosition = document.virtualCursor;
document.virtualCursor.modalRoot = modalDialogRoot; // modal dialog opens
// bla bla
document.virtualCursor.modalRoot = null; // modal dialog closes
document.virtualCursor.position = oldPosition;

The problem with both proposals, in my mind, is that they require holding a reference to the pre-dialog state (docPivot/oldPosition). A more complete solution would be similar to yours, but instead of a single virtualCursor, it would be a stack:

var modalPivot = newPivot(modalDialogRoot); // modal dialog open
document.virtualCursor.pushVirtualCursor(modalPivot)
// bla bla
document.virtualCurosr.popVirtualCursor(); // modal dialog closed

Like I said above, this only solves a small proportion of our disappearing node problem. So I am not convinced it would be worth it. That is why I am proposing a more modest addition (and not a change) to the API. The name may not be perfect, since it could be used for other things besides modal dialogs.

Comment 10

[alexander :surkov (:asurkov)]

I think I will be fine with
1) writable root or
2) writable virtual cursor

You can keep a stack on client side rather than embed it into virtual cursor. I'm not sure I see benefits of having of temporary root (if you don't want to have external references to old root then you should introduce a member to store temporary position to stay consistent). In either way 'modalRoot' is not a semantically correct name since as you said it's supposed to be used for menus as well.

So what API would you end up with?

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #10)
> So what API would you end up with?

What does that mean?

Comment 12

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

FWIW I think I like the stack approach.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> (In reply to alexander :surkov from comment #10)
> > So what API would you end up with?
> 
> What does that mean?

do you still think that API you proposed in the patch is the best?

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #13)
> (In reply to Eitan Isaacson [:eeejay] from comment #11)
> > (In reply to alexander :surkov from comment #10)
> > > So what API would you end up with?
> > 
> > What does that mean?
> 
> do you still think that API you proposed in the patch is the best?

I don't think any proposal is superior, they all add complexity, and they all have downsides. That is why I prefer my proposal because I already implemented it :)

We could change the name obviously, temporaryRoot?

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 746184 [details] [diff] [review]
Add modalRoot to pivot API

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

I don't mind. Up to you with modalRoot renaming. rs=me (I didn't check all code paths).

::: accessible/src/base/nsAccessiblePivot.h
@@ +62,2 @@
>     */
> +  bool IsDescendant(Accessible* aAccessible, Accessible* aAncestor);

perhaps IsDescendantOf but I liked IsRootDescendant more

@@ +75,5 @@
>    /*
>     * Reverse search in preorder for the first accessible to match the rule.
>     */
>    Accessible* SearchBackward(Accessible* aAccessible,
> +                             Accessible* aRoot,

I'd replaced mRoot to GetActiveRoot() in method bodies rather than add a new argument

@@ +83,5 @@
>  
>    /*
> +   * Get the effective root for this pivot, either the true root or modal root.
> +   */
> +  Accessible* GetActiveRoot();

i'd keep it inline

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Bug 869280 - Add modalRoot to pivot API

I know that making a function inline is simple. But I ended up reshuffling some of the header info, and I just want to make sure I didn't do anything horrible.

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: tests for modalroot

Comment 18

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

Comment on attachment 753379 [details] [diff] [review]
tests for modalroot

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

r=me for the logic. A couple o nits.

::: accessible/tests/mochitest/pivot.js
@@ +275,5 @@
> +    var errorResult = 0;
> +    try {
> +      aDocAcc.virtualCursor.modalRoot = aModalRootAcc;
> +    } catch (x) {
> +      errorResult = x.result || 1;

Can you add the more detailed info we talked about for the x.result == 0 case?

@@ +285,5 @@
> +
> +  this.getID = function setVCPosInvoker_getID()
> +  {
> +    return "Set modalRoot to " + prettyName(aModalRootAcc);
> +  };

Did you mean setModalRootInvoker_getID ?

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 753378 [details] [diff] [review]
Bug 869280 - Add modalRoot to pivot API

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

::: accessible/src/base/nsAccessiblePivot.cpp
@@ +439,5 @@
>      if (filtered & nsIAccessibleTraversalRule::FILTER_MATCH)
>        return accessible;
>    }
>  
> +  while (accessible != root) {

it doesn't make sense to keep a local variable for GetActiveRoot() if it's used only once

::: accessible/src/base/nsAccessiblePivot.h
@@ +74,5 @@
>  
>    /*
> +   * Get the effective root for this pivot, either the true root or modal root.
> +   */
> +  inline Accessible* GetActiveRoot()

you don't need 'inline' keyword. it'd be good to make it const

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #19)
> Comment on attachment 753378 [details] [diff] [review]
> Bug 869280 - Add modalRoot to pivot API
> 
> Review of attachment 753378 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessiblePivot.cpp
> @@ +439,5 @@
> >      if (filtered & nsIAccessibleTraversalRule::FILTER_MATCH)
> >        return accessible;
> >    }
> >  
> > +  while (accessible != root) {
> 
> it doesn't make sense to keep a local variable for GetActiveRoot() if it's
> used only once
> 

Wouldn't it get called every loop cycle?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #20)
> (In reply to alexander :surkov from comment #19)
> > Comment on attachment 753378 [details] [diff] [review]
> > Bug 869280 - Add modalRoot to pivot API
> > 
> > Review of attachment 753378 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/base/nsAccessiblePivot.cpp
> > @@ +439,5 @@
> > >      if (filtered & nsIAccessibleTraversalRule::FILTER_MATCH)
> > >        return accessible;
> > >    }
> > >  
> > > +  while (accessible != root) {
> > 
> > it doesn't make sense to keep a local variable for GetActiveRoot() if it's
> > used only once
> > 
> 
> Wouldn't it get called every loop cycle?

I'm not sure (Trev knows better how compliers work). Anyway move variable before loop to stay on safe side

Comment 22

[Eitan Isaacson [:eeejay]]

Attachment: Bug 869280 - Add modalRoot to pivot API. rs=surkov

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Bug 869280 - tests for modalroot. r=davidb

Comment 24

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/546a05ebe9b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f77e67ad954

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/546a05ebe9b7
https://hg.mozilla.org/mozilla-central/rev/0f77e67ad954