Closed Bug 391617 Opened 13 years ago Closed 13 years ago

CAccessibleText::scroll* methods are incorrect

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 4 obsolete files)

These methods should scroll to a substring.

They get the child accessible for the nsIAccessibleText() via GetAttributeRange(),  when they should really not count on the idea of an attribute range having it's own accessible.

In any case, it wants the dom node and offsets to scroll to, and then either reuse nsTextAccessibleWrap::scrolToSubstring() or do something like that.
Dupe of bug 371680?
Assignee: aaronleventhal → surkov.alexander
(In reply to comment #1)
> Dupe of bug 371680?
> 

Not exactly. This one is a bit wider.
Surkov, did you take care of this via a different bug?
(In reply to comment #3)
> Surkov, did you take care of this via a different bug?
> 

No, scrollSubstringToPoint is implemented via GetAttributeRange() still.
Severity: normal → major
Attached patch in progress (obsolete) — Splinter Review
Attached patch in progress2 (obsolete) — Splinter Review
Attachment #280890 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #280989 - Attachment is obsolete: true
Attachment #280994 - Flags: review?(aaronleventhal)
Status: NEW → ASSIGNED
Surkov, could you provide a summary of changes, either as the patch name or in a comment? This is generally a good idea, something like:
1) Move method foo to nsAccUtils because blah
2) Change loop to break when bar is true, because blah
3) etc.

It makes it much easier to review, IMO. I try to remember to do that myself.
Comment on attachment 280994 [details] [diff] [review]
patch

Waiting for patch description.
Attachment #280994 - Flags: review?(aaronleventhal)
Comment on attachment 280994 [details] [diff] [review]
patch


> /**
>- * These constants define which coordinate system a point is located in. Note,
>- * keep them synchronized with IA2CoordinateType.
>+ * These constants define which coordinate system a point is located in.
>  */

Actually they are no longer synchronized.

> [scriptable, uuid(c9fbdf10-619e-436f-bf4b-8566686f1577)]
> interface nsIAccessibleCoordinateType : nsISupports
> {
>   /**
>    * The coordinates are relative to the screen.
>    */
>   const unsigned long COORDTYPE_SCREEN_RELATIVE = 0x00;
>Index: accessible/src/base/nsAccessNode.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v
>retrieving revision 1.62
>diff -u -p -8 -r1.62 nsAccessNode.cpp
>--- accessible/src/base/nsAccessNode.cpp	5 Sep 2007 07:39:10 -0000	1.62
>+++ accessible/src/base/nsAccessNode.cpp	15 Sep 2007 11:39:16 -0000
>@@ -434,76 +434,37 @@ nsAccessNode::ScrollTo(PRUint32 aScrollT
> 
> NS_IMETHODIMP
> nsAccessNode::ScrollToPoint(PRUint32 aCoordinateType, PRInt32 aX, PRInt32 aY)
> {
>   nsIFrame *frame = GetFrame();
>   if (!frame)
>     return NS_ERROR_FAILURE;
> 
>-  nsPresContext *presContext = frame->PresContext();
>-
>-  switch (aCoordinateType) {
>-    case nsIAccessibleCoordinateType::COORDTYPE_SCREEN_RELATIVE:
>-      break;

move to new accUtils method to reuse it scrollSubstringTo

>Index: accessible/src/base/nsAccessibilityUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.cpp,v
>retrieving revision 1.11
>diff -u -p -8 -r1.11 nsAccessibilityUtils.cpp
>--- accessible/src/base/nsAccessibilityUtils.cpp	5 Sep 2007 07:39:10 -0000	1.11
>+++ accessible/src/base/nsAccessibilityUtils.cpp	15 Sep 2007 11:39:16 -0000
>@@ -34,16 +34,17 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAccessibilityUtils.h"
> 
> #include "nsIAccessibleTypes.h"
>+#include "nsPIAccessNode.h"
> #include "nsPIAccessible.h"
> #include "nsAccessibleEventData.h"
> 
> #include "nsIDocument.h"
> #include "nsIDOMAbstractView.h"
> #include "nsIDOMDocument.h"
> #include "nsIDOMDocumentView.h"
> #include "nsIDOMRange.h"
>@@ -276,16 +277,29 @@ nsAccUtils::GetAncestorWithRole(nsIAcces
> }
> 
> nsresult
> nsAccUtils::ScrollSubstringTo(nsIFrame *aFrame,
>                               nsIDOMNode *aStartNode, PRInt32 aStartIndex,
>                               nsIDOMNode *aEndNode, PRInt32 aEndIndex,
>                               PRUint32 aScrollType)
> {
>+  PRInt16 vPercent, hPercent;
>+  ConvertScrollTypeToPercents(aScrollType, &vPercent, &hPercent);
>+
>+  return ScrollSubstringTo(aFrame, aStartNode, aStartIndex, aEndNode, aEndIndex,
>+                           vPercent, hPercent);
>+}
>+
>+nsresult
>+nsAccUtils::ScrollSubstringTo(nsIFrame *aFrame,
>+                              nsIDOMNode *aStartNode, PRInt32 aStartIndex,
>+                              nsIDOMNode *aEndNode, PRInt32 aEndIndex,
>+                              PRInt16 aVPercent, PRInt16 aHPercent)
>+{

Have two version of methods, one is for scrollSubstringTo, another is for scrollSubstringToPoint.

>+nsresult
>+nsHyperTextAccessible::SubstringToDOMRange(PRInt32 aStartHTOffset,
>+                                           PRInt32 aEndHTOffset,
>+                                           nsIDOMNode **aStartNode,
>+                                           PRInt32 *aStartOffset,
>+                                           nsIDOMNode **aEndNode,
>+                                           PRInt32 *aEndOffset)
>+{

move code to new method method to use it in scrollSubstringTo[Point] methods

>+  nsPresContext *presContext = frame->PresContext();
>+
>+  PRBool initialScrolled = PR_FALSE;
>+  nsIFrame *parentFrame = frame;
>+  while (parentFrame = parentFrame->GetParent()) {
>+    nsIScrollableFrame *scrollableFrame = nsnull;
>+    CallQueryInterface(parentFrame, &scrollableFrame);
>+    if (scrollableFrame) {
>+      if (!initialScrolled) {
>+        nsIntRect frameRect = parentFrame->GetScreenRectExternal();
>+        PRInt32 devOffsetX = coords.x - frameRect.x;
>+        PRInt32 devOffsetY = coords.y - frameRect.y;
>+
>+        nsPoint offsetPoint(presContext->DevPixelsToAppUnits(devOffsetX),
>+                            presContext->DevPixelsToAppUnits(devOffsetY));
>+
>+        nsSize size(parentFrame->GetSize());
>+        PRInt16 hPercent = offsetPoint.x * 100 / size.width;
>+        PRInt16 vPercent = offsetPoint.y * 100 / size.height;
>+
>+        rv=  nsAccUtils::ScrollSubstringTo(GetFrame(), startNode, startOffset,
>+                                           endNode, endOffset,
>+                                           vPercent, hPercent);

Turn point to percents relative scrollable area to use nsISelection2::scrollIntoView.

>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        initialScrolled = PR_TRUE;
>+      } else {
>+        nsIntRect frameRect = frame->GetScreenRectExternal();
>+        PRInt32 devDeltaX = coords.x - frameRect.x;
>+        PRInt32 devDeltaY = coords.y - frameRect.y;
>+
>+        nsPoint deltaPoint(presContext->DevPixelsToAppUnits(devDeltaX),
>+                           presContext->DevPixelsToAppUnits(devDeltaY));
>+        nsPoint scrollPoint = scrollableFrame->GetScrollPosition();
>+        scrollPoint -= deltaPoint;
>+
>+        scrollableFrame->ScrollTo(scrollPoint);

If there are another scrollable areas then scroll previous scrollable area into view of top scrollable area.

> STDMETHODIMP
> CAccessibleText::scrollSubstringToPoint(long aStartIndex, long aEndIndex,
>-                                        enum IA2CoordinateType aCoordinateType,
>+                                        enum IA2CoordinateType aCoordType,
>                                         long aX, long aY)
> {

>-  IAccessible2 *pAccessible2 = static_cast<IAccessible2*>(*instancePtr);
>-  HRESULT hr = pAccessible2->scrollToPoint(aCoordinateType, aX, aY);
>-  pAccessible2->Release();
>-
>-  return hr;
>+  PRUint32 geckoCoordType = (aCoordType == IA2_COORDTYPE_SCREEN_RELATIVE) ?
>+    nsIAccessibleCoordinateType::COORDTYPE_SCREEN_RELATIVE :
>+    nsIAccessibleCoordinateType::COORDTYPE_PARENT_RELATIVE;
>+
>+  nsresult rv = textAcc->ScrollSubstringToPoint(aStartIndex, aEndIndex,
>+                                                geckoCoordType, aX, aY);
>+  return NS_FAILED(rv) ? E_FAIL : S_OK;
> }

Use new method.

> STDMETHODIMP
>-nsAccessibleWrap::scrollToPoint(enum IA2CoordinateType coordinateType,
>-                                long x, long y)
>+nsAccessibleWrap::scrollToPoint(enum IA2CoordinateType aCoordType,
>+                                long aX, long aY)
> {
>-  return E_NOTIMPL;
>+  PRUint32 geckoCoordType = (aCoordType == IA2_COORDTYPE_SCREEN_RELATIVE) ?
>+    nsIAccessibleCoordinateType::COORDTYPE_SCREEN_RELATIVE :
>+    nsIAccessibleCoordinateType::COORDTYPE_PARENT_RELATIVE;

gecko and IA2 coord types are not in sync more.

>+  return NS_SUCCEEDED(ScrollToPoint(geckoCoordType, aX, aY)) ?
>+    S_OK : E_FAIL;

and eventually I forgot to implement the method.
Attachment #280994 - Flags: review?(aaronleventhal)
Shouldn't SubstringToDOMRange() then be used by SetSelectionRange(). Can you take a look and compare how they both do their work and see if it is the same, and if diferent, why are they different?
I think most of the code is immediately clear, except for nsHyperTextAccessible::ScrollSubstringToPoint()

This comment did not help me understand it:
> If there are another scrollable areas then scroll previous scrollable area into
> view of top scrollable area.

Can you attach a new patch where you put plenty of comments inside that method?
(In reply to comment #11)
> Shouldn't SubstringToDOMRange() then be used by SetSelectionRange(). Can you
> take a look and compare how they both do their work and see if it is the same,
> and if diferent, why are they different?
> 

At quick sight they are much similar. I think SetSelectionRange() can use SubstringToDOMRange.
Lets make sure we reuse the smarter one.
Attached patch patch2 (obsolete) — Splinter Review
1) add comments to scrollSubstringToPoint
3) check for collapsed range inside SubstringToDOMRange()
2) reuse SubstringToDOMRange() for SetSelectionBounds()
Attachment #280994 - Attachment is obsolete: true
Attachment #281088 - Flags: review?(aaronleventhal)
Attachment #280994 - Flags: review?(aaronleventhal)
Can you call it
HyperTextOffsetToDOMRange?

That's more consistent with our DOMPointToHyperTextOffset naming.
(In reply to comment #16)
> Can you call it
> HyperTextOffsetToDOMRange?
> 
> That's more consistent with our DOMPointToHyperTextOffset naming.
> 

I thought about this. Then probably HyperTextOffset[s]ToDOMRange. Though substring is used often enough in code and substring is shorter. But if you like I will, I really haven't big preferences.
Right, HypertextOffsetsToDOMRange() (check the caps of the other places we use HypertextOffset).

Also, I forgot to mention bug 394689. It might be work to merge with that. I had to do some work to deal with the offset for the end of a hypertext, because frame comes back as nsnull. It also supports -1 to mean end of hypertext.
I see potential code reuse between nsAccessNode::ScrollTo() and nsHyperTextAccessible::ScrollSubstringToPoint().

I think ScrollSubStringToPoint() could possibly call nsAccessNode::ScrollTo() after the substring scroll. The code that walks the chain of ancestor frames looks exactly the same.
(In reply to comment #19)
> I see potential code reuse between nsAccessNode::ScrollTo() and
> nsHyperTextAccessible::ScrollSubstringToPoint().
> 
> I think ScrollSubStringToPoint() could possibly call nsAccessNode::ScrollTo()
> after the substring scroll. The code that walks the chain of ancestor frames
> looks exactly the same.
> 

Ok. Possibly it's worth to have util method that can be shared between scrollTo and scrollSubstringToPoint.
(In reply to comment #18)

> Also, I forgot to mention bug 394689. It might be work to merge with that. I
> had to do some work to deal with the offset for the end of a hypertext, because
> frame comes back as nsnull. It also supports -1 to mean end of hypertext.
> 

I'm not fun of merging of patches but I should take the logic from your patch and move it here.
Attached patch patch3Splinter Review
1) added utiliy method to be shared between scrollToPoint and scrollSubstringToPoint
2) merged with changed of bug 394689
3) updated to trunk
Attachment #281088 - Attachment is obsolete: true
Attachment #281430 - Flags: review?(aaronleventhal)
Attachment #281088 - Flags: review?(aaronleventhal)
How did you test?

I guess, isOnlyCaret should be a PRBool.
Attachment #281430 - Flags: superreview?(neil)
Attachment #281430 - Flags: review?(aaronleventhal)
Attachment #281430 - Flags: review+
Attached file testcase
Comment on attachment 281430 [details] [diff] [review]
patch3

I'm not sure why you asked for my sr on this; the code moves look vaguely ok but I don't really understand it.
Attachment #281430 - Flags: superreview?(neil)
Neil, I asked for your sr= because you're good at spotting coding issues in patches. It's big enough that I wanted a second pair of eyes at this late stage.
(In reply to comment #26)
>It's big enough that I wanted a second pair of eyes at this late stage.
Well it touches lots of layout stuff so maybe a layout peer should look?
Sorry, I mean uses, not touches...
Yeah, but he's already gotten signoff on his approach from roc before. This just reorganizes it and doesn't do anything new with that.

Having your opinion on the code organization here would be valuable.
OK, so I had another look and all I spotted was a measly rv= spacing error.
Attachment #281430 - Flags: approval1.9?
Attachment #281430 - Flags: approval1.9? → approval1.9+
checked in with Aaron's and Neil's nits.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.