297697 - ISimpleDOMText.get_unclippedSubstringBounds has trouble with character 1 of wrapped line 2

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tim McHyde]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050531 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050531 Firefox/1.0+

Get_UnclippedSubstringBounds returns an X that is offset too far right when
compared to the original mouse Point.X passed to AccessibleObjectFromPoint in
retrived the ISimpleDOMText (although Y is right as is the string returned from
Get_DOMtext)

Reproducible: Always

Steps to Reproduce:
1. Unzip the test demo (3 files) 
2. Open the html file in Deer Park Alpha 1
3. Run the exe and move the mouse over the document

Actual Results:  
Observe that Get_UnclippedSubstringBounds returns an X that is offset too far
right, although the string returned is always under the current mouse position.

To see this try points in the text in the first paragraph where the words are
italics (and get their own short textnode apparently..a clue?) or these specific
text cases always come up not in bounds:

or Justin's 

@@1:3~~ and 1 Pet 1:8
 
BTW, I have my browser sized to around 800 wide, certain points that go bad seem
to be good or bad depending on where they wrap based on width. 

Expected Results:  
the rectangle returned should always contain the X,Y of the mouse cursor
position as sent to AccessibleObjectFromPoint to get the ISimpleDOMTExt

Aaron Levanthal says this is no doubt a bug since no one has used this method yet.

Email me if more details are needed.  I'm happy to assist in any way to get this
fixed for 1.1 if possible!

Comment 1

[Tim McHyde]

Attachment: small windows demonstration program

Comment 2

[Tim McHyde]

Attachment: Latest Improved bug demo, use this one

load the htm into Deer Park Alpha and run the exe and run it over the words in
the document and observe how the rectangle (and I realize now, even the text)
does not corrolate to the Mouse Pt X,Y passed often.  Seems like extra "leaf"
text nodes for italics and such throws things off.

Comment 3

[Tim McHyde]

I understand the bug much better now, having come up with a workaround, in fact
I would rename it to be "ISimpleDOMText.get_unclippedSubstringBounds always
gives same Rect.Top for all lines of text"

The workaround I use is to loop through each word of the text for the node
calling for the bounds of each word.  Each time I find a negative Rect.Width
(happens with the first word of each wrapped line) I know I've hit another line
and increment a OffsetY by Rect.Height + 3 (the 3 seems to be fairly standard
gap between lines).  The OffsetY is added to the Rect.Top for every call for the
bounds of any word in node's text. 

BTW, is the -Width a bug when you are getting it for a single word on one line?
   I assume once this bug is fixed that a -Width would be returned if you ask
for a range of text that spans two lines where the Left > the Right rect bounds?

Comment 4

[Aaron Leventhal]

Attachment: Add offset specific line within block

Comment 5

[Tim McHyde]

(In reply to comment #4)k
Great, I see my further detail helped get it some cycles...but how does this
correct the -Width I'm seeing when getting the bounds for indexes representing
the first word (or just the first character) of a line?  Have you seen that or
that's something else you'll have to postpone for later

Also, on this fix do you build and test on windows?  Is there anyway you could
get me a drop-in binary to test since I'm not setup to build and I expect it
would take me hours to figure it out.

Comment 6

[Aaron Leventhal]

I don't know if this fixes the other problem you're seeing yet but it might.

This fix is definitely the right thing. Who knows we might need to do more. I'll
get this in and then will point you to a build with the fix for you to test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So what coordinate system is the point coming out of this method supposed to be
in?  What's aContainingFrame here?

Comment 8

[Aaron Leventhal]

(In reply to comment #7)
> So what coordinate system is the point coming out of this method supposed to be
> in?  

In the coordinate system of the text node / text accessible.

> What's aContainingFrame here?
The primary text frame. Perhaps that's a misnamed variable. Then again
GetChildFrameContainingOffset() seems to be misnamed because it can give you a
next-in-flow frame -- that is not actually a child of the primary frame is it?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> In the coordinate system of the text node / text accessible.

The former has no coordinate system.  What's the coordinate system of the latter?

And yes, what GetChildFrameContainingOffset does on textframes is rather odd
given the name...

What I don't understand is why we're adding in the position of textFrame,
exactly.  Hence my question about coordinate systems.

Comment 10

[Aaron Leventhal]

Okay, the accessible coordinate system is the same as the primary frame's.

Perhaps we should be adding the next in flow position but also subtracting the
primary frames, just in case it's not 0, 0 ?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Okay, the accessible coordinate system is the same as the primary frame's.

In that case, just use nsIFrame::GetOffsetTo appropriately?

Comment 12

[Aaron Leventhal]

Attachment: Use GetOffsetTo

Comment 13

[Aaron Leventhal]

Checking in nsTextAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsTextAccessibleWrap.cpp,v  <-- 
nsTextAccessibleWrap.cpp
new revision: 1.13; previous revision: 1.12
done

Comment 14

[Tim McHyde]

Using the 8-23 nightly, I'm sorry to report that the bug is not fixed, but
actually worse overall:
* X is now no longer correctly resetting with the first character of each line,
but waits until the 2nd character of the line to do so (I know this sounds like
an off by one error on my part but i checked and double checked my startindex)
* Y offset now changes according to wrapped lines per Aaron's change, but also
not until after the first character index of a line
* Height now is always negative after the first wrapped line even for startindex
- endindex representing a single word spanning a single line

Comment 15

[Aaron Leventhal]

Attachment: Fix aX, aY, aWidth, aHeight calculations

This time I used a better testing methodology.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So.. how's this different from startRect.UnionRect(endRect)?

Comment 17

[Aaron Leventhal]

(In reply to comment #16)
> So.. how's this different from startRect.UnionRect(endRect)?

Those are the entire text frame. We're interested only in the bounds from the
start character to the end character.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow.

For example, why if startFrame != endFrame, do we make *aX equal to
2*startRect.x?  That seems really odd.

So could you describe in words exactly what rectangle you're trying to return here?

Comment 19

[Aaron Leventhal]

Attachment: Clearer patch

We want the minimal rect that contains all the text from start to end offset.

Two possibilities:
if (startFrame = endFrame) then we want the rectangle that encompases the
startPoint, and the (endPoint + the height of the endFrame)
else
we want the union of the start and end rects, because that will contain all the
text

Comment 20

[Aaron Leventhal]

Attachment: Slightly smaller patch, perhaps better

Comment 21

[Aaron Leventhal]

Comment on attachment 191354 [details] [diff] [review]
Clearer patch

I think this is the clearest patch, although the one after this is smaller and
also works. Might want to consider that one also.

Comment 22

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

(In reply to comment #19)
> We want the minimal rect that contains all the text from start to end offset.

If this is true then I think you want a quite different calculation. For
example, what if you have text broken like this?

-----xx
xxxxxxx
xx-----

or like this (two columns)?

-------  xx-----
-------  -------
-----xx  -------

I think you need something like this:
  nsRect firstRect = firstFrame->GetRect();
  firstRect.x += startOffset; firstRect.width -= startOffset;
  nsRect lastRect = lastFrame->GetRect();
  lastRect.width = endOffset;
  
  nsRect result(0,0,0,0);
  result.UnionRect(result, firstRect);
  result.UnionRect(result. lastRect);
  for (nsIFrame* f = firstFrame->GetNextInFlow(); f != lastFrame; f =
f->GetNextInFlow()) {
    result.UnionRect(result, f->GetRect());
  }

Comment 23

[Aaron Leventhal]

Attachment: Use roc's suggestion

Comment 24

[Aaron Leventhal]

Oh, of course I'll remove the commented out code.

Comment 25

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

Comment on attachment 192218 [details] [diff] [review]
Use roc's suggestion

Unfortunately my suggestion isn't right because those rects might have
different parents and hence be in different coordinate systems. You need to
translate each rect into the final coordinate system you want, and then union
each of those.

Comment 26

[Aaron Leventhal]

If we check in this patch we're basically supporting more than we need. This
funcationality is used for word highlighting as text is read via text-to-speech.
The only known use case is 1 word at a time.

Can you explain a little more about the different parents for the
GetNextInFlow()'s? Will they all be from the same dom node?

Comment 27

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

I'd hate to check in code that is wrong just because it's right for the only use
we know of.

> the different parents for the GetNextInFlow()'s? Will they all be from the same
> dom node?

Yes.

My column example is a good example for this. Each column has its own block
frame. The first inline is a child of the first column and the second inline is
a child of the second column.

Another example would be nested spans:
<span>aaaa <span>bb bb</span> aaaa</span>
---aaaa bb
bb aaaa---

the first 'bb' frame is a child of the first frame for the outer span, the
second bb frame is a child of the second frame for the outer span.

Comment 28

[Aaron Leventhal]

Ok, new patch coming.... trying to test it now.

Comment 29

[Aaron Leventhal]

Attachment: Union screen rects

Comment 30

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

Why aren't you handling start and end offsets in the multi-frame case?

Comment 31

[Aaron Leventhal]

Attachment: Use startPoint and endPoint even when more than one frame involved

Comment 32

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

+  nsRect startRect = nsRect(startPoint, nsSize(firstEndPoint - startPoint.x,
+                                        endFrame->GetSize().height));

This doesn't look right ... startPoint is relative to startFrame but later you
treat startRect as being relative to "frame".

I would do something like this:

nsRect sum(0,0,0,0);
for (nsIFrame* f = startFrame; f != endFrame->GetNextInFlow(); f =
f->GetNextInFlow()) {
  nsRect r = f->GetScreenRectExternal();
  nscoord startOffsetPix = f == startFrame ? TwipsToPixels(startPoint.x) : 0;
  nscoord endOffsetPix = f == endFrame ? TwipsToPixels(endPoint.x) : r.width;
  r.x += startOffsetPix;
  r.width = endOffsetPix - startOffsetPix;
  sum.UnionRect(sum, r);
}

Comment 33

[Aaron Leventhal]

Attachment: Roc's loop (looks good to me)

Comment 34

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

Comment on attachment 193594 [details] [diff] [review]
Roc's loop (looks good to me)

yeah, that's simpler and more obviously correct :-)

Comment 35

[Aaron Leventhal]

Fixed on trunk since 8/23/2005.

Comment 36

[Tim McHyde]

The behavior has improved but the function is still broken, especially with
words that start a new line...which return a rectangle as wide as the entire
line rather than just the characters making up the first word of that line, in
my tests.  

I've changed the bug demo program at the above URL to help in seeing this bug
and testing that it is fixed comprehensively.  It nwo shows the current word
under the cursor underlined, making it easy to see how this breaks when you move
to the 2nd line, 1st word and 2nd word. 

I think will prove invaluable for testing the patch proposed to fix this bug.

Comment 37

[Aaron Leventhal]

Attachment: 1) Remove GetOffsetToExternal() usage because it is already accounted for, 2) Use inHint to GetChildFrameContainingOffset() to get the start vs. end of a frame for a boundary char

Fixes 2 problems:
1) Bounds for a substring on the second or greater line of a DOM node where the
start of the frame is not the same as the start of the first line's frame
2) Bounds for a substring that starts a line

Comment 38

[Aaron Leventhal]

Fixed on trunk (not on Firefox 1.5 branch yet!) Test builds here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Comment 39

[Tim McHyde]

My tester app with all my previous test pages now work correctly.  Good work,
it's fixed.

Comment 40

[Tim McHyde]

While it's true all my previous cases passed, I found a new "corner case" where
this bug still fails.  Load this html into the latest FF 1.5 nightly and use
your tester to get the accessible object and rectangle as you move your mouse
over each part of the text and you'll see for the right half of the 2nd line it
gives the rectangle for the previou bold part..until you move to the next line
of the non-bold part where it finally figures out the right accessible/rectangle.

<div style="width:600">This part works fine right here <b>and this part works
fine in bold but keep going  to the right where this bold ends and </b>
GetAccessibleFromPoint here gives wrong node for previous bold part. but here
after the line wraps it gets the correct accessible node and rect.</div>

Comment 41

[Aaron Leventhal]

Tim, that's not a problem with get_unclippedSubstringBounds. That's a problem
with our AccessibleObjectFromPoint impl via IAccessible::accHitTest(). Please
file a separate bug and attach a test case.