Summary: | Implement CSSOM DocumentView.caretRangeFromPoint | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | annevk, roc, sam, simon.fraser, xji | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2009-07-07 15:49:01 PDT
http://dev.w3.org/csswg/cssom-view/#documentview-caretrangefrompoint is the spec URL btw. Created attachment 32610 [details]
WIP patch (not ready for review)
Created attachment 32611 [details]
helpful test
Attaching a little helpful test I made when writing this code. With the patch applied, when you click, the position where you click extended one character forward is selected as an indication.
Created attachment 32694 [details]
Little more work.
Created attachment 32952 [details]
patch w/ layout test
Hi Sam,
I did not add myself into the cc list, and I am not aware that you are doing the implementation.
I am uploading my patch just for reference.
Thanks,
Xiaomei
(In reply to comment #4) > Created an attachment (id=32694) [details] > Little more work. Hi Sam, Thanks for implementing this! I have several questions regarding your patch: 1. The CSSOM spec says that: "If ..... no insertion point indicator would have been inserted, or editing is not supported, the method must return null". I do not quite understand why it returns NULL if "editing is not supported". So, I changed the implementation to "return NULL if the node type is not text related node type". For other node types, range::create(Document*, Position, Position) might raise error code and hit assertion failure (for example, "input" element). Or rangeCompliantEquivalent takes care of that? 2. When calculating the coordinate relative to document, you are taking zoom factor into consideration. Should it also take document scrolling into consideration as well? Since the spec says (x, y) is coordinates in viewport. Or maybe we can use coordinate relative to document and let JS do the computation. 3. Could you please add some comments in caretRangeFromPoint() declaration in Document.h? For a new comer like me, comments help him/her to learn the code, especially what/relative to which a point is. Thanks, Xiaomei (In reply to comment #6) > (In reply to comment #4) > > Created an attachment (id=32694) [details] [details] > > Little more work. > > Hi Sam, > > Thanks for implementing this! > > I have several questions regarding your patch: > 1. The CSSOM spec says that: "If ..... no insertion point indicator would have > been inserted, or editing is not supported, the method must return null". I do > not quite understand why it returns NULL if "editing is not supported". So, I > changed the implementation to "return NULL if the node type is not text related > node type". For other node types, range::create(Document*, Position, Position) > might raise error code and hit assertion failure (for example, "input" > element). Or rangeCompliantEquivalent takes care of that? I don't think limiting to just text related nodes is the right way to go since that removes are replaced elements as well. I used a combination of checking for shadow tree content (input elements, etc.) and using rangeComplientEquivalent. The spec is not all that clear, so we should probably confer with the editor on the intent. > > 2. When calculating the coordinate relative to document, you are taking zoom > factor into consideration. Should it also take document scrolling into > consideration as well? Since the spec says (x, y) is coordinates in viewport. > Or maybe we can use coordinate relative to document and let JS do the > computation. All computation should be done relative to the viewport, so I think scrolling is already accounted for, though adding tests is the way to be sure. > > 2. When calculating the coordinate relative to document, you are taking zoom
> > factor into consideration. Should it also take document scrolling into
> > consideration as well? Since the spec says (x, y) is coordinates in viewport.
> > Or maybe we can use coordinate relative to document and let JS do the
> > computation.
>
> All computation should be done relative to the viewport, so I think scrolling
> is already accounted for, though adding tests is the way to be sure.
If HitTest is done relative to viewport when the document is scrolled, the computed range offset is shifted too (relative to viewport, not relative to the element within the document).
I added the adjustment when I ran test. But I do not know whether zoom factor or other part in your patch takes care of that. And you are right, adding test is the way to be sure.
Thanks!
Xiaomei
"editing is not supported" was there because the use case for this method was editors. However, it seems better to remove that since finding a good definition for it seems hard and applications want to check something like that a better higher level as well. The intent of the specification is that if you have designMode set to "on" you'd get a range for where the cursor appears when you click or null if no cursor appears. Is that clear enough? I agree, "editing is not supported" should be removed. Landed in r48188. Created attachment 39487 [details]
caretRangeFromPointWithoutScroll.html
Created attachment 39488 [details]
caretRangeFromPointWithScroll.html
Hi Sam, Thanks for implementing this! It worked pretty well except the 2 issues I list below: 1. I think the offset in the returned range should be the character/caret offset relative to the whole element, not relative to the viewpoint. Please see the attached file caretRangeFromPointWithoutScroll.html for detailed explanation. 2. If I pass in event.clientX + document.body.scrollLeft and event.clientY + document.body.scrollTop as the x-axis and y-axis to caretRangeFromPoint(), there is a possibility that the range is null which should not. Please see attached file caretRangeFromPointWithScroll.html for detailed explanation. Please let me know if I mis-understood anything, or the code needs a bit tweak. Thanks, Xiaomei Created attachment 39493 [details]
caretRangeFromPointInNonTextNode.html
Another question: In caretRangeFromPoint(), there is the following code: Node* node = result.innerNode(); if (!node) return 0; Since we do not check whether the 'node' is a Text related node, there is a possibility that a range is created even when the mouse is not pointed in the text area. Please open caretRangeFromPointInNonTextNode.html, click mouse right before the first word "type", in the range created by caretRangeFromPoint(), the node is the Text node, the start/end offset is 0. click mouse several characters ahead of word "type" (the empty space in the left of "type"), although the 'node' is a HTMLParagraphElement, but it has the same visible position as the above, and the range created is the same as above. Is this the correct behavior? I think my assumption is that this function converts mouse position to the character position within an element, so a null range should be returned if click the empty spaces ahead of word "type". Thanks, Xiaomei This bug is now closed. Please file any bugs in the implementation in a separate bug. |