741683 - move nsAccessNode::ScrollTo to nsCoreUtils

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

nsAccessNode::ScrollTo(PRUint32 aScrollType)
to
nsCoreUtils::ScrollTo(nsIPresShell* aPresShell, nsIContent* aContent, PRUint32 aScrollType)

then nsAccessbile::ScrollTo does:
nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType)

same for nsAccessNodeWrap::scrollTo

that's a part of getting rid of nsAccessNode class.

Comment 1

[Trevor Saunders (:tbsaunde)]

> nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType)
> 
> same for nsAccessNodeWrap::scrollTo

could we do mContent->OwnerDoc()->GetPresShell() in nsAccessNodeWrap to make it easier to move mDoc to nsAccessible which also a part of getting rid of nsAccessNode?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType)
> > 
> > same for nsAccessNodeWrap::scrollTo
> 
> could we do mContent->OwnerDoc()->GetPresShell() in nsAccessNodeWrap to make
> it easier to move mDoc to nsAccessible which also a part of getting rid of
> nsAccessNode?

that doesn't work for multiple presshels per document, nsAccessNodeWrap should keep a link to a document accessible.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > > nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType)
> > > 
> > > same for nsAccessNodeWrap::scrollTo
> > 
> > could we do mContent->OwnerDoc()->GetPresShell() in nsAccessNodeWrap to make
> > it easier to move mDoc to nsAccessible which also a part of getting rid of
> > nsAccessNode?
> 
> that doesn't work for multiple presshels per document, nsAccessNodeWrap
> should keep a link to a document accessible.

yeah, but it would be equivalent until we base doc accessibles on presShells instead of documents.

keeping a pointer to the doc accessible in access nodes is sort of anoying though since we don't have a way to shut them down if they aren't ccessibles, and so also mke making them tearoffs or something harder.

How about having nsAccessNodeWrap store a presShell on windows (and keep a strong ref)?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> How about having nsAccessNodeWrap store a presShell on windows (and keep a
> strong ref)?

no, no, accessibility shouldn't keep presshell and other things alive :)

Comment 5

[alexander :surkov (:asurkov)]

since we don't control life cycle of COM objects then we should practice weak refs to accessible objects instead.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > How about having nsAccessNodeWrap store a presShell on windows (and keep a
> > strong ref)?
> 
> no, no, accessibility shouldn't keep presshell and other things alive :)

yeah, that was a welp, not sure what else we can do here idea.

(In reply to alexander :surkov from comment #5)
> since we don't control life cycle of COM objects then we should practice
> weak refs to accessible objects instead.

I'm fine with this if you have a reasonable way to implement, I don't remember your plan off hand, but can go back and look soon.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> since we don't control life cycle of COM objects then we should practice
> weak refs to accessible objects instead.

what about the mContent pointer? that might actually be a bigger issue since I suspect one ISimpleDOMNode could keep everything in a doc alive.

one idea that occured to me is to see if we can abuse the cycle collector into telling us when it wants to make the mContent, or for that matter a pres shell go away.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #5)
> > since we don't control life cycle of COM objects then we should practice
> > weak refs to accessible objects instead.
> 
> what about the mContent pointer? that might actually be a bigger issue since
> I suspect one ISimpleDOMNode could keep everything in a doc alive.

ideally, maybe one day we can switch to frames and do not keep mContent strong ref. But Trevor is it a right place to keep discussion here that's not very related with this bug. All of this makes harder for contributor to understand what he need to fix here.

> one idea that occured to me is to see if we can abuse the cycle collector
> into telling us when it wants to make the mContent, or for that matter a
> pres shell go away.

might be nice too.

Btw, I'm leaning towards to agree with idea from comment #1. It never worked, ISimpleDOM times passes away and AT has IA2 alternative.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #5)
> > > since we don't control life cycle of COM objects then we should practice
> > > weak refs to accessible objects instead.
> > 
> > what about the mContent pointer? that might actually be a bigger issue since
> > I suspect one ISimpleDOMNode could keep everything in a doc alive.
> 
> ideally, maybe one day we can switch to frames and do not keep mContent
> strong ref. But Trevor is it a right place to keep discussion here that's
> not very related with this bug. All of this makes harder for contributor to
> understand what he need to fix here.

yeah, at this point we're pretty off topic.

> > one idea that occured to me is to see if we can abuse the cycle collector
> > into telling us when it wants to make the mContent, or for that matter a
> > pres shell go away.
> 
> might be nice too.

ok, I'll try and poke smaug

> Btw, I'm leaning towards to agree with idea from comment #1. It never
> worked, ISimpleDOM times passes away and AT has IA2 alternative.

makes sense.

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v1) very rough

Though I see Alex and Trev have helped define this bugs requirements, you're listed as the mentor so I thought I'd start asking for feedback from you.

I've cloned the ScrollTo logic from nsAccessNode into nsCoreUtils and this allowed me to change the calls in both nsAccessible and nsAccessibleWrap from nsAccessNode::ScrollTo to nsCoreUtils::ScrollTo.

(At this point I've left the original logic in  place in nsAccessNode).

Can you comment on the attached, and maybe explain a little further about the final piece where we address nsAccessNodeWrap::scrollTo? Is the thought to clone the scrollTo function into nsCoreUtils alongside ScrollTo? Or to somehow combine them?

Comment 11

[Mark Capella [:capella]]

The attached builds clean locally and passes all mochitest-a11y tests btw...

Comment 12

[Trevor Saunders (:tbsaunde)]

> (At this point I've left the original logic in  place in nsAccessNode).

ok, but part of this bug is to remove that.

> Can you comment on the attached, and maybe explain a little further about
> the final piece where we address nsAccessNodeWrap::scrollTo? Is the thought
> to clone the scrollTo function into nsCoreUtils alongside ScrollTo? Or to
> somehow combine them?

I think per comment 8 you can go with calling the function you add to nsCoreUtils with mContent->OwnerDoc()->GetPresShell() mContent and the type.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 613140 [details] [diff] [review]
Patch (v1) very rough

> nsAccessible::ScrollTo(PRUint32 aHow)
> {
>-  nsAccessNode::ScrollTo(aHow);
>+  nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aHow);

GetFrame()->GetContent() is not always the same as mContent, so this isn't equivilent.

Since you didn't move the IsDefunct() check here in bug 737724 you need to do it now.

>+nsCoreUtils::ScrollTo(nsIPresShell *aPresShell,
>+                      nsIContent *aContent,
>+                      PRUint32 aScrollType)

nit, I'd be suprised if you can't get this on fewer lines.

>+{
>+  nsIPresShell::ScrollAxis vertical, horizontal;
>+  nsCoreUtils::ConvertScrollTypeToPercents(aScrollType, &vertical, &horizontal);

since you're in nsCoreUtils no need for the class qualification.

>+  static void ScrollTo(nsIPresShell* aPresShell,
>+                       nsIContent* aContent,
>+                       PRUint32 aScrollType);

nit, use fewer lines.

> nsAccessibleWrap::scrollTo(enum IA2ScrollType aScrollType)
> {
> __try {
>-  nsAccessNode::ScrollTo(aScrollType);
>+  nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType);

same issues as nsAccessible

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 613140 [details] [diff] [review]
Patch (v1) very rough

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

Looks good on overall

::: accessible/src/base/nsCoreUtils.cpp
@@ +736,5 @@
> +  aPresShell->ScrollContentIntoView(aContent, vertical, horizontal,
> +                               nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
> +}
> +
> +

only one blank line, please.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok, this addresses the nits from hub and tbsaunde ... I've also pulled the code from nsAccessNode.cpp/.h as was desired in the original bug request.

I noticed but haven't done anything with nsApplicationAccesible.h / .cpp ... it defines a "NS_SCRIPTABLE NS_IMETHOD ScrollTo(PRUint32 aScrollType);" that doesn't seem to do anything. I can pull it, unless it's required in a way that I don't understand.

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 613167 [details] [diff] [review]
Patch (v2)

IMETHODIMP
> nsAccessible::ScrollTo(PRUint32 aHow)
> {
>-  nsAccessNode::ScrollTo(aHow);
>+  

THIS ISN'T ADDRESSED, AND SAME FOR THE OTHER CALLERS.

Comment 17

[Mark Capella [:capella]]

Yeah I'm confused ... with the back and forth between you and Alex on the issue it looked like it was decided not to do that ... comment8 "maybe one day we can switch to frames and do not keep mContent strong ref. But Trevor is it a right place ..." ... I don't know enough to have an opinion one way or the other so I'll wait for agreement between you two.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 613167 [details] [diff] [review]
Patch (v2)

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

GetFrame()->GetContent() is not equivalent to mContent but
1) in case of list bullets mContent makes more sense becuase GetFrame()->GetContent() is null and thus no scrolling.
2) in case of document it's not equivalent (when document contains accessible nodes between root html element and html body element) but this is rather an edge case. Also scrolling to html:body might be expected behavior since it's used to define a document role (via ARIA) and thus defines a document semantics. On the another hand no html:body means no scrolling which is not desirable result.

So in either case accessibles should provide own implementation of this method when it's needed. For example, scrollTo for XUL tree item accessible scrolls to XUL tree accessible (for any approach). So I think I'm fine to go with mContent approach for now and file a bug to provide correct implementations.

::: accessible/src/base/nsCoreUtils.cpp
@@ +727,5 @@
>  }
>  
> +void
> +nsCoreUtils::ScrollTo(nsIPresShell *aPresShell, nsIContent *aContent,
> +                      PRUint32 aScrollType)

type* aName

@@ +732,5 @@
> +{
> +  nsIPresShell::ScrollAxis vertical, horizontal;
> +  ConvertScrollTypeToPercents(aScrollType, &vertical, &horizontal);
> +  aPresShell->ScrollContentIntoView(aContent, vertical, horizontal,
> +                               nsIPresShell::SCROLL_OVERFLOW_HIDDEN);

wrong indentation

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +396,5 @@
>    PRUint32 scrollType =
>      aScrollTopLeft ? nsIAccessibleScrollType::SCROLL_TYPE_TOP_LEFT :
>                       nsIAccessibleScrollType::SCROLL_TYPE_BOTTOM_RIGHT;
>  
> +  nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, scrollType);

I'm fine with either case for now. You could do mContent->OwnerDoc()->GetPresShell() but please add a comment that it isn't supposed to work in multi presshell documents.

Comment 19

[alexander :surkov (:asurkov)]

Mark, you might want to upload a patch and ask Hub or Trevor for review or just ask for this one

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> Comment on attachment 613167 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 613167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> GetFrame()->GetContent() is not equivalent to mContent but
> 1) in case of list bullets mContent makes more sense becuase
> GetFrame()->GetContent() is null and thus no scrolling.
> 2) in case of document it's not equivalent (when document contains
> accessible nodes between root html element and html body element) but this
> is rather an edge case. Also scrolling to html:body might be expected
> behavior since it's used to define a document role (via ARIA) and thus
> defines a document semantics. On the another hand no html:body means no
> scrolling which is not desirable result.
> 
> So in either case accessibles should provide own implementation of this
> method when it's needed. For example, scrollTo for XUL tree item accessible
> scrolls to XUL tree accessible (for any approach). So I think I'm fine to go
> with mContent approach for now and file a bug to provide correct
> implementations.

OK, i GUES THAT MAKES SENSE.

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #17)
> Yeah I'm confused ... with the back and forth between you and Alex on the
> issue it looked like it was decided not to do that ... comment8 "maybe one
> day we can switch to frames and do not keep mContent strong ref. But Trevor
> is it a right place ..." ... I don't know enough to have an opinion one way
> or the other so I'll wait for agreement between you two.

that isn't completely related, in any case if you don't check the accessible is not defunct before calling nsCoreUtils::ScrollTo() you'll crash.

Comment 22

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok, with Alexs' feedback+ and the last few nits fixed, let's ask Hub to do the review?

Comment 23

[Hubert Figuiere [:hub]]

Comment on attachment 613245 [details] [diff] [review]
Patch (v3)

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

A couple of trivial things needs to be addressed.

And the patch needs to be rebased as well.

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +396,5 @@
>    PRUint32 scrollType =
>      aScrollTopLeft ? nsIAccessibleScrollType::SCROLL_TYPE_TOP_LEFT :
>                       nsIAccessibleScrollType::SCROLL_TYPE_BOTTOM_RIGHT;
>  
> +  nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, scrollType);

In comment #18, surkov asked you to add a comment that it isn't supposed to work in multi presshell documents.

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1143,5 @@
>  STDMETHODIMP
>  nsAccessibleWrap::scrollTo(enum IA2ScrollType aScrollType)
>  {
>  __try {
> +  nsCoreUtils::ScrollTo(mDoc->PresShell(), mContent, aScrollType);

likely the same as above.

Comment 24

[Mark Capella [:capella]]

Oh, Surkovs comment was "I'm fine with either case for now. You could do mContent->OwnerDoc()->GetPresShell() but please add a comment that it isn't supposed to work in multi presshell documents." (Also see comment2)

I didn't do this case, so I didn't add the comment.

Comment 25

[Hubert Figuiere [:hub]]

(In reply to Mark Capella [:capella] from comment #24)
> Oh, Surkovs comment was "I'm fine with either case for now. You could do
> mContent->OwnerDoc()->GetPresShell() but please add a comment that it isn't
> supposed to work in multi presshell documents." (Also see comment2)
> 
> I didn't do this case, so I didn't add the comment.

Maybe I misunderstood. I apologize. We need a rebased patch though.

Comment 26

[Mark Capella [:capella]]

Attachment: Patch (v4)

Ok, try try again. I've pulled a current repo, re-based the patch, built and mochitest tested it successfully. This should be good to go ;)

Comment 27

[Hubert Figuiere [:hub]]

Mmm... The need to rebase rose from the fix for bug 539683 that is still on inbound. (I build against inbound)....

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #25)
> (In reply to Mark Capella [:capella] from comment #24)
> > Oh, Surkovs comment was "I'm fine with either case for now. You could do
> > mContent->OwnerDoc()->GetPresShell() but please add a comment that it isn't
> > supposed to work in multi presshell documents." (Also see comment2)
> > 
> > I didn't do this case, so I didn't add the comment.
> 
> Maybe I misunderstood. I apologize. We need a rebased patch though.

I guess you can require that as a reviewer if you like, but its not very typical, I'd certainly be ok with someone rebasing this against bug 539683 after my r+.

Comment 29

[Hubert Figuiere [:hub]]

Comment on attachment 613450 [details] [diff] [review]
Patch (v4)

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

But we need a rebased patch for commit now that the patch making this needed has landed.

Comment 30

[Hubert Figuiere [:hub]]

Forget about the rebasing. I'll do it and check it in.

Comment 31

[Hubert Figuiere [:hub]]

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

Comment 32

[Hubert Figuiere [:hub]]

backed out. will re-land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df10e8b5ce2

Comment 33

[Hubert Figuiere [:hub]]

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

Comment 34

[Matt Brubeck (:mbrubeck)]

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