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)
Core
DOM: Core & HTML
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.
Reporter | ||
Comment 2•17 years ago
|
||
Yeah, the former, that's the whole point.
Reporter | ||
Comment 3•15 years ago
|
||
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
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)
Reporter | ||
Comment 6•15 years ago
|
||
+ *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!
Reporter | ||
Updated•15 years ago
|
Attachment #399620 -
Flags: review?(roc)
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)
Reporter | ||
Comment 8•15 years ago
|
||
+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)
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)
Reporter | ||
Comment 10•15 years ago
|
||
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+
Reporter | ||
Comment 12•15 years ago
|
||
liucougar, now you can just attach a final patch and add checkin-needed. yay!
Assignee | ||
Comment 13•15 years ago
|
||
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
Reporter | ||
Updated•15 years ago
|
Attachment #400614 -
Flags: superreview?(roc) → superreview+
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → liucougar
Keywords: dev-doc-needed
Comment 14•15 years ago
|
||
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•15 years ago
|
||
my name is Heng Liu, thanks for taking care of this
Comment 16•15 years ago
|
||
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•15 years ago
|
||
this patch should apply cleanly to trunk
Attachment #400614 -
Attachment is obsolete: true
Attachment #402130 -
Flags: superreview+
Attachment #402130 -
Flags: review+
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #402130 -
Attachment is obsolete: true
Attachment #402379 -
Flags: superreview+
Attachment #402379 -
Flags: review+
Updated•15 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [c-n: needs updated patch]
Reporter | ||
Updated•15 years ago
|
Whiteboard: [needs landing]
Comment 19•15 years ago
|
||
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]
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a1
Updated•15 years ago
|
Whiteboard: [doc-waiting-1.9.3]
Comment 20•15 years ago
|
||
{ [01:30] <rob_strong_sheriff> sgautherie: please wait until after the tree reopens }
Comment 21•15 years ago
|
||
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]
Comment 22•15 years ago
|
||
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•15 years ago
|
||
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]
Reporter | ||
Comment 24•15 years ago
|
||
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.
Reporter | ||
Comment 25•15 years ago
|
||
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•15 years ago
|
||
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.
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: Bv1] [doc-waiting-1.9.3] → [doc-waiting-1.9.3]
Assignee | ||
Comment 27•15 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
Reporter | ||
Comment 28•15 years ago
|
||
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•15 years ago
|
||
Can we get a (blocking 1.9.3) bug filed on comment 28?
Reporter | ||
Comment 30•15 years ago
|
||
Checked in test fix: http://hg.mozilla.org/mozilla-central/rev/bce848ea984d
Updated•15 years ago
|
Attachment #402771 -
Attachment description: fix test and reenable → fix test and reenable
[Checkin: See comment 30]
Assignee | ||
Comment 31•15 years ago
|
||
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)
Comment 32•15 years ago
|
||
(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•15 years ago
|
||
shall I file a bug on this after this is committed?
Reporter | ||
Comment 34•15 years ago
|
||
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•15 years ago
|
||
separated API move into a new patch
Attachment #405636 -
Attachment is obsolete: true
Attachment #409416 -
Flags: review+
Assignee | ||
Comment 36•15 years ago
|
||
added tests for rtl Could you enligthen me how to achieve different text directions in a single text node (I doubt it's possible).
Reporter | ||
Comment 37•15 years ago
|
||
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•15 years ago
|
||
Attachment #409417 -
Attachment is obsolete: true
Attachment #409474 -
Flags: review?(roc)
Reporter | ||
Comment 39•15 years ago
|
||
Comment on attachment 409474 [details] [diff] [review] new tests with mixed dir text node These are good.
Attachment #409474 -
Flags: review?(roc) → review+
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [doc-waiting-1.9.3] → [doc-waiting-1.9.3][needs landing]
Reporter | ||
Comment 40•15 years ago
|
||
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•15 years ago
|
||
I really don't know much about Hebrew (or any other rtl language), so I will leave that to experts
Reporter | ||
Comment 42•15 years ago
|
||
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 ago → 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [doc-waiting-1.9.3][needs landing] → [doc-waiting-1.9.3]
Comment 43•15 years ago
|
||
The following snippet crashes 3.7 on my Mac OS X (10.6.2): document.createRange().getBoundingClientRect(); but probably shouldn't.
Comment 44•15 years ago
|
||
that's bug 529411
Comment 45•14 years ago
|
||
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]
Updated•11 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•