Closed Bug 654352 Opened 13 years ago Closed 11 years ago

document.caretPositionFromPoint API

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
fennec + ---

People

(Reporter: liucougar, Assigned: jwir3)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 14 obsolete files)

2.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.94 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
according to:
http://www.w3.org/TR/cssom-view/#dom-documentview-caretrangefrompoint

The caretRangeFromPoint(x, y) method, when invoked, must return an empty text range for the position where a text insertion point indicator would have been inserted if editing was enabled and hit testing was performed at the coordinates x,y in the viewport. If either argument is negative, x is greater than the viewport width excluding the size of a rendered scroll bar (if any), y is greather than the viewport height excluding the size of a rendered scroll bar (if any), or no insertion point indicator would have been inserted, the method must return null. [DOM2TR]

this is the standard way of mapping between a client coordinate to a range, which can replace the rangeParent/rangeOffset properties on mouse events (which are not standard, and only implemented in Firefox, and limited to mouse events)
(In reply to comment #0)
> according to:
> http://www.w3.org/TR/cssom-view/#dom-documentview-caretrangefrompoint

That document is old. The valid one is 
http://dev.w3.org/csswg/cssom-view/#extensions-to-the-document-interface
which doesn't have caretRangeFromPoint.
caretRangeFromPoint has some obvious problems for example with 
replaced elements, such as <input> and <textarea>.
Changing the summary. We can use this bug for caretPositionFromPoint.
Summary: document.caretRangeFromPoint API → document.caretPositionFromPoint API
...but how caretPositionFromPoint is specified needs to be still
reviewed properly before implementing it.
does it make sense to just expose the underlying logic for generating rangeParent/rangeOffset on mouse events? (adjust it for any inconsistency with regards to current draft)

IMHO, that something is not properly specified/reviewed does not mean it can't be implemented (if that's the case, no new feature would have been introduced)
liucougar: the most important part of reviewing a spec is figuring out how we *want* things to work. Obviously we can't implement an API without figuring that out.

Another important aspect is that I think we should moz-prefix things.
I think CaretPosition needs an extra flag to indicate "hint left" or "hint right", the equivalent of our ContentOffsets::associateWithNext. Other than that, the caretPositionFromPoint spec looks good to me.
ContentOffsets::associateWithNext sounds useful in the CaretPosition

I may be able to give it a shot at coming up with a patch to implement this. could anyone provide some hints on where to start?
looks like nsIFrame::GetContentOffsetsFromPoint can do the heavy lifting, but it has the following comments: 
/**
 * This function calculates the content offsets for selection relative to
 * a point.  Note that this should generally only be callled on the event
 * frame associated with an event because this function does not account
 * for frame lists other than the primary one.
 * @param aPoint point relative to this frame
 */

the "generally only be callled on the event frame" part concerns me: document.caretPositionFromPoint can be called from places other than event handlers.
That's fine. The important thing is that you first use the point to identify the frame that would handle a mouse event at that point, i.e. by calling nsLayoutUtils::GetFrameForPoint. Then you call GetEventCoordinatesRelativeTo with that frame, then you call GetContentOffsetsFromPoint on that frame.
Assignee: nobody → liucougar
Attached patch patch (obsolete) — Splinter Review
Assignee: liucougar → blassey.bugs
Attachment #542194 - Flags: review?(roc)
Comment on attachment 542194 [details] [diff] [review]
patch

Review of attachment 542194 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +8402,4 @@
>    return NS_OK;
>  }
>  
> +class nsDOMCaretPosition : public nsIDOMCaretPosition

I think we need cycle collection support here, since you're holding a strong reference to a node. Imagine what happens if a script stores a CaretPosition as an expando property of the DOM node, for example.

@@ +8435,5 @@
> +{
> +  nsCOMPtr<nsIPresShell> shell = GetShell();
> +  nsIFrame* rootFrame = shell->GetRootFrame();
> +  nsPoint widgetOffset;
> +  nsIWidget* widget = rootFrame->GetView()->GetNearestWidget(&widgetOffset);

Why are we going through views and widgets here?

I think you can just find the frame for the point using nsLayoutUtils::GetFrameForPoint like ElementFromPointHelper does, then you can call nsIFrame::GetContentOffsetsFromPoint on that frame.

There is one issue I hadn't thought of, which is that document.caretPositionFromPoint should always return a node in that document (similar to what ElementFromPointHelper does). So when you use this from chrome, you'll need to make sure you first find the node at that position from *any* document, and then call caretPositionFromPoint on its document. Make sense?

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +52,5 @@
> +[scriptable, uuid(cf5ad6cf-6f49-4ca7-9b50-590d7bb27a13)]
> +interface nsIDOMCaretPosition : nsISupports {
> +  readonly attribute nsIDOMNode offsetNode;
> +  readonly attribute unsigned long offset;
> +};

I think this deserves its own file. nsIDOMDocument is a busy file already!
(In reply to comment #11)
> Comment on attachment 542194 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 542194 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +8402,4 @@
> >    return NS_OK;
> >  }
> >  
> > +class nsDOMCaretPosition : public nsIDOMCaretPosition
> 
> I think we need cycle collection support here, since you're holding a strong
> reference to a node. Imagine what happens if a script stores a CaretPosition
> as an expando property of the DOM node, for example.
Is there any documentation for implementing cycle collecting support?
I just cargo-cult it from another similar object.
That is the easiest way, but yes, there is even some documentation.
https://developer.mozilla.org/en/Interfacing_with_the_XPCOM_cycle_collector
Blocks: 669816
No longer blocks: 669816
This API is really needed for Fennec's text selection features
Really? I would have thought that using mouse events' rangeParent/offset would be enough for Fennec.

This is still very useful feature.
(In reply to comment #17)
> Really? I would have thought that using mouse events' rangeParent/offset
> would be enough for Fennec.

We do this now and the mouse events cause other actions to happen, when positioned over a link or textbox, for example. We have hacks in place to try and stop the effects.
(In reply to comment #16)
> This API is really needed for Fennec's text selection features

text selection landed in 7, so it should track 7
tracking-fennec: --- → 7+
Comment on attachment 542194 [details] [diff] [review]
patch

nsDOMCaretPosition needs to be cycle collectable, otherwise you may leak
mNode and everything it keeps alive.

Coding style is
if (expr) {
  stmt
}
trackingfirefox7 because this could have implications for more than just Fennec.
Comment on attachment 542194 [details] [diff] [review]
patch

@@ -362,4 +368,6 @@ interface nsIDOMDocument : nsIDOMNode
    */
   void mozSetImageElement(in DOMString aImageElementId,
                           in nsIDOMElement aImageElement);
+
+  nsIDOMCaretPosition caretPositionFromPoint(in float x, in float y);

This will need an IID bump on trunk, and a separate interface on branches, if we'll be taking this on branches...
Also, the patch is actually implementing something else than what the spec says ;)
I added a patch in bug 669816, that basicaly mimimicks mouse-drag selection as in desktop Firefox. It seems to work well. Maybe something like that could be used, instead?
Blocks: 668228
I don't understand the formulation here, but will this request allow me to somehow translate between X/Y coordinates on the page to a character index of an input/textarea element, like that of the current cursor/caret position? I mean it the way to display a dropdown list at the cursor location, or (the other direction) to determine which word in a textarea is hovered by the mouse. If that's something else, is there another bug for it (this is the only one I've found) or can I open a new one for it?
Attached patch patch (obsolete) — Splinter Review
Attachment #551213 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
forgot to add the idl file
Attachment #542194 - Attachment is obsolete: true
Attachment #551213 - Attachment is obsolete: true
Attachment #542194 - Flags: review?(roc)
Attachment #551213 - Flags: review?(roc)
Attachment #551234 - Flags: review?(roc)
(In reply to Olli Pettay [:smaug] from comment #23)
> Also, the patch is actually implementing something else than what the spec
> says ;)

Can you elaborate on that?
The first patch was changing selection, and was doing things like HandleClick.
That stuff has been removed from the new patch, which looks almost
ok to me. (There should be tests, and I believe the patch is missing
stuff which should be added to nsDOMClassInfo so that the class gets right nsIClassInfo )
Attached patch patch to add DOMClassInfo bits (obsolete) — Splinter Review
here's a follow up patch to add the DOMClassInfo stuff you mentioned
Attachment #551262 - Flags: review?(Olli.Pettay)
Comment on attachment 551234 [details] [diff] [review]
patch

Some nit, and one serious, probably-exploitable bug.

>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>+class nsDOMCaretPosition : public nsIDOMCaretPosition

I would kind of like to give this its own file as well... There's a lot of junk in nsDocument.cpp already.

>+  NS_IMETHOD GetOffsetNode(nsIDOMNode **aOffsetNode)

nsIDOMNode** aOffsetNode

>+    *aOffsetNode = mNode;

nsCOMPtr<nsIDOMNode> node = mNode;
node.forget(aOffsetNode);

(Always addref outparams.) <-- Serious bug

>+  NS_IMETHOD GetOffset(PRUint32 *aOffset)

PRUint32* aOffset

>+  nsDOMCaretPosition(nsIDOMNode* aNode, PRUint32 aOffset) :
>+    mNode(aNode), mOffset(aOffset) {}

  nsDOMCaretPosition(nsIDOMNode* aNode, PRUint32 aOffset)
    : mNode(aNode), mOffset(aOffset) {}

>+  nsCOMPtr<nsIDOMNode> mNode;

Why is this public?

>+  /* additional members */

This comment can go, IMO

>+  PRUint32 mOffset;
>+

As well as this empty line.

>+nsDocument::CaretPositionFromPoint(float aX, float aY, nsIDOMCaretPosition **aCaretPos)

nsIDOMCaretPosition** aCaretPos

>+{
>+

Here too

>+  if (aCaretPos) {

Anybody who passes in null here deserves to crash.

>--- /dev/null
>+++ b/dom/interfaces/core/nsIDOMCaretPosition.idl
>@@ -0,0 +1,44 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

"the Mozilla Foundation."

http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html
Attached patch patch (obsolete) — Splinter Review
Attachment #551234 - Attachment is obsolete: true
Attachment #551262 - Attachment is obsolete: true
Attachment #551234 - Flags: review?(roc)
Attachment #551262 - Flags: review?(Olli.Pettay)
Attachment #551275 - Flags: review?(roc)
> >+  if (aCaretPos) {
> 
> Anybody who passes in null here deserves to crash.
As I understand it, that crash would be exploitable from content by simply calling 
"doc.caretPositionFromPoint(0,0)" with nothing to receive the return value. To protect against this I added this to the beginning of the function:

  NS_ENSURE_ARG_POINTER(aCaretPos);
  *aCaretPos = nsnull;
content cannot call caretPositionFromPoint without "receiving" the return value.
Comment on attachment 551275 [details] [diff] [review]
patch

Review of attachment 551275 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed. I think Olli should review this too.

::: content/base/src/nsDocument.cpp
@@ +8413,5 @@
> +    return NS_OK;
> +  nsFrame::ContentOffsets offsets = ptFrame->GetContentOffsetsFromPoint(pt);
> +  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(offsets.content);
> +
> +  *aCaretPos = new nsDOMCaretPosition(node, offsets.StartOffset());

I think you should use .offset here. Per the documention of ContentOffsets: "The primary offset is the expected position of the cursor calculated from a point".

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +363,5 @@
>     */
>    void mozSetImageElement(in DOMString aImageElementId,
>                            in nsIDOMElement aImageElement);
> +
> +  nsIDOMCaretPosition caretPositionFromPoint(in float x, in float y);

IID rev.
Attachment #551275 - Flags: review?(roc)
Attachment #551275 - Flags: review?(Olli.Pettay)
Attachment #551275 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
Attachment #551275 - Attachment is obsolete: true
Attachment #551275 - Flags: review?(Olli.Pettay)
Attachment #551384 - Flags: review?(Olli.Pettay)
Comment on attachment 551384 [details] [diff] [review]
patch


>+class nsDOMCaretPosition : public nsIDOMCaretPosition
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsDOMCaretPosition, nsIDOMCaretPosition)
_AMBIGUOUS is probably not needed



>+  NS_IMETHOD GetOffsetNode(nsIDOMNode** aOffsetNode);
>+  NS_IMETHOD GetOffset(PRUint32* aOffset);
NS_DECL_NSIDOMCARETPOSITION

>+  nsDOMCaretPosition(nsIDOMNode* aNode, PRUint32 aOffset);
>+
>+private:
>+  nsCOMPtr<nsIDOMNode> mNode;
>+
>+protected:
>+  virtual ~nsDOMCaretPosition();
>+  PRUint32 mOffset;

Why mOffset is protected but mNode is private?
Both could be just protected, or private.



>+nsDocument::CaretPositionFromPoint(float aX, float aY, nsIDOMCaretPosition** aCaretPos)
>+{
>+  NS_ENSURE_ARG_POINTER(aCaretPos);
>+  *aCaretPos = nsnull;
>+  
>+  nscoord x = nsPresContext::CSSPixelsToAppUnits(aX);
>+  nscoord y = nsPresContext::CSSPixelsToAppUnits(aY);
>+  nsPoint pt(x, y);
>+
>+  nsIPresShell *ps = GetShell();
>+  NS_ENSURE_STATE(ps);
The spec doesn't actually specify this case.
The most logical thing is to return null. I'll file a spec bug.
So
if (!ps) {
  return NS_OK;
}

>+  nsIFrame *rootFrame = ps->GetRootFrame();
>+
>+  // XUL docs, unlike HTML, have no frame tree until everything's done loading
>+  if (!rootFrame)
>+    return NS_OK; // return null to premature XUL callers as a reminder to wait
Nit,
if (expr) {
  stmt;
}

>+
>+  nsIFrame *ptFrame = nsLayoutUtils::GetFrameForPoint(rootFrame, pt, PR_TRUE,
>+                                                      PR_FALSE);
>+  if (!ptFrame)
>+    return NS_OK;
Ditto.

this really needs tests.
r=me if you add tests.
Attachment #551384 - Flags: review?(Olli.Pettay) → review+
Attached patch patch, ready to land (obsolete) — Splinter Review
updated patch for smaug's review
Attachment #551384 - Attachment is obsolete: true
Attachment #551464 - Flags: review+
Attached patch test patch, WIP (obsolete) — Splinter Review
Here's what I"ve got for a test, however it fails with:
Passed: 0
Failed: 1
Todo: 0
failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass at http://mochi.test:8888/tests/content/base/test/bug654352.html:26

in addition, I get 1 warning and 1 error on my console:
Use of enablePrivilege is deprecated.  Please use code that runs with the system principal (e.g. an extension) instead.
http://mochi.test:8888/tests/content/base/test/bug654352.html
0
Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass
http://mochi.test:8888/tests/content/base/test/bug654352.html
26
Attachment #551465 - Flags: feedback?(Olli.Pettay)
Oh, one thing, quite major one. The patch doesn't handle native anonymous content.
So if the position is inside input element, the native anonymous element
shouldn't be returned, but the input element.
(In reply to Olli Pettay [:smaug] from comment #40)
> Oh, one thing, quite major one. The patch doesn't handle native anonymous
> content.
> So if the position is inside input element, the native anonymous element
> shouldn't be returned, but the input element.

I don't know what to do with this
If the node is anonymous, you need to find the nearest non-anonymous ancestor and return that. You'll also need to return an adjusted offset. You could check NODE_IS_IN_ANONYMOUS_SUBTREE.

Not sure what to do about XBL though.
so, walking up the content nodes to find a non-anonymous one is relatively strait forward. But how do you adjust the offset?
XBL isn't really available to web content, so we don't probably need
to handle that in any special way.

For native anonymous, we probably need to special case the cases we care
about: input and textarea. They have internal <div> and the offset is related
to that. So we can probably just keep the offset but change the node to
input/textarea.
note, the spec does also specify few cases when we should return null.
IMO, if the point points to any scrollbar, we should return null. 
Same probably with video controls. Both those are native anonymous.
We also need to ensure that the handling for <video> is correct. I think the way it uses XBL results in all video controls being native anonymous, but it's worth checking.
(In reply to Olli Pettay [:smaug] from comment #44)
> XBL isn't really available to web content, so we don't probably need
> to handle that in any special way.

Marquee uses xbl bindings.
tracking-fennec: 7+ → 8+
> failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred:
> Permission denied for <http://mochi.test:8888> to create wrapper for object
> of class UnnamedClass at
> http://mochi.test:8888/tests/content/base/test/bug654352.html:26
Is this just an error in the dom class info stuff? does this need flags other than DOM_DEFAULT_SCRIPTABLE_FLAGS?
DOM_DEFAULT_SCRIPTABLE_FLAGS should be enough, but now I see,
you miss the classinfo stuff in NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsDOMCaretPosition)
NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(CaretPosition)
Attachment #551464 - Attachment is obsolete: true
Attachment #551465 - Attachment is obsolete: true
Attachment #551465 - Flags: feedback?(Olli.Pettay)
Attachment #552644 - Flags: review?(Olli.Pettay)
Whiteboard: [needs-review (smaug)]
Attached patch patch (obsolete) — Splinter Review
added anonymous content to the test, which revealed a flaw in this implementation. The spec specifies unsigned long for the offset. In gecko we use signed offsets. 

Seems like we should implement this with signed longs and push for the spec to be updated accordingly.
Attachment #552644 - Attachment is obsolete: true
Attachment #552644 - Flags: review?(Olli.Pettay)
Attachment #553374 - Flags: review?(Olli.Pettay)
(In reply to Brad Lassey [:blassey][blassey@mozilla.com] from comment #51)
> Created attachment 553374 [details] [diff] [review]
> patch
> 
> added anonymous content to the test, which revealed a flaw in this
> implementation. The spec specifies unsigned long for the offset. In gecko we
> use signed offsets. 
> 
> Seems like we should implement this with signed longs and push for the spec
> to be updated accordingly.

Why? Can the offset be negative?
Comment on attachment 553374 [details] [diff] [review]
patch


>+  nsFrame::ContentOffsets offsets = ptFrame->GetContentOffsetsFromPoint(pt);
>+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(offsets.content);
>+  nsIContent* ptContent = offsets.content;
>+  PRInt32 offset = offsets.offset;
>+  if (ptContent &&  ptContent->IsInAnonymousSubtree()) {
>+    nsTArray<nsIContent*> ancestors;
>+    nsTArray<PRInt32> nodeOffsets;
>+    nsContentUtils::GetAncestorsAndOffsets(node, offset, &ancestors, &nodeOffsets);
>+    PRInt32 index;
>+    for (index = 1; index < ancestors.Length() && index < nodeOffsets.Length() && 
>+           ancestors[index] && ancestors[index]->IsInAnonymousSubtree(); index++);
>+    node = do_QueryInterface(ancestors[index]);
>+    offset = nodeOffsets[index];
>+  }


So why is this correct when offsets.content is in native anonymous content of an input element?
Doesn't offset always end up being -1 in that case? It should be the position of the caret in
the input element (if mouse was clicked at the point).
(In reply to Ms2ger from comment #52)
> Why? Can the offset be negative?

The offset returned from GetAncestorsAndOffsets() for the first non-anonymous node is negative when the point is over an <input type="text"/>.

(In reply to Olli Pettay [:smaug] from comment #53)
> So why is this correct when offsets.content is in native anonymous content
> of an input element?
Is it correct? These functions aren't exactly documented well.
> Doesn't offset always end up being -1 in that case? It should be the
> position of the caret in
> the input element (if mouse was clicked at the point).
The offset seems to be -1 consistently for my test. What should it be? and how would you determine it pragmatically?
From the spec

"If at the coordinates x,y in the viewport a text insertion point indicator would have been inserted in a text entry widget which is also a replaced element return a caret position with its properties set as follows:

caret node

    The node corresponding to the text entry widget.
caret offset

    The amount of 16-bit units to the left of where the text insertion point indicator would have inserted."
so you're saying that input boxes and text areas need special handling? Why doesn't GetAncestorsAndOffsets() return the right thing?
Trying to special case input fields

  if (offset <= 0 ) {
      offset = 0;
      PRBool isText;
      nsITextControlFrame* textControlFrame = nsnull;
      nsCOMPtr<nsGenericHTMLElement> el;
      printf("%d\n", __LINE__);
      nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(node);
      if ( input &&
           NS_SUCCEEDED(input->MozIsTextField(PR_FALSE, &isText)) && isText ) {
        nsIntPoint cursor(x, y);
        nsIWidget* window = rootFrame->GetNearestWidget()->GetTopLevelWidget(); 
        nsQueryContentEvent charAtPt(PR_TRUE, NS_QUERY_CHARACTER_AT_POINT, window);
        charAtPt.time = PR_IntervalNow();
        nsEventStatus status;
        window->DispatchEvent(&charAtPt, status);
        if (charAtPt.mSucceeded)
          offset = charAtPt.mReply.mOffset;
        else
          printf("mSucceeded is false, offset is %d\n", charAtPt.mReply.mOffset);
      }
    }
  }
however its failing here [https://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#99] during the dispatch which means we don't get anything meaningful from the query. Any ideas?
I don't understand what comment 57 tries to do.

The attached patch is mostly ok, but it handles native anonymous content
in wrong way. If the position points to Text node inside a
text form control, you could use the offset related to text node.

One thing to check: How should the API work when the text is rtl;
Attachment #553374 - Flags: review?(Olli.Pettay) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #553374 - Attachment is obsolete: true
Attachment #553751 - Flags: review?(Olli.Pettay)
Comment on attachment 553751 [details] [diff] [review]
patch


>+  nsFrame::ContentOffsets offsets = ptFrame->GetContentOffsetsFromPoint(pt);
>+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(offsets.content);
>+  nsIContent* ptContent = offsets.content;
>+  PRInt32 offset = offsets.offset;
>+  if (ptContent &&  ptContent->IsInAnonymousSubtree()) {
IsInNativeAnonymousSubtree()
And then you could just use
ptContent->FindFirstNonNativeAnonymous()
and if that returns input element (which is MozIsTextField) or textarea,
you could return offset
Otherwise returned node should be null, and offset 0.
Attachment #553751 - Flags: review?(Olli.Pettay) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #553751 - Attachment is obsolete: true
Attachment #554096 - Flags: review?(Olli.Pettay)
Comment on attachment 554096 [details] [diff] [review]
patch


>+/*  if (ptContent &&  ptContent->IsInAnonymousSubtree()) {
>+    nsTArray<nsIContent*> ancestors;
>+    nsTArray<PRInt32> nodeOffsets;
>+    nsContentUtils::GetAncestorsAndOffsets(node, offset, &ancestors, &nodeOffsets);
>+    PRInt32 index;
>+    for (index = 1; index < ancestors.Length() && index < nodeOffsets.Length() && 
>+           ancestors[index] && ancestors[index]->IsInAnonymousSubtree(); index++);
>+    node = do_QueryInterface(ancestors[index]);
>+    nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(node);
>+    nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea = do_QueryInterface(node);
>+    PRBool isText;
>+    if (!textArea &&  !input ||
>+        NS_FAILED(input->MozIsTextField(PR_FALSE, &isText)) || !isText)
>+      offset = nodeOffsets[index];
>+      }*/
I assume you'll remove this.
Attachment #554096 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 554096 [details] [diff] [review]
patch

>--- /dev/null
>+++ b/content/base/src/nsDOMCaretPosition.h

>+#ifndef NSDOMCARETPOSITION_H
>+#define NSDOMCARETPOSITION_H

Please use nsDOMCaretPosition_h.

>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp

>+  if (ptContent &&  ptContent->IsInNativeAnonymousSubtree()) {

Just one space will do after the &&

>+    if (textArea ||  input &&
>+        NS_SUCCEEDED(input->MozIsTextField(PR_FALSE, &isText)) && isText) {

And here after the ||. Please put parens around the second clause to clarify the operator precedence.

>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -503,6 +503,7 @@ _TEST_FILES2 = \
> 		test_bug666604.html \
> 		test_bug675121.html \
> 		file_bug675121.sjs \
>+		bug654352.html \

Will this even run without the "test_" in front?

And then land it, before I find more to complain about :)
> >+		bug654352.html \
> 
> Will this even run without the "test_" in front?

Good catch! Thanks!
Attachment #554096 - Flags: approval-mozilla-aurora?
We had a long discussion about this bug and bug 667243 for mozilla-aurora. In the end we decided we would take it on Aurora ifdef'd for mobile-only and one central we'll take it without them.
I just realized we should probably implement this with a moz prefix for now. Anne, what do you think?
http://hg.mozilla.org/mozilla-central/rev/70b03a8b5a95
http://hg.mozilla.org/mozilla-central/rev/66dbf7e560ca
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67)
> I just realized we should probably implement this with a moz prefix for now.

Yeah, I really think we should. So does Olli. Brad, can you do it?
Comment on attachment 554464 [details] [diff] [review]
patch to moz prefix


>-  nsIDOMCaretPosition caretPositionFromPoint(in float x, in float y);
>+  nsIDOMCaretPosition mozCaretPositionFromPoint(in float x, in float y);
Update the iid of nsIDOMDocument


Thanks!
Attachment #554464 - Flags: review?(roc) → review+
(In reply to Olli Pettay [:smaug] from comment #71)
> Comment on attachment 554464 [details] [diff] [review]
> patch to moz prefix
> 
> 
> >-  nsIDOMCaretPosition caretPositionFromPoint(in float x, in float y);
> >+  nsIDOMCaretPosition mozCaretPositionFromPoint(in float x, in float y);
> Update the iid of nsIDOMDocument

And its subclasses.
(In reply to Ms2ger from comment #72)
> And its subclasses.
Whoa, really? I don't recall ever having to do that before.
Yeah, we discovered that not revving nsIDOMHTMLDocument when nsIDOMDocument changes causes binary extension crashes.

You can use http://people.mozilla.org/~sfink/uploads/update-uuids to do the work for you.
Hey guys could you give me some steps on how i could verify this issue?
Thanks.
This is already in WebKit. Don't really see the need for a prefix here.
(In reply to Anne van Kesteren from comment #76)
> This is already in WebKit. Don't really see the need for a prefix here.

ok, not landing the prefix patch then
Actually WebKit has caretRangeFromPoint. But OK, at least we checked :-).
I wonder what the caret range is doing and if it's accessible: http://www.w3.org/TR/cssom-view/#caret-range
Depends on: 681387
Comment on attachment 554096 [details] [diff] [review]
patch

Needs more baking on trunk
Attachment #554096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Whiteboard: [inbound]
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #79)
> I wonder what the caret range is doing and if it's accessible:
> http://www.w3.org/TR/cssom-view/#caret-range

What do you mean?
I doesn't matter, it just doesn't seem to do anything.
I did not realize WebKit still had the older version and never updated. Lame. Without prefix still seems fine though.

Martijn, it is used to define behavior of the method.
tracking-fennec: 8+ → ---
tracking-fennec: --- → 9+
We should back this out from Aurora.
Using Bug 681387 to track that.
This is going to be backed out, so it should no longer affect add-on compatibility.
Keywords: addon-compat
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: 9+ → +
Assignee: blassey.bugs → sjohnson
Target Milestone: mozilla9 → ---
Attached patch b654352 (v2) (obsolete) — Splinter Review
I'm pushing this for preliminary review. Aside from un-bitrotting (and administrative stuff like removing PR types and adjusting the boilerplate), I added lines so that offsets are put in frame-relative coordinates. GetOffsetsFromPoint() expects that the coordinates passed into it are frame-relative, which they weren't prior to this new, improved patch. Hence, the problems. 

I've verified that this works as expected with the test cases from bug 681387, BUT I've been unable to get automated tests in for these cases yet. I can't seem to get the mouse click events, when generated via an initEvent() call in JS to call the selection handler code. :( This sucks, because, unless I'm missing something, we can only add automated tests for the kind of test cases Martijn brought up in that bug by changing the selection using an API. This defeats the purpose of verifying that the caretPositionFromPoint and selection focusOffset and focusNode return the same things for a given point. 

Any assistance in this area would be helpful.
Attachment #554096 - Attachment is obsolete: true
Attachment #693483 - Flags: review?(blassey.bugs)
Attachment #693483 - Flags: review?(blassey.bugs)
Added tests for the new aspects, and modified the old test to test the (now) correct values.
Attachment #693483 - Attachment is obsolete: true
Attachment #693642 - Flags: review?(blassey.bugs)
(In reply to Scott Johnson (:jwir3) from comment #86)
> I've verified that this works as expected with the test cases from bug
> 681387, BUT I've been unable to get automated tests in for these cases yet.
> I can't seem to get the mouse click events, when generated via an
> initEvent() call in JS to call the selection handler code. :( This sucks,
> because, unless I'm missing something, we can only add automated tests for
> the kind of test cases Martijn brought up in that bug by changing the
> selection using an API. This defeats the purpose of verifying that the
> caretPositionFromPoint and selection focusOffset and focusNode return the
> same things for a given point. 

Try using EventUtils.js and synthesizeMouse()? That should perform all mouse-related operations including selection.
Attachment #693642 - Flags: review?(blassey.bugs) → review+
Comment on attachment 693642 [details] [diff] [review]
b654352 (v3, now with more tests)

Review of attachment 693642 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +367,5 @@
>     * @see <http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html>
>     */
>    readonly attribute nsIDOMElement mozPointerLockElement;
>  
> +  nsIDOMCaretPosition caretPositionFromPoint(in float x, in float y);

A comment header here explaining what this api does would be helpful.
Attached patch b654352 (v4)Splinter Review
Converted to webidl. Sending over to Ms2ger for final review on these bindings, then I will push to m-c.

(Along with the moz-prefix patch, too).
Attachment #693642 - Attachment is obsolete: true
Attachment #694551 - Flags: review?(Ms2ger)
Comment on attachment 694551 [details] [diff] [review]
b654352 (v4)

Review of attachment 694551 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I've found a few small issues still :)

I think you'll need to add to dom/tests/mochitest/general/test_interfaces.html too.

::: content/base/src/nsDOMCaretPosition.cpp
@@ +8,5 @@
> +
> +nsDOMCaretPosition::nsDOMCaretPosition(nsINode* aNode, uint32_t aOffset)
> +  : mOffset(aOffset), mOffsetNode(aNode)
> +{
> +  SetIsDOMBinding();

Assert that mOffsetNode is null, or make it nullable in the IDL file (but then you'll need to call the getter 'GetOffsetNode' instead of just 'OffsetNode').

Yeah, you'll need to make it nullable. Add a note in the IDL that that's necessary because of anonymous content, please.

::: content/base/src/nsDOMCaretPosition.h
@@ +15,5 @@
> + * that node, in the DOM tree.
> + *
> + * http://www.w3.org/TR/cssom-view/#dom-documentview-caretrangefrompoint
> + *
> + * @see nsDOMDocument::caretPositionFromPoint(float x, float y)

nsDOMDocument isn't a thing, afaik. Document:: is fine

@@ +41,5 @@
> +   * node that lies at the point specified.
> +   *
> +   * @returns The DOM node of the CaretPosition.
> +   *
> +   * @see nsDOMDocument::caretPositionFromPoint(float x, float y)

Same here

@@ +47,5 @@
> +  nsINode* OffsetNode();
> +
> +  nsISupports* GetParentObject() const
> +  {
> +    return nullptr;

Return OffsetNode() here.

@@ +50,5 @@
> +  {
> +    return nullptr;
> +  }
> +
> +  virtual JSObject *WrapObject(JSContext *aCx, JSObject *aScope, bool *aTried)

* to the left

::: content/base/src/nsDocument.cpp
@@ +8422,5 @@
> +  if (!rootFrame) {
> +    return NS_OK; // return null to premature XUL callers as a reminder to wait
> +  }
> +
> +  nsIFrame *ptFrame = nsLayoutUtils::GetFrameForPoint(rootFrame, pt, true,

* to the left (three times)

@@ +8437,5 @@
> +  nsFrame::ContentOffsets offsets =
> +    ptFrame->GetContentOffsetsFromPoint(adjustedPoint);
> +
> +  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(offsets.content);
> +  nsIContent* ptContent = offsets.content;

Replace those two lines by

nsCOMPtr<nsIContent> node = offsets.content;

@@ +8447,5 @@
> +    bool isText;
> +    if (textArea || (input &&
> +                     NS_SUCCEEDED(input->MozIsTextField(false, &isText)) &&
> +                     isText)) {
> +      node = do_QueryInterface(nonanon);

and don't QI here

@@ +8455,5 @@
> +    }
> +  }
> +
> +  nsCOMPtr<nsINode> genericNode = do_QueryInterface(node);
> +  *aCaretPos = new nsDOMCaretPosition(genericNode, offset);

And get rid of 'genericNode'; pass 'node' instead.

::: content/base/test/Makefile.in
@@ +503,5 @@
>  		file_html_in_xhr3.html \
>  		file_html_in_xhr.sjs \
>  		test_bug647518.html \
> +		test_bug654352.html \
> +               test_bug654352-2.html \

Looks like you need to use two tabs here :/

::: dom/interfaces/core/nsIDOMDocument.idl
@@ +12,5 @@
>  interface nsIDOMNodeIterator;
>  interface nsIDOMNodeFilter;
>  interface nsIDOMTreeWalker;
>  interface nsIDOMLocation;
> +interface nsIDOMCaretPosition;

Don't need this anymore

@@ +376,5 @@
> +   *          page coordinates.
> +   * @param y Vertical point at which to determine the caret position, in
> +   *          page coordinates.
> +   */
> +  nsISupports caretPositionFromPoint(in float x, in float y);

Make this

nsISupports /* CaretPosition */ caretPositionFromPoint(in float x, in float y);

::: dom/webidl/DOMCaretPosition.webidl
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +interface DOMCaretPosition {

Hm, actually this should be called 'CaretPosition', not 'DOMCaretPosition'. Sorry for not noticing earlier.
Attachment #694551 - Flags: review?(Ms2ger) → review+
Inbound, with changes requested by Ms2ger and prefixing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e01ca7212c8a
Target Milestone: --- → mozilla20
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c7bc64a114

Your test was failing pretty universally:
32353 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug654352-2.html | expected offset at (115, 35): 11, got: 13
32357 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug654352-2.html | expected offset at (204, 106): 2, got: 8
32358 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug654352-2.html | expected offset at (44, 148): 1, got: 7
32365 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug654352.html | offset in CaretPosition not correct (6== 4)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #93)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c7bc64a114
> 
> Your test was failing pretty universally:
> 32353 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_bug654352-2.html | expected offset at (115,
> 35): 11, got: 13
> 32357 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_bug654352-2.html | expected offset at (204,
> 106): 2, got: 8
> 32358 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_bug654352-2.html | expected offset at (44,
> 148): 1, got: 7
> 32365 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/base/test/test_bug654352.html | offset in CaretPosition not
> correct (6== 4)


Guh... these tests are in need of work. I should've pushed to try. thanks for backing out, Gavin.
Wait, why prefix it?
(In reply to :Ms2ger from comment #95)
> Wait, why prefix it?

From comment 67, roc and Olli seem to think this should be prefixed.
That comment significantly predates our new no-prefixing policy (https://groups.google.com/forum/#!topic/mozilla.dev.platform/34JfwyEh5e4).
Depends on: 824965
Reworked the tests to be a bit more stable, unprefixed (as per comment 97), and landed on inbound again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3cc198a6b83
https://hg.mozilla.org/mozilla-central/rev/c3cc198a6b83
Status: REOPENED → RESOLVED
Closed: 13 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 825499
Depends on: 826060
Depends on: 826069
Component: DOM: Other → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: