The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Tim McHyde, Assigned: Aaron Leventhal)

Tracking

({fixed1.8})

Trunk
x86
Windows 2000
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 9 obsolete attachments)

274.35 KB, application/octet-stream
Details
274.56 KB, application/octet-stream
Details
3.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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!
(Reporter)

Comment 1

12 years ago
Created attachment 186236 [details]
small windows demonstration program
(Reporter)

Comment 2

12 years ago
Created attachment 186502 [details]
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.
(Reporter)

Comment 3

12 years ago
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?
Summary: ISimpleDOMText.get_unclippedSubstringBounds returns X too far right → ISimpleDOMText.get_unclippedSubstringBounds returns X too far right
(Reporter)

Updated

12 years ago
Summary: ISimpleDOMText.get_unclippedSubstringBounds returns X too far right → ISimpleDOMText.get_unclippedSubstringBounds always gives same Rect.Top for all lines of text
(Assignee)

Comment 4

12 years ago
Created attachment 190091 [details] [diff] [review]
Add offset specific line within block
Attachment #190091 - Flags: review?(timeless)
(Assignee)

Updated

12 years ago
Attachment #190091 - Flags: superreview?(bzbarsky)
(Reporter)

Comment 5

12 years ago
(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.
(Assignee)

Comment 6

12 years ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
So what coordinate system is the point coming out of this method supposed to be
in?  What's aContainingFrame here?
(Assignee)

Comment 8

12 years ago
(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?

> 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.
(Assignee)

Comment 10

12 years ago
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 ?
> Okay, the accessible coordinate system is the same as the primary frame's.

In that case, just use nsIFrame::GetOffsetTo appropriately?
(Assignee)

Updated

12 years ago
Attachment #190091 - Flags: superreview?(bzbarsky)
Attachment #190091 - Flags: review?(timeless)
(Assignee)

Comment 12

12 years ago
Created attachment 190148 [details] [diff] [review]
Use GetOffsetTo
Attachment #190091 - Attachment is obsolete: true
Attachment #190148 - Flags: superreview?(bzbarsky)
Attachment #190148 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

12 years ago
Attachment #190148 - Flags: review?

Updated

12 years ago
Attachment #190148 - Flags: review? → review+
(Assignee)

Updated

12 years ago
Attachment #190148 - Flags: approval1.8b4?
Attachment #190148 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 13

12 years ago
Checking in nsTextAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsTextAccessibleWrap.cpp,v  <-- 
nsTextAccessibleWrap.cpp
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

12 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

12 years ago
Created attachment 191340 [details] [diff] [review]
Fix aX, aY, aWidth, aHeight calculations

This time I used a better testing methodology.
(Assignee)

Updated

12 years ago
Attachment #191340 - Flags: superreview?(bzbarsky)
Attachment #191340 - Flags: review?(bzbarsky)
So.. how's this different from startRect.UnionRect(endRect)?
(Assignee)

Comment 17

12 years ago
(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.
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?
(Assignee)

Comment 19

12 years ago
Created attachment 191354 [details] [diff] [review]
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
Attachment #191340 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #191354 - Flags: superreview?(bzbarsky)
Attachment #191354 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

12 years ago
Created attachment 191356 [details] [diff] [review]
Slightly smaller patch, perhaps better
Attachment #191356 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #191340 - Flags: superreview?(bzbarsky)
Attachment #191340 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #191356 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

12 years ago
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.
Attachment #191354 - Flags: superreview?(roc)
Attachment #191354 - Flags: superreview?(bzbarsky)
Attachment #191354 - Flags: review?(roc)
Attachment #191354 - Flags: review?(bzbarsky)
(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());
  }
(Assignee)

Comment 23

12 years ago
Created attachment 192218 [details] [diff] [review]
Use roc's suggestion
Attachment #192218 - Flags: superreview?(roc)
Attachment #192218 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Attachment #191356 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #191354 - Attachment is obsolete: true
Attachment #191354 - Flags: superreview?(roc)
Attachment #191354 - Flags: review?(roc)
(Assignee)

Comment 24

12 years ago
Oh, of course I'll remove the commented out code.
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.
Attachment #192218 - Flags: superreview?(roc)
Attachment #192218 - Flags: superreview-
Attachment #192218 - Flags: review?(roc)
Attachment #192218 - Flags: review-
(Assignee)

Comment 26

12 years ago
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?
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.
(Assignee)

Comment 28

12 years ago
Ok, new patch coming.... trying to test it now.
(Assignee)

Comment 29

12 years ago
Created attachment 192985 [details] [diff] [review]
Union screen rects
Attachment #192218 - Attachment is obsolete: true
Attachment #192985 - Flags: superreview?(roc)
Attachment #192985 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Whiteboard: Seeking r+sr= roc
Why aren't you handling start and end offsets in the multi-frame case?
(Assignee)

Updated

12 years ago
Attachment #192985 - Attachment is obsolete: true
Attachment #192985 - Flags: superreview?(roc)
Attachment #192985 - Flags: review?(roc)
(Assignee)

Comment 31

12 years ago
Created attachment 193449 [details] [diff] [review]
Use startPoint and endPoint even when more than one frame involved
Attachment #193449 - Flags: superreview?(roc)
Attachment #193449 - Flags: review?(roc)
+  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);
}
Attachment #193449 - Flags: superreview?(roc)
Attachment #193449 - Flags: superreview-
Attachment #193449 - Flags: review?(roc)
Attachment #193449 - Flags: review-
(Assignee)

Comment 33

12 years ago
Created attachment 193594 [details] [diff] [review]
Roc's loop (looks good to me)
Attachment #193594 - Flags: superreview?(roc)
Attachment #193594 - Flags: review?(roc)
Comment on attachment 193594 [details] [diff] [review]
Roc's loop (looks good to me)

yeah, that's simpler and more obviously correct :-)
Attachment #193594 - Flags: superreview?(roc)
Attachment #193594 - Flags: superreview+
Attachment #193594 - Flags: review?(roc)
Attachment #193594 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #193594 - Flags: approval1.8b4?
(Assignee)

Comment 35

12 years ago
Fixed on trunk since 8/23/2005.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: blocking1.8b4+

Updated

12 years ago
Attachment #193594 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
(Reporter)

Comment 36

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: ISimpleDOMText.get_unclippedSubstringBounds always gives same Rect.Top for all lines of text → ISimpleDOMText.get_unclippedSubstringBounds has trouble with character 1 of wrapped line 2

Updated

12 years ago
Whiteboard: Seeking r+sr= roc
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
Whiteboard: [ETA: need to address more issues with this ]
(Assignee)

Comment 37

12 years ago
Created attachment 197043 [details] [diff] [review]
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
Attachment #190148 - Attachment is obsolete: true
Attachment #193449 - Attachment is obsolete: true
Attachment #193594 - Attachment is obsolete: true
Attachment #197043 - Flags: superreview?(roc)
Attachment #197043 - Flags: review?(roc)

Updated

12 years ago
Whiteboard: [ETA: need to address more issues with this ] → [ETA: need to address more issues with this ][needs review+SR roc]
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: need to address more issues with this ][needs review+SR roc] → [ETA: needs review+SR roc]
Attachment #197043 - Flags: superreview?(roc)
Attachment #197043 - Flags: superreview+
Attachment #197043 - Flags: review?(roc)
Attachment #197043 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #197043 - Flags: approval1.8b5?
(Assignee)

Comment 38

12 years ago
Fixed on trunk (not on Firefox 1.5 branch yet!) Test builds here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #197043 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
(Reporter)

Comment 39

12 years ago
My tester app with all my previous test pages now work correctly.  Good work,
it's fixed.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 40

12 years ago
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>
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

12 years ago
Whiteboard: [ETA: needs review+SR roc]
(Assignee)

Comment 41

12 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.