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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: capella, Assigned: capella)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 4 obsolete files)
5.15 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | 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.
Assignee | ||
Comment 1•8 years ago
|
||
Ehsan might be a better reviewer
Assignee: nobody → markcapella
Attachment #8727366 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8729020 -
Flags: review?(ehsan)
Comment 2•8 years ago
|
||
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-
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
Refactor, no logic changes
Attachment #8729020 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Actual simplified correction.
Comment 6•8 years ago
|
||
(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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
(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 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7695ae2d52a2
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10678ba7b7a8
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da9e4663a54d
Assignee | ||
Comment 13•8 years ago
|
||
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
Comment 14•8 years ago
|
||
(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.
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2d350254313
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
•