Closed
Bug 654352
Opened 13 years ago
Closed 11 years ago
document.caretPositionFromPoint API
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
Comment 1•13 years ago
|
||
(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>.
Comment 2•13 years ago
|
||
Changing the summary. We can use this bug for caretPositionFromPoint.
Summary: document.caretRangeFromPoint API → document.caretPositionFromPoint API
Comment 3•13 years ago
|
||
...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.
Comment 10•13 years ago
|
||
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!
Comment 12•13 years ago
|
||
(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.
Comment 14•13 years ago
|
||
That is the easiest way, but yes, there is even some documentation. https://developer.mozilla.org/en/Interfacing_with_the_XPCOM_cycle_collector
Comment 16•13 years ago
|
||
This API is really needed for Fennec's text selection features
tracking-firefox8:
--- → ?
Comment 17•13 years ago
|
||
Really? I would have thought that using mouse events' rangeParent/offset would be enough for Fennec. This is still very useful feature.
Comment 18•13 years ago
|
||
(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.
Comment 19•13 years ago
|
||
(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
Comment 20•13 years ago
|
||
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 }
Comment 21•13 years ago
|
||
trackingfirefox7 because this could have implications for more than just Fennec.
Comment 22•13 years ago
|
||
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...
Comment 23•13 years ago
|
||
Also, the patch is actually implementing something else than what the spec says ;)
Comment 24•13 years ago
|
||
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?
Comment 25•13 years ago
|
||
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?
Comment 26•13 years ago
|
||
Attachment #551213 -
Flags: review?(roc)
Comment 27•13 years ago
|
||
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?
Comment 29•13 years ago
|
||
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 )
Comment 30•13 years ago
|
||
here's a follow up patch to add the DOMClassInfo stuff you mentioned
Attachment #551262 -
Flags: review?(Olli.Pettay)
Comment 31•13 years ago
|
||
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
Comment 32•13 years ago
|
||
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)
Comment 33•13 years ago
|
||
> >+ 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;
Comment 34•13 years ago
|
||
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+
Comment 36•13 years ago
|
||
Attachment #551275 -
Attachment is obsolete: true
Attachment #551275 -
Flags: review?(Olli.Pettay)
Attachment #551384 -
Flags: review?(Olli.Pettay)
Comment 37•13 years ago
|
||
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+
Comment 38•13 years ago
|
||
updated patch for smaug's review
Attachment #551384 -
Attachment is obsolete: true
Attachment #551464 -
Flags: review+
Comment 39•13 years ago
|
||
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)
Comment 40•13 years ago
|
||
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.
Comment 41•13 years ago
|
||
(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.
Comment 43•13 years ago
|
||
so, walking up the content nodes to find a non-anonymous one is relatively strait forward. But how do you adjust the offset?
Comment 44•13 years ago
|
||
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.
Comment 46•13 years ago
|
||
(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.
Updated•13 years ago
|
tracking-fennec: 7+ → 8+
Comment 47•13 years ago
|
||
> 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?
Comment 48•13 years ago
|
||
DOM_DEFAULT_SCRIPTABLE_FLAGS should be enough, but now I see, you miss the classinfo stuff in NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsDOMCaretPosition)
Comment 49•13 years ago
|
||
NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(CaretPosition)
Comment 50•13 years ago
|
||
Attachment #551464 -
Attachment is obsolete: true
Attachment #551465 -
Attachment is obsolete: true
Attachment #551465 -
Flags: feedback?(Olli.Pettay)
Attachment #552644 -
Flags: review?(Olli.Pettay)
Updated•13 years ago
|
Whiteboard: [needs-review (smaug)]
Comment 51•13 years ago
|
||
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)
Comment 52•13 years ago
|
||
(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 53•13 years ago
|
||
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).
Comment 54•13 years ago
|
||
(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?
Comment 55•13 years ago
|
||
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."
Comment 56•13 years ago
|
||
so you're saying that input boxes and text areas need special handling? Why doesn't GetAncestorsAndOffsets() return the right thing?
Comment 57•13 years ago
|
||
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?
Comment 58•13 years ago
|
||
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;
Updated•13 years ago
|
Attachment #553374 -
Flags: review?(Olli.Pettay) → review-
Comment 59•13 years ago
|
||
Attachment #553374 -
Attachment is obsolete: true
Attachment #553751 -
Flags: review?(Olli.Pettay)
Comment 60•13 years ago
|
||
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-
Comment 61•13 years ago
|
||
Attachment #553751 -
Attachment is obsolete: true
Attachment #554096 -
Flags: review?(Olli.Pettay)
Comment 62•13 years ago
|
||
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 63•13 years ago
|
||
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 :)
Comment 64•13 years ago
|
||
> >+ bug654352.html \
>
> Will this even run without the "test_" in front?
Good catch! Thanks!
Comment 65•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/70b03a8b5a95
Whiteboard: [needs-review (smaug)] → [inbound]
Updated•13 years ago
|
Attachment #554096 -
Flags: approval-mozilla-aurora?
Comment 66•13 years ago
|
||
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.
Keywords: dev-doc-needed
I just realized we should probably implement this with a moz prefix for now. Anne, what do you think?
Comment 68•13 years ago
|
||
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 70•13 years ago
|
||
Attachment #554464 -
Flags: review?(roc)
Comment 71•13 years ago
|
||
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+
Comment 72•13 years ago
|
||
(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.
Comment 73•13 years ago
|
||
(In reply to Ms2ger from comment #72) > And its subclasses. Whoa, really? I don't recall ever having to do that before.
Comment 74•13 years ago
|
||
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.
Comment 75•13 years ago
|
||
Hey guys could you give me some steps on how i could verify this issue? Thanks.
Comment 76•13 years ago
|
||
This is already in WebKit. Don't really see the need for a prefix here.
Comment 77•13 years ago
|
||
(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 :-).
Comment 79•13 years ago
|
||
I wonder what the caret range is doing and if it's accessible: http://www.w3.org/TR/cssom-view/#caret-range
Comment 80•13 years ago
|
||
Comment on attachment 554096 [details] [diff] [review] patch Needs more baking on trunk
Attachment #554096 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•13 years ago
|
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?
Comment 82•13 years ago
|
||
I doesn't matter, it just doesn't seem to do anything.
Comment 83•13 years ago
|
||
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.
Updated•13 years ago
|
tracking-fennec: 8+ → ---
Updated•13 years ago
|
tracking-fennec: --- → 9+
tracking-firefox7:
+ → ---
Updated•13 years ago
|
Keywords: addon-compat
Comment 84•13 years ago
|
||
We should back this out from Aurora. Using Bug 681387 to track that.
Comment 85•13 years ago
|
||
This is going to be backed out, so it should no longer affect add-on compatibility.
Keywords: addon-compat
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•13 years ago
|
tracking-fennec: 9+ → +
Assignee | ||
Updated•12 years ago
|
Assignee: blassey.bugs → sjohnson
Updated•12 years ago
|
Target Milestone: mozilla9 → ---
Assignee | ||
Comment 86•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #693483 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 87•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #693642 -
Flags: review?(blassey.bugs) → review+
Comment 89•11 years ago
|
||
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.
Assignee | ||
Comment 90•11 years ago
|
||
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 91•11 years ago
|
||
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+
Assignee | ||
Comment 92•11 years ago
|
||
Inbound, with changes requested by Ms2ger and prefixing: https://hg.mozilla.org/integration/mozilla-inbound/rev/e01ca7212c8a
Target Milestone: --- → mozilla20
Comment 93•11 years ago
|
||
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)
Assignee | ||
Comment 94•11 years ago
|
||
(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.
Comment 95•11 years ago
|
||
Wait, why prefix it?
Assignee | ||
Comment 96•11 years ago
|
||
(In reply to :Ms2ger from comment #95) > Wait, why prefix it? From comment 67, roc and Olli seem to think this should be prefixed.
Comment 97•11 years ago
|
||
That comment significantly predates our new no-prefixing policy (https://groups.google.com/forum/#!topic/mozilla.dev.platform/34JfwyEh5e4).
Assignee | ||
Comment 98•11 years ago
|
||
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
Comment 99•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3cc198a6b83
Status: REOPENED → RESOLVED
Closed: 13 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 100•11 years ago
|
||
Mentioned on https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers#DOM New pages: https://developer.mozilla.org/en-US/docs/DOM/document.caretPositionFromPoint https://developer.mozilla.org/en-US/docs/DOM/CaretPosition
Keywords: dev-doc-needed → dev-doc-complete
Updated•11 years ago
|
Component: DOM: Other → DOM
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•