Bug 27046 - Implement CSSOM DocumentView.caretRangeFromPoint
Summary: Implement CSSOM DocumentView.caretRangeFromPoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-07 15:49 PDT by Eric Seidel (no email)
Modified: 2009-09-11 17:44 PDT (History)
5 users (show)

See Also:


Attachments
WIP patch (not ready for review) (3.02 KB, patch)
2009-07-10 22:43 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
helpful test (862 bytes, text/html)
2009-07-10 22:46 PDT, Sam Weinig
no flags Details
Little more work. (14.60 KB, patch)
2009-07-13 21:07 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
patch w/ layout test (7.34 KB, patch)
2009-07-17 10:23 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
caretRangeFromPointWithoutScroll.html (2.01 KB, text/html)
2009-09-11 16:21 PDT, Xiaomei Ji
no flags Details
caretRangeFromPointWithScroll.html (1.57 KB, text/html)
2009-09-11 16:23 PDT, Xiaomei Ji
no flags Details
caretRangeFromPointInNonTextNode.html (1.13 KB, text/html)
2009-09-11 17:05 PDT, Xiaomei Ji
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-07-07 15:49:01 PDT
Implement CSSOM DocumentView.caretRangeFromPoint

Right now it's not possible to find out what word is under the mouse.  Adding this new feature from CSSOM would solve this (and other related) issue(s).

interface DocumentView {
  readonly attribute AbstractView defaultView;
  Element elementFromPoint(in float x, in float y);
  Range caretRangeFromPoint(in float x, in float y);
};
Comment 1 Eric Seidel (no email) 2009-07-07 15:49:57 PDT
http://dev.w3.org/csswg/cssom-view/#documentview-caretrangefrompoint is the spec URL btw.
Comment 2 Sam Weinig 2009-07-10 22:43:30 PDT
Created attachment 32610 [details]
WIP patch (not ready for review)
Comment 3 Sam Weinig 2009-07-10 22:46:39 PDT
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.
Comment 4 Sam Weinig 2009-07-13 21:07:22 PDT
Created attachment 32694 [details]
Little more work.
Comment 5 Xiaomei Ji 2009-07-17 10:23:05 PDT
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
Comment 6 Xiaomei Ji 2009-07-17 10:59:47 PDT
(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
Comment 7 Sam Weinig 2009-07-17 12:36:50 PDT
(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.
Comment 8 Xiaomei Ji 2009-07-17 12:50:36 PDT
> > 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
Comment 9 Anne van Kesteren 2009-07-27 23:55:51 PDT
"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?
Comment 10 Robert O'Callahan 2009-07-28 00:32:07 PDT
I agree, "editing is not supported" should be removed.
Comment 11 Sam Weinig 2009-09-08 15:03:18 PDT
Landed in r48188.
Comment 12 Xiaomei Ji 2009-09-11 16:21:43 PDT
Created attachment 39487 [details]
caretRangeFromPointWithoutScroll.html
Comment 13 Xiaomei Ji 2009-09-11 16:23:28 PDT
Created attachment 39488 [details]
caretRangeFromPointWithScroll.html
Comment 14 Xiaomei Ji 2009-09-11 16:24:34 PDT
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
Comment 15 Xiaomei Ji 2009-09-11 17:05:37 PDT
Created attachment 39493 [details]
caretRangeFromPointInNonTextNode.html
Comment 16 Xiaomei Ji 2009-09-11 17:18:40 PDT
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
Comment 17 Sam Weinig 2009-09-11 17:44:37 PDT
This bug is now closed.  Please file any bugs in the implementation in a separate bug.