377302 - implement nsIAccessNode::scrollToPoint

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

It's not right logic of nsIAccessNode::scrollToPoint implementation but it's a sort of approximation of desired behavior. It's hard to me to implement correct behavior without strong knowledge of layout code. If you'll see how can I be more right then please advise me.

Comment 2

[[e-mail]]

Comment on attachment 263473 [details] [diff] [review]
patch

You really need someone like roc to look at the use of layout code like this.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What are WINDOW_RELATIVE and PARENT_RELATIVE supposed to mean?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this supposed to scroll an acessible node to a particular point on the screen? That seems like a weird API.

Comment 5

[Aaron Leventhal]

> What are WINDOW_RELATIVE and PARENT_RELATIVE supposed to mean?
WINDOW_RELATIVE means the coordinates are relative to the top level window
PARENT_RELATIVE means the coordinates are relative to the parent accessible

> Is this supposed to scroll an acessible node to a particular point on the
> screen? That seems like a weird API.
It's for screen magnification software, which likes to have very exact control as it reads through a document, highlighting words and speaking as it goes.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd prefer code that walked the frame tree looking for nsIScrollableFrames.

Comment 7

[Aaron Leventhal]

Comment on attachment 263473 [details] [diff] [review]
patch

Clearing my request until Surkov responds to that.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

usage of nsIScrollableFrame. The patch doesn't work as I expected. It looks frame bounds are relative to position of frame on screen. It's difference from nsIScrollableView usage.

Robert, can you say what's wrong?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        nsIntRect screenRect = frame->GetScreenRectExternal();
+        x -= screenRect.x;
+        y -= screenRect.y;

I don't think this is right. after this 'switch' you want x and y to be window-relative, right? So you need to subtract the *window*'s top-left.

But I don't really understand how this patch is supposed to work at all. It ignores x and y completely.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe it would help if someone pointed me to the specification that defines exactly what this method is supposed to do.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Maybe it would help if someone pointed me to the specification that defines
> exactly what this method is supposed to do.
> 

/**
   * Moves the top left of an object to a specified location.
   *
   * @param coordinateType - specifies whether the coordinates are relative to
   *                         the screen or the parent object (for available
   *                         constants refer to nsIAccessibleCoordinateType)
   * @param aX - defines the x coordinate
   * @param aY - defines the y coordinate
  */
  void scrollToPoint(in unsigned long aCoordinateType, in long aX, in long aY);

(http://lxr.mozilla.org/mozilla/source/accessible/public/nsIAccessNode.idl#136)

Comment 12

[alexander :surkov (:asurkov)]

Originally the method goes from IA2 (http://lxr.mozilla.org/mozilla/source/other-licenses/ia2/Accessible2.idl#177)

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> But I don't really understand how this patch is supposed to work at all. It
> ignores x and y completely.
> 

Yes, the patch2 is actually test patch. I think it should scroll the page to show top left edge of the frame. But it doesn't do it.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then you just want to scroll to (0,0) to get that effect.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> Then you just want to scroll to (0,0) to get that effect.
> 

If the frame is contained by couple of non scrollable parent frames then (0, 0) will scroll top parent non scrollable frame to top left edge, right?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah OK.

One problem with this code is that the scrolled frame's rect is positioned at the current scroll point. Consider the case where the accessible is the scrolled frame and things are scrolled down by ten pixels: the scrolled frame is at (0,-600). You are going to try to scroll to (0,-600) which isn't right.

So you need to walk up the frame tree; when you see a scrolled frame, you should explicitly compute where you want to scroll the child to using nsIFrame::GetOffsetTo --- this will simplify the code. If you just want to scroll to the top-left of targetFrame, you could set toPoint = targetFrame->GetOffsetTo(scrollableFrame->GetScrolledFrame()) --- the offset of targetFrame's top-left within the scrolled frame.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> +        nsIntRect screenRect = frame->GetScreenRectExternal();
> +        x -= screenRect.x;
> +        y -= screenRect.y;
> 
> I don't think this is right. after this 'switch' you want x and y to be
> window-relative, right? So you need to subtract the *window*'s top-left.

How can I obtain top level window?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't you get it somewhere else in accessibility already?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Don't you get it somewhere else in accessibility already?
> 

No, if I'm right. Will do nsIPresShell::GetRootFrame() return frame for top level window?

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch3

patch should move top left edge of frame to the given point as much as possible. It doesn't do it :). I think toPoint should be <= frame->GetOffsetToExternal(scrollableFrame->GetScrolledFrame()). It looks it isn't. Robert, can you help me to understnd what's wrong in patch logic? Thank you.

Comment 21

[Aaron Leventhal]

Surkov, GetRootFrame is just for the root frame in the  current window. To get the top level window, you can use nsIDocShellTreeItem::GetRootTreeItem() or you can use nsAccessNode::GetRootAccessible(). From the root accessible you can get things like the DOM node, frame or DOM Window. You could use DOMWindowInternal::screenX/screenY etc.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> Surkov, GetRootFrame is just for the root frame in the  current window.

Thanks.

> To get
> the top level window, you can use nsIDocShellTreeItem::GetRootTreeItem()

I can't find how to get document of presshell from nsIDocShellTreeItem.

> or you
> can use nsAccessNode::GetRootAccessible(). From the root accessible you can get
> things like the DOM node, frame or DOM Window. You could use
> DOMWindowInternal::screenX/screenY etc.
> 

Ok, probably this is the best.

Comment 23

[Aaron Leventhal]

We have code that does the same thing right here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsMaiInterfaceComponent.cpp#95

Maybe it just needs to be moved into thebase.

Comment 24

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 25

[Aaron Leventhal]

Comment on attachment 276459 [details] [diff] [review]
patch

r+ for the code reorge, but where's the actual nsAccessNode::scrollToPoint impl?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (From update of attachment 276459 [details] [diff] [review])
> r+ for the code reorge, but where's the actual nsAccessNode::scrollToPoint
> impl?
> 

in nsAccessNode::ScrollToPoint or what do you mean?

Comment 27

[Aaron Leventhal]

Somehow I missed that before. Did you find a way to test this?

Comment 28

[alexander :surkov (:asurkov)]

Attachment: testcase

I tested the patch with this testcase.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsPresContext *presContext = GetPresContext();
+  if (!presContext)
+    return NS_ERROR_FAILURE;

you could just use frame->PresContext(), and then you won't have to null-check.

The way ScrollToPoint uses screen coordinates will be slow on Linux because window-to-screen conversions are slow there.

You should use nsIDeviceContext::UnscaledAppUnitsPerDevPixel() when converting screen coordinates to/from appunits, otherwise this will break with full-page-zoom.

+nsAccUtils::ScreenToWindowCoords(nsIDOMNode *aNode,
+                                 PRInt32 *aX, PRInt32 *aY)
+nsAccUtils::WindowToScreenCoords(nsIDOMNode *aNode,
+                                 PRInt32 *aX, PRInt32 *aY)
+nsAccUtils::GetScreenCoordsForWindow(nsIDOMNode *aNode,
+                                     PRInt32 *aX, PRInt32 *aY)

How about not using COM style here? Actually I think you should return an nsIntPoint from GetScreenCoordsForWindow, and scrap ScreenToWindowCoords/WindowToScreenCoords; callers can just add/subtract the result of GetScreenCoordsForWindow.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> The way ScrollToPoint uses screen coordinates will be slow on Linux because
> window-to-screen conversions are slow there.

What code exactly is slow on Linux?

But in any way in meantime the method won't be used on linux. So I think I can add XXX section for now.

> You should use nsIDeviceContext::UnscaledAppUnitsPerDevPixel() when converting
> screen coordinates to/from appunits, otherwise this will break with
> full-page-zoom.

Can you clarify a rule when to use unscaled version of AppUnitsPerDevPixel?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsIWidget::WidgetToScreen/ScreenToWidget and their callers.

> Can you clarify a rule when to use unscaled version of AppUnitsPerDevPixel?

When you're converting appunits to and from screen coordinates. When we're converting to and from "device coordinates" for drawing, we use the regular AppUnitsPerDevPixel.

Comment 32

[Aaron Leventhal]

We don't actually need this particular method to be fast. It only needs to keep up with a text to speech stream -- one scroll operation per word that is read. So at most a few per second.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch5

fixed roc's comments
added implementation of COORDTYPE_PARENT_RELATIVE

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      nsCOMPtr<nsIAccessible> accessible;
+      nsresult rv = QueryInterface(NS_GET_IID(nsIAccessible),
+                                   getter_AddRefs(accessible));

Why aren't you doing
  nsCOMPtr<nsIAccessible> accessible = do_QueryInterface(this);
?

Sorry, I gave you bad advice. You don't want to use UnscaledAppUnitsPerDevPixel() here. Sorry! Sorry. Just use DevPixelsToAppUnits as normal.

> When you're converting appunits to and from screen coordinates. When we're
> converting to and from "device coordinates" for drawing, we use the regular
> AppUnitsPerDevPixel.

My comment was wrong. We only need to use UnscaledAppUnitsPerDevPixel when we're converting to/from screen coordinates *and we don't want the Web content to know it's been scaled* so we pretend we aren't. In this case, the accessibility client just wants us to scroll to the screen position regardless of scaling. You should test with full zoom:
http://mozillalinks.org/wp/2007/07/try-firefox-3-full-page-zoom-with-full-page-zoom/

+      scrollPoint.x -= deltaPoint.x;
+      scrollPoint.y -= deltaPoint.y;

scrollPoint -= deltaPoint;

+  if (!treeItem)
+    return coords;

These are returning uninitialized values; do "nsIntPoint coords(0,0);" to avoid that.

Otherwise good.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> +      nsCOMPtr<nsIAccessible> accessible;
> +      nsresult rv = QueryInterface(NS_GET_IID(nsIAccessible),
> +                                   getter_AddRefs(accessible));
> 
> Why aren't you doing
>   nsCOMPtr<nsIAccessible> accessible = do_QueryInterface(this);
> ?

I just got conversion error here.

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch7

updated to trunk

Comment 39

[Damon Sicore (:damons)]

Just one request:  Can we wait on asking for approval until all reviews are completed, please?  It will speed up the approval process a bit, I think. 

Comment 40

[Damon Sicore (:damons)]

Comment on attachment 278788 [details] [diff] [review]
patch7

-'ing the approval until reviews are completed.  Just re-request approval when done.  This way, I can quickly clear out the queue.  :)

Comment 41

[alexander :surkov (:asurkov)]

checked in