Closed Bug 1254096 Opened 8 years ago Closed 8 years ago

nsIDocument::CaretPositionFromPoint() could return anonymous node/offset for <input> type=number

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: capella, Assigned: capella)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 4 obsolete files)

Attached patch untitled.diff (obsolete) — Splinter Review
While researching Bug 1224216 - copy/paste doesn't work in type=number text fields, I note that CaretPositionFromPoint() doesn't support <input> type=number elements.

see: bug 1224216 comment 7 for quick write-up.

We're deciding to WONTFIX that particular bug, but I'm filing a WIP here in case this is worthwhile to enhance in any case.

cc: ms2ger as I see his nick on related bug 654352.
Blocks: 1224216
Attached patch bug1254096.diff (obsolete) — Splinter Review
Ehsan might be a better reviewer
Assignee: nobody → markcapella
Attachment #8727366 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8729020 - Flags: review?(ehsan)
Comment on attachment 8729020 [details] [diff] [review]
bug1254096.diff

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

Mostly minusing for the last part of my comment.  Please also add a test for this -- I think a test would have caught that part of my comment.

Thanks!

::: dom/base/nsDocument.cpp
@@ +10815,5 @@
>      nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea = do_QueryInterface(nonanon);
> +
> +    // HTMLInputElement type=text and type=number both contain
> +    // anonymous content.
> +    bool isText, isNumber;

Nit: please initialize both to false.

@@ +10823,5 @@
> +        nsAutoString value;
> +        input->GetType(value);
> +        isNumber = value.LowerCaseEqualsLiteral("number");
> +      }
> +    }

This part looks fine...  However, I think you can probably make this much more reliable by checking the kind of frame ptFrame is.  For example, if you want to know whether you have the number control frame, you can do_QueryFrame() it to a nsNumberControlFrame.  And if you want to see whether you have a text control frame, you can similarly check for nsTextControlFrame.  That should make this code more robust.

@@ +10831,5 @@
>        // that we get the appropriate child, as otherwise the offset may not be
>        // correct when we construct a range for it.
>        nsCOMPtr<nsIContent> firstChild = anonNode->GetFirstChild();
>        if (firstChild) {
>          anonNode = firstChild;

I think this part also needs changing, since the number control would contain a totally different native anonymous structure than a normal text box (see nsNumberControlFrame::CreateAnonymousContent).
Attachment #8729020 - Flags: review?(ehsan) → review-
Whiteboard: btpp-active
Blocks: 1255819
Thanks for the suggestions!

Using ptFrame for the do_QueryFrame() doesn't seem to return meaningful results for either nsNumberControlFrame or nsTextControlFrame.

The more appropriate thing seems to be checking a nsITextControlFrame cast from do_QueryFrame() using the non-Chrome content of the ptFrame:

nsIContent* nonAnon = ptFrame->GetContent()->FindFirstNonChromeOnlyAccessContent();
bool isText = static_cast<nsITextControlFrame*>(do_QueryFrame(nonAnon->GetPrimaryFrame()));

This returns correct results for both <input> type="text" or type="number".

This also produces correct results without further changes for deriving the node we use to SetAnonymousContentNode().

Basically, for <textarea> or either <input> type, we wind up populating that with the associated #text node of the offsets content node from ContentOffsetsFromPoint() of the original ptFrame.


fwiw, if I can get it past a review, I'd like to refactor this method first as in the following patch I'll attach (bug1254096_p1.diff). This streamlines things with early exits, removes re-use of vars and multi-path code, and adds clear documentation.

Then we can apply the correction for this bug proper, attached in (bug1254096_p2.diff).
Flags: needinfo?(ehsan)
Attached patch bug1254096_p1.diff (obsolete) — Splinter Review
Refactor, no logic changes
Attachment #8729020 - Attachment is obsolete: true
Attached patch bug1254096_p2.diff (obsolete) — Splinter Review
Actual simplified correction.
(In reply to Mark Capella [:capella] from comment #3)
> Thanks for the suggestions!
> 
> Using ptFrame for the do_QueryFrame() doesn't seem to return meaningful
> results for either nsNumberControlFrame or nsTextControlFrame.

Why not?

> The more appropriate thing seems to be checking a nsITextControlFrame cast
> from do_QueryFrame() using the non-Chrome content of the ptFrame:
> 
> nsIContent* nonAnon =
> ptFrame->GetContent()->FindFirstNonChromeOnlyAccessContent();
> bool isText =
> static_cast<nsITextControlFrame*>(do_QueryFrame(nonAnon->GetPrimaryFrame()));

Note that you're supposed to do it like this:

nsITextControlFrame* textFrame = do_QueryFrame(nonAnon->GetPrimaryFrame());
bool isText = !!textFeame; // or just use !!textFrame directly

> This returns correct results for both <input> type="text" or type="number".
> 
> This also produces correct results without further changes for deriving the
> node we use to SetAnonymousContentNode().

Great!

> Basically, for <textarea> or either <input> type, we wind up populating that
> with the associated #text node of the offsets content node from
> ContentOffsetsFromPoint() of the original ptFrame.

Yeah that seems like the right thing to do.

> fwiw, if I can get it past a review, I'd like to refactor this method first
> as in the following patch I'll attach (bug1254096_p1.diff). This streamlines
> things with early exits, removes re-use of vars and multi-path code, and
> adds clear documentation.
> 
> Then we can apply the correction for this bug proper, attached in
> (bug1254096_p2.diff).

I honestly find the refactored version much more difficult to read.  What do you find difficult to read with the current code?  I'm not 100% sure if I understand the goal behind refactoring it.

More importantly from a getting review perspective, you'll need a test.  :-)
Flags: needinfo?(ehsan)
Attached patch bug1254096.diffSplinter Review
What do I find difficult? Basically, it wraps three distinct code paths / unique solutions into one long train of thought ...

The easy example being the re-purposing of the |node| var as we drop through [0]
Additionally the re-purposing on top of that of the |anonNode| var which itself derives from the meaning of the |node| var at a discrete point in time [1]
And then re-checking partial results from previous conditions [2] at the tail end of the thing

I lean towards early exits and more-tightly coupled logic. The code is fine, and makes sense now, but the initial un-familiar read-through was confusing, is all. Not a biggie, and on the flip-side, my style-tendency to over-comment things does drive others crazy.

:-)

Thanks for your suggestions above ! To keep it easy, perhaps I can wrap it up with this version of the patch.


[0] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=deadb414ee23&mark=10806-10806,10811-10811,10831-10831,10833-10833,10838-10838#10798
[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=deadb414ee23&mark=10808-10808,10822-10822,10824-10824,10840-10840#10798
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=deadb414ee23&mark=10810-10810,10839-10839#10798
Attachment #8730011 - Attachment is obsolete: true
Attachment #8730012 - Attachment is obsolete: true
Attachment #8730313 - Flags: review?(ehsan)
(In reply to Mark Capella [:capella] from comment #7)
> What do I find difficult? Basically, it wraps three distinct code paths /
> unique solutions into one long train of thought ...
> 
> The easy example being the re-purposing of the |node| var as we drop through
> [0]
> Additionally the re-purposing on top of that of the |anonNode| var which
> itself derives from the meaning of the |node| var at a discrete point in
> time [1]
> And then re-checking partial results from previous conditions [2] at the
> tail end of the thing
> 
> I lean towards early exits and more-tightly coupled logic. The code is fine,
> and makes sense now, but the initial un-familiar read-through was confusing,
> is all. Not a biggie, and on the flip-side, my style-tendency to
> over-comment things does drive others crazy.

Fair enough!  I guess my brain is corrupted by having read code like this for years.  :-)

I agree that the variable reusing and repurposing is annoying.  But perhaps that can be addressed by renaming a variable or some such?

The thing that bothered me about your refactoring patch the most was the second case there where it seems to me like that's probably a bug, being promoted to an explicit case...  That being said, I haven't completely investigated whether that's a buggy path...

I'd be happy to take patches to clean this up in other bugs.  :-)
Comment on attachment 8730313 [details] [diff] [review]
bug1254096.diff

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

::: dom/base/nsDocument.cpp
@@ +10810,5 @@
>    bool nodeIsAnonymous = node && node->IsInNativeAnonymousSubtree();
>    if (nodeIsAnonymous) {
>      node = ptFrame->GetContent();
>      nsIContent* nonanon = node->FindFirstNonChromeOnlyAccessContent();
>      nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(nonanon);

Once you address the below comment, input becomes unused, so you can remove it!

@@ +10814,5 @@
>      nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(nonanon);
>      nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea = do_QueryInterface(nonanon);
> +    nsITextControlFrame* textFrame = do_QueryFrame(nonanon->GetPrimaryFrame());
> +    bool isText = !!textFrame;
> +    if (textArea || (input && isText)) {

a textarea will also get an nsTextControlFrame, so you can simplify this check into: |if (isText)|.
Attachment #8730313 - Flags: review?(ehsan) → review+
Well this was somewhat annoying, but I learned that mochitests that work perfectly fine during local desktop testing with an available window.innerWidth/Height of 1276 / 874, may fail when pushed to try where the available window.innerWidth/Height is considerable smaller at 500 / 300.

After a quick test adjustment, the below TRY push looks better.

Assuming no other strangeness develops, and as the patch code itself wasn't at fault, I plan to carry over the r+ and push here in a couple hours.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31f3dbf9988
(In reply to Mark Capella [:capella] from comment #13)
> Well this was somewhat annoying, but I learned that mochitests that work
> perfectly fine during local desktop testing with an available
> window.innerWidth/Height of 1276 / 874, may fail when pushed to try where
> the available window.innerWidth/Height is considerable smaller at 500 / 300.

Yeah...  I've run into this too many times as well.
https://hg.mozilla.org/mozilla-central/rev/e2d350254313
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.