Last Comment Bug 396392 - Support for getClientRects and getBoundingClientRect in DOM Range
: Support for getClientRects and getBoundingClientRect in DOM Range
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.9.3a1
Assigned To: liucougar
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 529411 529670
Blocks: 220262 503399
  Show dependency treegraph
 
Reported: 2007-09-16 22:43 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2013-04-04 13:53 PDT (History)
18 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to add getClientRects and getBoundingClientRect to Range (12.92 KB, patch)
2009-09-09 16:22 PDT, liucougar
no flags Details | Diff | Splinter Review
revised patch with test case (18.05 KB, patch)
2009-09-11 16:10 PDT, liucougar
no flags Details | Diff | Splinter Review
New patch and test case (20.00 KB, patch)
2009-09-13 20:05 PDT, liucougar
roc: review+
jonas: superreview+
Details | Diff | Splinter Review
final patch with test case (19.95 KB, patch)
2009-09-14 16:18 PDT, liucougar
liucougar: review+
roc: superreview+
Details | Diff | Splinter Review
updated patch against trunk (20.62 KB, patch)
2009-09-22 11:50 PDT, liucougar
liucougar: review+
liucougar: superreview+
Details | Diff | Splinter Review
removed unrelated changes from the last patch [Checkin: Comment 19] (19.46 KB, patch)
2009-09-23 10:05 PDT, liucougar
liucougar: review+
liucougar: superreview+
Details | Diff | Splinter Review
(Bv1) Disable test (which fails on MacOSX) [Checkin: See comment 22+23] (838 bytes, patch)
2009-09-24 16:32 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
fix test and reenable [Checkin: See comment 30] (6.91 KB, patch)
2009-09-24 22:23 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
move these 2 APIs to nsIDOMNSRange with more test (9.66 KB, patch)
2009-10-09 23:45 PDT, liucougar
roc: review+
Details | Diff | Splinter Review
api move (1.60 KB, patch)
2009-10-30 14:40 PDT, liucougar
liucougar: review+
Details | Diff | Splinter Review
more unit tests (8.17 KB, patch)
2009-10-30 14:42 PDT, liucougar
no flags Details | Diff | Splinter Review
new tests with mixed dir text node (9.54 KB, patch)
2009-10-30 21:03 PDT, liucougar
roc: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2007-09-16 22:43:57 PDT
We need a good way to get geometry for substrings of a DOM text node. Rather than introducing an IE-style TextRange object, we can just add methods to Range. So I propose to add getClientRects and getBoundingClientRect to Range.

I think Range.getClientRects should basically concatenate the results of Element.getClientRects for each element in the range. More precisely, in the language of the DOM Range spec:
-- For each element selected by the range, whose parent is not selected by the range, we include the results of getClientRects() on the element.
-- For each text node selected or partially selected by the range, whose parent is not selected by the range, we include one rectangle for each CSS box associated with the text in the range, using the same units and coordinate system as for Element.getClientRects. The bounds are computed using font metrics, not the inked glyph extents; thus, for horizontal writing, the vertical dimension of each box is determined by the font ascent and descent, and the horizontal dimension by the text advance width --- i.e., left and right bearing are not included.
-- The rectangles are returned in content order.

Range.getBoundingClientRect is of course the union of the non-empty rectangles in Range.getClientRects. When there are no non-empty rectangles, we follow Element.getBoundingClientRect: if getClientRects is an empty list, return (0,0,0,0), otherwise return an empty rectangle whose top-left is the top-left of the first rectangle in getClientRects.

Anne, Jonas, Fantasai, what do you think?
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-16 23:36:32 PDT
"For each text node selected or partially selected by the range, whose parent
is not selected by the range, we include one rectangle for each CSS box
associated with the text in the range"

Does that mean a CSS box spanning just the characters in the range? Or the whole line-box? I think it should be the former to make it possible to determine the coordinates for a single character or word.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-09-17 01:23:02 PDT
Yeah, the former, that's the whole point.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-02 20:01:37 PDT
Anne added this:
http://www.w3.org/TR/2009/WD-cssom-view-20090804/#extensions-to-the-range-interface
I'd really like to get this implemented... It would help with testing caret positions.
Comment 4 liucougar 2009-09-04 14:53:50 PDT
nsIContentIterator can be used to iterate through the nodes selected by a range

to deal with text node: each text node may contain several nsTextFrame, may need to call nsTextFrame::GetPointFromOffset to get the position
Comment 5 liucougar 2009-09-09 16:22:42 PDT
Created attachment 399620 [details] [diff] [review]
patch to add getClientRects and getBoundingClientRect to Range

this is a RFC patch, has no unit tests written

function GetContainingBlockForClientRect and two structs (RectListBuilder and RectAccumulator) is moved to nsLayoutUtils.cpp and exposed in nsLayoutUtils.h, so they can be used in nsRange.cpp

this patch implements support for both collapsed and non-collapsed ranges.

also fixed a make file typo
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-09 17:53:09 PDT
+    *aResult = rectList.forget().get();

You can now use rectList.forget(aResult);

+static nsresult CollectClientRects(nsLayoutUtils::RectCallback * collector, 
+                nsRange * range, nsINode * startParent, PRInt32 startOffset, 
+                nsINode * endParent, PRInt32 endOffset)

No space between type and *, also, align the params with the first param

+    if (content->IsNodeOfType(nsINode::eTEXT)){

Space before {

CollectClientRects should return void.

We need to call FlushPendingNotifications(Flush_Layout) before we start getting frames.

+           nsPoint point;
+	   GetRelativePointFromOffset(outFrame, 
+	     nsLayoutUtils::GetContainingBlockForClientRect(outFrame), startOffset, &point);
+           nsRect r(outFrame->GetOffsetTo(nsLayoutUtils::GetContainingBlockForClientRect(outFrame)), outFrame->GetSize());
+           r.width = 0;
+           r.x = point.x;
+           collector->AddRect(r);

Fix indent.

Just call nsLayoutUtils::GetContainingBlockForClientRect(outFrame) once

+  iter.First();

Shouldn't this come before the first test of iter.IsDone()? Would be slightly better if we had
  iter.First();
  if (iter.IsDone()) {
    ...
    return;
  }

  do {
    ...
  } while (!iter.IsDone());

+       }else if (node == endContainer){

Space after } and before {

Instead of setting 'handled' to true, just use the "continue" statement.

+        nsCOMPtr<nsGenericElement> geelm(do_QueryInterface(node));

nsGenericElement is not an interface so you shouldn't do this. QI to nsIContent and use presShell->GetPrimaryFrameFor() on that.

+            nsLayoutUtils::GetContainingBlockForClientRect(frame)

Hmm. If the range happens to span a foreignObject boundary or a CSS-Transforms element boundary, we're going to get meaningless results, but I guess there isn't much we can do about that. Let's leave this as-is.

GetRelativePointFromOffset can just call nsIFrame::GetOffsetTo instead of using that loop, can't it?

All your parameter names should start with 'a'.

I'm not sure how partially selected SVG text should be handled by this API. In your patch you're ignoring it, which I think is fine.

+    PRBool rtl = f->GetTextRun()->IsRightToLeft();

rtl-ness can vary for each frame in the continuation chain, so setting rtl outside the loop here isn't right, you should set it inside the loop.

+      if (!(fend <= startOffset || fstart >= endOffset))

I would find this easier to read if you wrote the loop as a for loop:
  for (nsIFrame* f = ...; f; f = f->GetNextContinuation())
and then if (fend <= startOffset || fStart >= endOffset) continue;

... except, what if the start of the (non-collapsed) range is the end of a text node or vice versa? We probably should return a zero-length rectangle in that case, but does the DOM Range spec actually allow that?

+        {
+  	  //startOffset is within this frame
+          nsPoint point;
+	  GetRelativePointFromOffset(f, relativeTo, startOffset, &point);
+          SplitRect(&r, point.x, rtl);
+        }

Wonky indentation

+      }else if(fend >= endOffset){
+        //gone past the endOffset, just stop the loop
+        break;

This optimization is probably not worth having

I would call SplitRect "ExtractRect", and it needs a documentation comment.

+  if (r->x < x && (r->width + r->x > x))
+  {
+    if (keepLeft)
+      r->width -= x - r->x;
+    else
+    {
+      r->width = x - r->x;
+      r->x = x;
+    }
+  }

I think your !keepLeft case is wrong. You can probably also use an NS_ASSERTION instead of an if, i.e., NS_ASSERTION(r->x <= x && x <= r->XMost(), ...). Then simplify:
  if (keepLeft) {
    r->width = x - r->x;
  } else {
    r->width = r->XMost() - x;
    r->x = x;
  }

These are all details ... your overall code structure seems just right!
Comment 7 liucougar 2009-09-11 16:10:16 PDT
Created attachment 400168 [details] [diff] [review]
revised patch with test case

this patch corrected all the issues in the last comment

iter->First() is removed, because iter->init() calls First() automatically

let me know if I missed some test cases
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-12 04:47:32 PDT
+static void ExtractRect(nsRect * r, PRInt32 x, PRBool keepLeft)

No space between nsRect and *. Param names should start with 'a'

+  if (keepLeft)
+    r->width = x - r->x;
+  else {

{} around if bodies (except "return"/"break"/"continue" statements)

+                const nsIFrame* relativeTo, const PRInt32 aOffset, nsPoint* aOutPoint)

Fix indent (and break line), "relativeTo" should be "aRelativeTo"

Should make GetRelativePointFromOffset just return the point as a direct result. Outparams suck. Or maybe merge ExtractRect with GetRelativePointFromOffset into a single helper function. Yeah!

+static nsresult GetPartialTextRect(nsLayoutUtils::RectCallback* aCallback, nsIPresShell* aPresShell, 
+                                   nsIContent* aContent, PRInt32 aStartOffset, PRInt32 aEndOffset)
+{
+  nsIFrame* frame = aPresShell->GetPrimaryFrameFor(aContent);
+  if (frame && frame->GetType() == nsGkAtoms::textFrame) {
+    nsTextFrame* textFrame = static_cast<nsTextFrame*>(frame);
+    nsIFrame* relativeTo = nsLayoutUtils::GetContainingBlockForClientRect(textFrame);
+    for (nsTextFrame* f = textFrame; f; f = static_cast<nsTextFrame*>(f->GetNextContinuation())) {
+      PRInt32 fstart = f->GetContentOffset(), fend=f->GetContentEnd();

Break lines at 80 columns

+      PRInt32 fstart = f->GetContentOffset(), fend=f->GetContentEnd();

Be consistent about the spaces around = ... best to add spaces around =

+static nsIPresShell * GetOwnerDocPresShell(nsINode * aNode, PRBool aFlush = PR_FALSE)

Don't use default parameters here, they make code a bit harder to read usually. And don't put space before *

+  if (aFlush)
+    document->FlushPendingNotifications(Flush_Layout);

{} around statement

+        nsIFrame * outFrame;

Remove space before *

+	   nsIFrame* relativeTo = nsLayoutUtils::GetContainingBlockForClientRect(outFrame);
+	   GetRelativePointFromOffset(outFrame, relativeTo, aStartOffset, &point);

These two lines have tabs. Only use spaces. But the lines around them are misindented too.

+         PRInt32 offset = startContainer == endContainer ? aEndOffset : content->GetText()->GetLength();

Looks more than 80 columns

Need to rev UUID in nsIDOMRange.idl

+  is(rectlist.length, 0, name + 'empty rectlist should have zero rect');

"zero rects"

Your test code needs spaces in "if{", "}else{', "try{", etc... And after commas in object initializers too I think.

Right now your tests check for hardcoded numeric results. That's not going to work in general because font metrics vary across platforms and we don't want to depend on them.

I think the way you're going to have to test this is to build up each expected test result by calling element.getBoundingClientRect and element.getClientRects on the various elements that should be selected by the range. To test offsets within a text node, I think you can calculate the expected results by measuring span widths in something like <span>000</span>000.
Comment 9 liucougar 2009-09-13 20:05:16 PDT
Created attachment 400422 [details] [diff] [review]
New patch and test case

modified according to the latest comment

added a unit test case
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-13 20:12:48 PDT
Comment on attachment 400422 [details] [diff] [review]
New patch and test case

+static void ExtractRectFromOffset(nsIFrame* aFrame,
+                                          const nsIFrame* aRelativeTo, 
+                                          const PRInt32 aOffset, nsRect* aR, PRBool aKeepLeft)

Indenting's messed up

+           ExtractRectFromOffset(outFrame, relativeTo, aStartOffset,&r, PR_FALSE);

space before &r

Need a DOM peer review here I think
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-09-14 16:08:12 PDT
Comment on attachment 400422 [details] [diff] [review]
New patch and test case

GetOwnerDocPresShell isn't a very good name given that you're using the CurrentDoc to get the PresShell. I'd say simply call the function GetPresShell.

sr=me with that.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-14 16:10:32 PDT
liucougar, now you can just attach a final patch and add checkin-needed. yay!
Comment 13 liucougar 2009-09-14 16:18:19 PDT
Created attachment 400614 [details] [diff] [review]
final patch with test case

style update according to review and superview comments
Comment 14 Serge Gautherie (:sgautherie) 2009-09-21 09:15:30 PDT
Patch doesn't apply cleanly to current m-c.

May I suggest you to fill in your name(s) in Bugzilla ?
Comment 15 liucougar 2009-09-21 10:17:14 PDT
my name is Heng Liu, thanks for taking care of this
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-09-22 06:53:17 PDT
Heng, I think Serge would be thankful if you would update your patch so it would apply cleanly to current trunk build. It's much easier for him to check in that way.
Comment 17 liucougar 2009-09-22 11:50:22 PDT
Created attachment 402130 [details] [diff] [review]
updated patch against trunk

this patch should apply cleanly to trunk
Comment 18 liucougar 2009-09-23 10:05:31 PDT
Created attachment 402379 [details] [diff] [review]
removed unrelated changes from the last patch
[Checkin: Comment 19]
Comment 19 Serge Gautherie (:sgautherie) 2009-09-24 14:04:52 PDT
Comment on attachment 402379 [details] [diff] [review]
removed unrelated changes from the last patch
[Checkin: Comment 19]


http://hg.mozilla.org/mozilla-central/rev/3262c0bd49ac
Comment 20 Serge Gautherie (:sgautherie) 2009-09-24 16:32:52 PDT
Created attachment 402708 [details] [diff] [review]
(Bv1) Disable test (which fails on MacOSX)
[Checkin: See comment 22+23]

{
[01:30]	<rob_strong_sheriff>	sgautherie: please wait until after the tree reopens
}
Comment 21 Serge Gautherie (:sgautherie) 2009-09-24 16:33:59 PDT
The test passes on Linux and Windows but fails on MacOSX:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1253831320.1253833919.12635.gz
OS X 10.5.2 mozilla-central test mochitests on 2009/09/24 15:28:40
Comment 22 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-09-24 19:04:51 PDT
Disabled the test in http://hg.mozilla.org/mozilla-central/rev/7d45032ce70f. It was failing on Windows too, so the message/comment was adjusted.
Comment 23 Serge Gautherie (:sgautherie) 2009-09-24 19:28:27 PDT
Comment on attachment 402708 [details] [diff] [review]
(Bv1) Disable test (which fails on MacOSX)
[Checkin: See comment 22+23]


http://hg.mozilla.org/mozilla-central/rev/d39a7c261f9d
(Cv1) Actually disable test ;-<
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 22:17:34 PDT
One problem in these tests is that we're expecting the vertical dimensions of
the range in a text node to match the vertical dimensions of the <p> containing
the text. That's not necessarily true because of line-height and leading.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 22:23:04 PDT
Created attachment 402771 [details] [diff] [review]
fix test and reenable
[Checkin: See comment 30]

That was basically the whole problem. These tests pass for me, we should be good.

liucougar, we actually need some more tests involving RTL and <span>s and text nodes that break across lines, too, I just noticed...
Comment 26 Serge Gautherie (:sgautherie) 2009-09-25 04:37:23 PDT
Comment on attachment 402771 [details] [diff] [review]
fix test and reenable
[Checkin: See comment 30]

>diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -307,16 +307,17 @@ _TEST_FILES = 	test_bug5141.html \
> 		test_bug514487.html \
>+		test_range_bounds.html \
> 		$(NULL)
> 
> # Disabled; see bug 396392: fails on MacOSX, Windows.
> #		test_range_bounds.html \

Need to remove this comment too.
Comment 27 liucougar 2009-09-25 10:22:23 PDT
(In reply to comment #25)
> Created an attachment (id=402771) [details]
> liucougar, we actually need some more tests involving RTL and <span>s and text
> nodes that break across lines, too, I just noticed...

thanks for the test fix. I will try to add more tests after your patch lands in trunk
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-27 16:41:18 PDT
Urgh. One thing I just noticed is that we added getClientRects/getBoundingClientRect to a frozen interface :-(. They should have been added to nsIDOMNSRange. liucougar, can you attach a patch to fix that? The IID of nsIDOMRange should revert to what it was before.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2009-09-30 09:50:29 PDT
Can we get a (blocking 1.9.3) bug filed on comment 28?
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-01 23:45:42 PDT
Checked in test fix:
http://hg.mozilla.org/mozilla-central/rev/bce848ea984d
Comment 31 liucougar 2009-10-09 23:45:56 PDT
Created attachment 405636 [details] [diff] [review]
move these 2 APIs to nsIDOMNSRange with more test

fixed comment #28

added more tests: rtl and text spaning multiple rows.

there is a problem: the last two tests does not pass in rtl, so they are disabled for rtl.
Comment 32 Simon Montagu :smontagu 2009-10-10 23:35:38 PDT
(In reply to comment #31)
> the last two tests does not pass in rtl, so they are
> disabled for rtl.

Can you file a bug on this (and cc me) please?
Comment 33 liucougar 2009-10-10 23:44:59 PDT
shall I file a bug on this after this is committed?
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-11 17:52:00 PDT
Comment on attachment 405636 [details] [diff] [review]
move these 2 APIs to nsIDOMNSRange with more test

+  while(1){

It would be a bit nicer to move this code to its own helper function and call that function twice.

Let's separate out the test changes from the interface move, please. r+ on the interface move.

I'd like to see another version of the tests. Also, do these tests test text with different directions in the same text node?
Comment 35 liucougar 2009-10-30 14:40:31 PDT
Created attachment 409416 [details] [diff] [review]
api move

separated API move into a new patch
Comment 36 liucougar 2009-10-30 14:42:39 PDT
Created attachment 409417 [details] [diff] [review]
more unit tests

added tests for rtl

Could you enligthen me how to achieve different text directions in a single text node (I doubt it's possible).
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-30 14:52:38 PDT
You get different directions in a single text node by mixing LTR characters (e.g., English) with RTL characters (e.g., Hebrew).
Comment 38 liucougar 2009-10-30 21:03:16 PDT
Created attachment 409474 [details] [diff] [review]
new tests with mixed dir text node
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-01 15:27:41 PST
Comment on attachment 409474 [details] [diff] [review]
new tests with mixed dir text node

These are good.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-01 15:30:50 PST
If you feel like it, you could write even more tests to test partial selection of a mixed-direction text node. That behaves weirdly since you can get rectangles that aren't actually adjacent to each other, e.g. if you select the English text before a Hebrew word and the first half of the Hebrew word.
Comment 41 liucougar 2009-11-01 16:06:05 PST
I really don't know much about Hebrew (or any other rtl language), so I will leave that to experts
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 15:07:13 PST
I landed the interface fixup patch:
http://hg.mozilla.org/mozilla-central/rev/0547086328e8

I also landed the RTL tests:
http://hg.mozilla.org/mozilla-central/rev/9eda936adc30
but backed them out because they failed on Mac:
http://hg.mozilla.org/mozilla-central/rev/52a26cd4e2bb
Comment 43 Juriy "kangax" Zaytsev 2009-11-22 21:12:03 PST
The following snippet crashes 3.7 on my Mac OS X (10.6.2):

  document.createRange().getBoundingClientRect();

but probably shouldn't.
Comment 44 timeless 2009-11-22 22:12:28 PST
that's bug 529411
Comment 45 Eric Shepherd [:sheppy] 2010-11-03 11:48:44 PDT
Documented:

https://developer.mozilla.org/en/DOM/range.getClientRects
https://developer.mozilla.org/en/DOM/range.getBoundingClientRect

And noted on:

https://developer.mozilla.org/en/DOM/range

And of course on Firefox 4 for developers.

Note You need to log in before you can comment on or make changes to this bug.