Closed Bug 396392 Opened 17 years ago Closed 15 years ago

Support for getClientRects and getBoundingClientRect in DOM Range

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: roc, Assigned: liucougar)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 7 obsolete files)

19.46 KB, patch
liucougar
: review+
liucougar
: superreview+
Details | Diff | Splinter Review
838 bytes, patch
Details | Diff | Splinter Review
6.91 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
liucougar
: review+
Details | Diff | Splinter Review
9.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
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?
"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.
Yeah, the former, that's the whole point.
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.
Blocks: 503399
Blocks: 220262
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
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
Attachment #399620 - Flags: review?(roc)
+    *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!
Attached patch revised patch with test case (obsolete) — Splinter Review
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
Attachment #400168 - Flags: review?(roc)
+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.
Attachment #400168 - Flags: review?(roc)
Attached patch New patch and test case (obsolete) — Splinter Review
modified according to the latest comment

added a unit test case
Attachment #399620 - Attachment is obsolete: true
Attachment #400168 - Attachment is obsolete: true
Attachment #400422 - Flags: review?(roc)
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
Attachment #400422 - Flags: superreview?(jst)
Attachment #400422 - Flags: review?(roc)
Attachment #400422 - Flags: review+
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.
Attachment #400422 - Flags: superreview?(jst) → superreview+
liucougar, now you can just attach a final patch and add checkin-needed. yay!
Attached patch final patch with test case (obsolete) — Splinter Review
style update according to review and superview comments
Attachment #400422 - Attachment is obsolete: true
Attachment #400614 - Flags: superreview?(roc)
Attachment #400614 - Flags: review+
Keywords: checkin-needed
Attachment #400614 - Flags: superreview?(roc) → superreview+
Assignee: nobody → liucougar
Keywords: dev-doc-needed
Patch doesn't apply cleanly to current m-c.

May I suggest you to fill in your name(s) in Bugzilla ?
Whiteboard: [c-n: needs updated patch]
my name is Heng Liu, thanks for taking care of this
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.
Attached patch updated patch against trunk (obsolete) — Splinter Review
this patch should apply cleanly to trunk
Attachment #400614 - Attachment is obsolete: true
Attachment #402130 - Flags: superreview+
Attachment #402130 - Flags: review+
Attachment #402130 - Attachment is obsolete: true
Attachment #402379 - Flags: superreview+
Attachment #402379 - Flags: review+
Status: NEW → ASSIGNED
Whiteboard: [c-n: needs updated patch]
Whiteboard: [needs landing]
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
Attachment #402379 - Attachment description: removed unrelated changes from the last patch → removed unrelated changes from the last patch [Checkin: Comment 19]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a1
Whiteboard: [doc-waiting-1.9.3]
{
[01:30]	<rob_strong_sheriff>	sgautherie: please wait until after the tree reopens
}
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
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Whiteboard: [doc-waiting-1.9.3] → [c-n: Bv1] [doc-waiting-1.9.3]
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 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 ;-<
Attachment #402708 - Attachment description: (Bv1) Disable test (which fails on MacOSX) → (Bv1) Disable test (which fails on MacOSX) [Checkin: See comment 22+23]
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.
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 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.
Keywords: checkin-needed
Whiteboard: [c-n: Bv1] [doc-waiting-1.9.3] → [doc-waiting-1.9.3]
(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
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.
Can we get a (blocking 1.9.3) bug filed on comment 28?
Attachment #402771 - Attachment description: fix test and reenable → fix test and reenable [Checkin: See comment 30]
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.
Attachment #405636 - Flags: review?(roc)
(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?
shall I file a bug on this after this is committed?
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?
Attachment #405636 - Flags: review?(roc) → review+
Attached patch api moveSplinter Review
separated API move into a new patch
Attachment #405636 - Attachment is obsolete: true
Attachment #409416 - Flags: review+
Attached patch more unit tests (obsolete) — Splinter Review
added tests for rtl

Could you enligthen me how to achieve different text directions in a single text node (I doubt it's possible).
You get different directions in a single text node by mixing LTR characters (e.g., English) with RTL characters (e.g., Hebrew).
Attachment #409417 - Attachment is obsolete: true
Attachment #409474 - Flags: review?(roc)
Comment on attachment 409474 [details] [diff] [review]
new tests with mixed dir text node

These are good.
Attachment #409474 - Flags: review?(roc) → review+
Keywords: checkin-needed
Whiteboard: [doc-waiting-1.9.3] → [doc-waiting-1.9.3][needs landing]
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.
I really don't know much about Hebrew (or any other rtl language), so I will leave that to experts
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
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [doc-waiting-1.9.3][needs landing] → [doc-waiting-1.9.3]
Depends on: 529411
Depends on: 529670
The following snippet crashes 3.7 on my Mac OS X (10.6.2):

  document.createRange().getBoundingClientRect();

but probably shouldn't.
that's bug 529411
Component: DOM: Traversal-Range → DOM: Core & HTML
Depends on: 1439891
You need to log in before you can comment on or make changes to this bug.