Support for getClientRects and getBoundingClientRect in DOM Range

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: roc, Assigned: liucougar)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 7 obsolete attachments)

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

Updated

8 years ago
Blocks: 220262
(Assignee)

Comment 4

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

Comment 5

8 years ago
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
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!
Attachment #399620 - Flags: review?(roc)
(Assignee)

Comment 7

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

Updated

8 years ago
Attachment #400168 - Flags: review?(roc)
(Assignee)

Comment 9

8 years ago
Created attachment 400422 [details] [diff] [review]
New patch and test case

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

Comment 13

8 years ago
Created attachment 400614 [details] [diff] [review]
final patch with test case

style update according to review and superview comments
Attachment #400422 - Attachment is obsolete: true
Attachment #400614 - Flags: superreview?(roc)
Attachment #400614 - Flags: review+
(Assignee)

Updated

8 years ago
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]
(Assignee)

Comment 15

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

Comment 17

8 years ago
Created attachment 402130 [details] [diff] [review]
updated patch against trunk

this patch should apply cleanly to trunk
Attachment #400614 - Attachment is obsolete: true
Attachment #402130 - Flags: superreview+
Attachment #402130 - Flags: review+
(Assignee)

Comment 18

8 years ago
Created attachment 402379 [details] [diff] [review]
removed unrelated changes from the last patch
[Checkin: Comment 19]
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
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a1
Whiteboard: [doc-waiting-1.9.3]
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
}
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.
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 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]
(Assignee)

Comment 27

8 years ago
(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?
Checked in test fix:
http://hg.mozilla.org/mozilla-central/rev/bce848ea984d
Attachment #402771 - Attachment description: fix test and reenable → fix test and reenable [Checkin: See comment 30]
(Assignee)

Comment 31

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

Comment 33

8 years ago
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+
(Assignee)

Comment 35

8 years ago
Created attachment 409416 [details] [diff] [review]
api move

separated API move into a new patch
Attachment #405636 - Attachment is obsolete: true
Attachment #409416 - Flags: review+
(Assignee)

Comment 36

8 years ago
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).
You get different directions in a single text node by mixing LTR characters (e.g., English) with RTL characters (e.g., Hebrew).
(Assignee)

Comment 38

8 years ago
Created attachment 409474 [details] [diff] [review]
new tests with mixed dir text node
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.
(Assignee)

Comment 41

8 years ago
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
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [doc-waiting-1.9.3][needs landing] → [doc-waiting-1.9.3]

Updated

8 years ago
Depends on: 529411

Updated

8 years ago
Depends on: 529670
The following snippet crashes 3.7 on my Mac OS X (10.6.2):

  document.createRange().getBoundingClientRect();

but probably shouldn't.

Comment 44

8 years ago
that's bug 529411
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.
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [doc-waiting-1.9.3]
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.