824965 - Frame representing text area gets pseudo-incorrect offset from GetContentOffsetsFromPoint

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Scott Johnson (:jwir3)]

Attachment: testcase

GetContentOffsetsFromPoint() returns a (seemingly) incorrect offset for block frames representing text areas, if the point given is to the right of the last text frame in the block. (A test case is given, but relies on bug 654352 to perform the click point->offset point conversion).

The offset that is returned is typically 1. During some debugging, I've realized that the frame being returned from GetSelectionClosestFrame() is a BRFrame. This appears to be because we're inserting <br> content nodes into the content of the text area. Technically, this behavior is correct, so I'm hesitant to change the GetContentOffsetsFromPoint() or GetSelectionClosestFrame(), but it's sub-optimal behavior for bug 654352.

NOTE: We don't do the same for <input type="text"> elements, but this is likely because there aren't any line breaks.

The ideal fix would be to not insert <br> elements into the content of the text area, but there are probably other ways to deal with this.  Thoughts and comments are appreciated!

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b824965

This is a hacky solution, but I'm not sure of another way to proceed here.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 696523 [details] [diff] [review]
b824965

You don't need to test for being arbitrarily deep within a textarea; textareas don't have arbitrary structures inside them.  I think all you need to check for is that your current frame is a brFrame and its grandparent's node is a textarea (with appropriate null-checks), plus the next/previous sibling conditions that you already have.  (Its parent is presumably the div with class="anonymous-div".)

But it's not clear to me why you need to do this for a br frame that's at the end of a textarea but not for any earlier br frames.  Could you explain?

And might it be good to set frameEdge and afterFrame to true, in case the user happened to click exactly on the br and they're not already set?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, what does this make the function end up returning?  Is the goal to end up returning the text frame/node inside the textarea's anonymous content, or to return the textarea?  (Note that we should never expose the anonymous content inside the textarea to unprivileged Web content.)

Comment 4

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #2)
> You don't need to test for being arbitrarily deep within a textarea;
> textareas don't have arbitrary structures inside them.  I think all you need
> to check for is that your current frame is a brFrame and its grandparent's
> node is a textarea (with appropriate null-checks), plus the next/previous
> sibling conditions that you already have.  (Its parent is presumably the div
> with class="anonymous-div".)
Ah, ok. I wasn't aware of this. I'll make that change.

> But it's not clear to me why you need to do this for a br frame that's at
> the end of a textarea but not for any earlier br frames.  Could you explain?
Through my (albeit somewhat limited) testing, I found that newlines in textareas don't insert <br> frames. They insert newlines ("\n") into the text frame instead. Perhaps I'm missing something, but I was unsure how a <br> frame would be inserted in the middle of the content of a textarea.

> And might it be good to set frameEdge and afterFrame to true, in case the
> user happened to click exactly on the br and they're not already set?
Yeah, that makes sense. I'll make that change as well.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Scott Johnson (:jwir3) from comment #4)
> > But it's not clear to me why you need to do this for a br frame that's at
> > the end of a textarea but not for any earlier br frames.  Could you explain?
> Through my (albeit somewhat limited) testing, I found that newlines in
> textareas don't insert <br> frames. They insert newlines ("\n") into the
> text frame instead. Perhaps I'm missing something, but I was unsure how a
> <br> frame would be inserted in the middle of the content of a textarea.

OK; we used to have <br>s in the middle, but it's good we got rid of that.

That said, if they're always at the end, it (a) doesn't seem like there's any need to check what's after them and (b) if they weren't at the end, you'd probably want the same handling that you get if they were at the end, no?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #5)
> (b) if they weren't at the end,
> you'd probably want the same handling that you get if they were at the end,
> no?

... at least for some possible answers to comment 3

Comment 7

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #3)
> Also, what does this make the function end up returning?  Is the goal to end
> up returning the text frame/node inside the textarea's anonymous content, or
> to return the textarea?  (Note that we should never expose the anonymous
> content inside the textarea to unprivileged Web content.)

The function ends up returning an HTMLTextAreaElement. I know we talked about this, but could you elaborate once more as to WHY we don't want to return the anonymous content? Is it just an encapsulation/data-hiding rationale, or is there actually a security reason behind it?

Comment 9

[(no longer active)]

Comment on attachment 696523 [details] [diff] [review]
b824965

This patch is sort of terrible as it adds an ugly dependency on the internal structure of textareas and doesn't add any tests, etc...  I'm not even convinced that what comment 0 describes is a real bug.  What does the spec say should happen here?

Comment 10

[Scott Johnson (:jwir3)]

(In reply to :Ehsan Akhgari from comment #9)
> Comment on attachment 696523 [details] [diff] [review]
> b824965
> 
> This patch is sort of terrible as it adds an ugly dependency on the internal
> structure of textareas and doesn't add any tests, etc... 

Absolutely it's terrible, but I'm not exactly sure what else we could do here. I am open to suggestions. As for tests, a test for this behavior was landed as part of bug 654352, and will be changed once we fix the behavior of it.

> I'm not even
> convinced that what comment 0 describes is a real bug.  What does the spec
> say should happen here?

Unclear... all we have for a spec is http://www.w3.org/TR/cssom-view/#the-caretposition-interface

It says "A caret position gives the position of a text insertion point indicator." I would argue that since clicking in the region after the last text node in a text area makes the cursor indicator (visually) positioned at the end of the last text node, that it should return the textarea node (based on comment 3 by dbaron), and N as the offset, where N is the total number of characters in the textarea.

Ideally, this would just _work_, but it's picking up the anonymous <br> content that's inserted at the end of the textarea's anonymous content. Thus, an alternative is to get rid of this extraneous frame, but dbaron has informed me this will be difficult. Apparently, there are quite a few things that depend on that behavior.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The bit slightly higher up, in http://www.w3.org/TR/cssom-view/#dom-document-caretpositionfrompoint , does answer a bunch of the key questions:  in particular, it has special wording for text inputs that say we should be returning offsets inside their values (rather than offsets in the text of a node as for everything else).

Comment 12

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #11)
> The bit slightly higher up, in
> http://www.w3.org/TR/cssom-view/#dom-document-caretpositionfrompoint , does
> answer a bunch of the key questions:  in particular, it has special wording
> for text inputs that say we should be returning offsets inside their values
> (rather than offsets in the text of a node as for everything else).

If I understand correctly, then, this amounts to being equivalent to what I'm doing in this patch?

Comment 13

[(no longer active)]

Why are we descending into the text control frame at all?  That seems to me like the real bug here, and your patch just fixes the side-effects of that bug.  I would have guessed that my patch in bug 762764 has fixed this kind of issue, but then again I haven't looked into the implementation of caretPositionFromPoint.

Comment 14

[Scott Johnson (:jwir3)]

Ehsan and I spoke about this on IRC. Basically, what we decided to do is refactor nsContentUtils::GetSelectionInTextControl() to take a more general node/offset as input, and then use this in the case of text selection. This function is smarter about how to handle text/br combinations. I'll post the refactoring as a separate patch shortly.

Comment 15

[Scott Johnson (:jwir3)]

Attachment: b824965 (v2)

(In reply to Scott Johnson (:jwir3) from comment #14)
> Ehsan and I spoke about this on IRC. Basically, what we decided to do is
> refactor nsContentUtils::GetSelectionInTextControl() to take a more general
> node/offset as input, and then use this in the case of text selection. This
> function is smarter about how to handle text/br combinations. I'll post the
> refactoring as a separate patch shortly.

I originally tried to do this as suggested above, but there were some issues I hadn't anticipated - namely GetSelectionInTextControl() compares the content passed in with the first child of the text area/input element. Unfortunately, as per a discussion with ehsan on IRC, the frame that's being retrieved (i.e. the one that's displayed) is actually a kind of "mirror" node that has the text area node as it's parent, but contains content as shown on the screen. The text area only has a node as a child that doesn't change - i.e. always has the original content. ehsan suggested I use a more frame-based approach to this to navigate through the correct parent/child relationship, which is what this patch does.

I'm wondering though, whether the implementation for GetSelectionInTextControl is correct, since it traverses the content tree, rather than the frame tree (using nsIFrame::GetContent()).

At any rate, this solution does work for both mobile (i.e. the implementation of position maintenance added in bug 803719) and desktop (I augmented test_caretPositionFromPoint to add new checks).

Comment 16

[(no longer active)]

(In reply to Scott Johnson (:jwir3) from comment #15)
> I'm wondering though, whether the implementation for
> GetSelectionInTextControl is correct, since it traverses the content tree,
> rather than the frame tree (using nsIFrame::GetContent()).

It is, since it's always called with the anonymous div for text controls.

Comment 17

[(no longer active)]

Comment on attachment 734078 [details] [diff] [review]
b824965 (v2)

I don't think I'm the right reviewer for this.

Comment 18

[Scott Johnson (:jwir3)]

Comment on attachment 734078 [details] [diff] [review]
b824965 (v2)

Daniel,

If you could give feedback on this, I'd really appreciate it!

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 734078 [details] [diff] [review]
b824965 (v2)

># HG changeset patch
># Parent e53044984f1b81bf7e7ff93fdd13887d90d3383e
># User Scott Johnson <[e-mail]>
>Bug 824965: Implement a method of getting correct CaretPosition from within anonymous content nodes.
>
>diff --git a/content/base/public/nsContentUtils.h b/content/base/public/nsContentUtils.h
>--- a/content/base/public/nsContentUtils.h
>+++ b/content/base/public/nsContentUtils.h
>+void nsContentUtils::GetAdjustedOffsetInTextControl(nsIFrame* aOffsetFrame,
>+                                                    int32_t aOffset,
>+                                                    int32_t& aOutOffset)
>+{

Add newline after 'void', and add a 
  // static
comment to match e.g. the function right after this one.

Also: Why does this return its result as an outparam? Seems like it'd be simpler/cleaner to just directly return it.  (So, make each of the lines that sets 'aOutOffset' a 'return [value];' statement, and make the return type int32_t instead of void)

>+++ b/content/base/src/nsDOMCaretPosition.cpp
>@@ -2,17 +2,17 @@
>  * 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/. */
> 
> #include "nsDOMCaretPosition.h"
> #include "mozilla/dom/CaretPositionBinding.h"
> #include "nsContentUtils.h"
> 
> nsDOMCaretPosition::nsDOMCaretPosition(nsINode* aNode, uint32_t aOffset)
>-  : mOffset(aOffset), mOffsetNode(aNode)
>+: mOffset(aOffset), mOffsetNode(aNode), mAnonymousContentNode(nullptr)
> {
>   SetIsDOMBinding();
> }
> 
> nsDOMCaretPosition::~nsDOMCaretPosition()
> {
> }
> 
>diff --git a/content/base/src/nsDOMCaretPosition.h b/content/base/src/nsDOMCaretPosition.h
>--- a/content/base/src/nsDOMCaretPosition.h
>+++ b/content/base/src/nsDOMCaretPosition.h
>@@ -41,23 +41,52 @@ public:
>    * node that lies at the point specified.
>    *
>    * @returns The DOM node of the CaretPosition.
>    *
>    * @see Document::caretPositionFromPoint(float x, float y)
>    */
>   nsINode* GetOffsetNode() const;
> 
>+  /**
>+   * Set the anonymous content node that is the actual parent of this
>+   * CaretPosition object. This is used to get the correct bounding box of a
>+   * CaretPosition object that lies within a textarea or input element.
>+   *
>+   * @param aNode A pointer to an nsINode object that is the actual element
>+   *        within which this CaretPosition lies, but is an anonymous content
>+   *        node.
>+   */
>+  void SetAnonymousContentNode(nsINode* aNode)
>+  {
>+    mAnonymousContentNode = aNode;
>+  }
>+
>   nsISupports* GetParentObject() const
>   {
>     return GetOffsetNode();
>   }
> 
>   virtual JSObject* WrapObject(JSContext *aCx, JSObject *aScope)
>     MOZ_OVERRIDE MOZ_FINAL;
> 
> protected:
>   virtual ~nsDOMCaretPosition();
>+
>+  /**
>+   * Retrieve the actual parent DOM node for which this CaretPosition was
>+   * created from.


Drop that terminal "from", in "for which this CaretPosition was created from".


>    [...] In situations where the DOM node for which a CaretPosition
>+   * actually lies within

s/for which/for/ in this sentence, I think?  "for which a CaretPosition" doesn't make sense there.


> [...] an anonymous content node (e.g. a textarea), the
>+   * actual parent is not set as the offset node. In this situation, there are
>+   * circumstances where the actual parent is required, but should not be
>+   * accessible to content.
>+   */
>+  nsINode* GetAnonymousContentNode() const
>+  {
>+    return mAnonymousContentNode;
>+  }

If the getter is protected, then there's not really any point in having a getter, right?  Anyone who could use the getter could instead just use |mAnonymousContentNode| directly.  Maybe just have a setter, and merge the getter's documentation with the setter's documentation?

Also, it looks like the getter is completely unused, as is mAnonymousContentNode itself, unless I'm failing at search somehow.  Presumably you should be using the node somewhere?  Or maybe you don't need it at all anymore?

I didn't do a thorough review since there's a fair amount of background here that I'm not familiar with; I suspect ehsan is the right person to review this, despite comment 17, since he's worked on text/editor stuff and he was involved with coming up with the solution here.  (He also says in IRC that he might've been mistaken in comment 17, from having skimmed over the bug too quickly.)

So: next review should probably go to him, but I'm not kicking it over to him yet, to give you a chance to address the feedback I've provided so far. :)

Comment 20

[Daniel Holbert [:dholbert]]

(oops, sorry for insufficiently deleting patch-chunks in prev. comment)

Comment 21

[Scott Johnson (:jwir3)]

Attachment: b824965 (v3)

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Add newline after 'void', and add a 
>   // static
> comment to match e.g. the function right after this one.

Changed.

> Also: Why does this return its result as an outparam? Seems like it'd be
> simpler/cleaner to just directly return it.  (So, make each of the lines
> that sets 'aOutOffset' a 'return [value];' statement, and make the return
> type int32_t instead of void)

Well, I started by trying to refactor the method below it, GetSelectionInTextControl, but then realized I would need a different method. As such, I copied the outparam style from the previous function, which probably wasn't the best option. :) I've made this change now.

> Drop that terminal "from", in "for which this CaretPosition was created
> from".

Done.

> s/for which/for/ in this sentence, I think?  "for which a CaretPosition"
> doesn't make sense there.

Indeed. Thanks. I've changed it.

> If the getter is protected, then there's not really any point in having a
> getter, right?  Anyone who could use the getter could instead just use
> |mAnonymousContentNode| directly.  Maybe just have a setter, and merge the
> getter's documentation with the setter's documentation?

Yeah, that makes sense. I think this was originally public, but then I wanted to disallow access to the anonymous content node, and thus moved the function without realizing that this would effectively make it useless. ;)

> Also, it looks like the getter is completely unused, as is
> mAnonymousContentNode itself, unless I'm failing at search somehow. 
> Presumably you should be using the node somewhere?  Or maybe you don't need
> it at all anymore?

It's actually used in the patch for bug 803719. I added it in this patch, because it seemed more cohesive here, but that's probably open to debate. I could just as easily add it in the patch for bug 803719.

> I didn't do a thorough review since there's a fair amount of background here
> that I'm not familiar with; I suspect ehsan is the right person to review
> this, despite comment 17, since he's worked on text/editor stuff and he was
> involved with coming up with the solution here.  (He also says in IRC that
> he might've been mistaken in comment 17, from having skimmed over the bug
> too quickly.)
> 
> So: next review should probably go to him, but I'm not kicking it over to
> him yet, to give you a chance to address the feedback I've provided so far.
> :)

Thanks for the feedback. I'm going to kick it over to ehsan now.

Comment 22

[(no longer active)]

Comment on attachment 735345 [details] [diff] [review]
b824965 (v3)

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

(Sorry for saying that I'm not the right person to review this, I totally did not pay enough attention to the patch!)

::: content/base/src/nsContentUtils.cpp
@@ +6772,5 @@
> +      !aOffsetFrame->GetNextSibling()) {
> +    return aOffsetFrame->GetParent()->GetFirstPrincipalChild()->GetContent()->Length();
> +  }
> +
> +  return aOffset;

Please document what this function is doing.  It's not clear why the above two conditions do the right thing.

::: content/base/src/nsDOMCaretPosition.h
@@ +59,5 @@
> +   *        node.
> +   */
> +  void SetAnonymousContentNode(nsINode* aNode)
> +  {
> +    mAnonymousContentNode = aNode;

This seems to be a write-only variable here.  Where is this supposed to be used?

@@ +77,3 @@
>    uint32_t mOffset;
>    nsCOMPtr<nsINode> mOffsetNode;
> +  nsCOMPtr<nsINode> mAnonymousContentNode;

You should add this to the cycle collector list.

::: content/base/src/nsDocument.cpp
@@ +9350,5 @@
>      ptFrame->GetContentOffsetsFromPoint(adjustedPoint);
>  
>    nsCOMPtr<nsIContent> node = offsets.content;
>    uint32_t offset = offsets.offset;
> +  nsINode* anonNode = node;

Please make this a string ref to make sure that nothing you do below can kill it prematurely.

@@ +9368,2 @@
>        node = nonanon;
> +      offset = outOffset;

No need for the additional outOffset variable, just assign the rest of GetAdjustedOffsetInTextControl directly to offset.

Comment 23

[Scott Johnson (:jwir3)]

Attachment: b824965 (v4)

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22)
> (Sorry for saying that I'm not the right person to review this, I totally
> did not pay enough attention to the patch!)

No worries.

> Please document what this function is doing.  It's not clear why the above
> two conditions do the right thing.

I documented it now in the latest version. Conceptually, I tried to mirror the function below it (GetSelectionInTextControl).

> This seems to be a write-only variable here.  Where is this supposed to be
> used?

Yes, I've removed this now and placed it into the patch where it's actually used (bug 803719). I'm not sure why I originally put it in this patch.

> You should add this to the cycle collector list.

I've done this in the latest revision of the patch for bug 803719, where this is added.

> @@ +9368,2 @@
> >        node = nonanon;
> > +      offset = outOffset;
> 
> No need for the additional outOffset variable, just assign the rest of
> GetAdjustedOffsetInTextControl directly to offset.
 
Removed.

> Please make this a strong ref to make sure that nothing you do below can
> kill it prematurely.

Done, but again, this was added to the patch for bug 803719, as that's where it's used.

Comment 24

[Scott Johnson (:jwir3)]

Try Run:
https://tbpl.mozilla.org/?tree=Try&rev=49e7459ff4dc

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65a30bc9da3c

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65a30bc9da3c