Closed
Bug 998188
Opened 10 years ago
Closed 10 years ago
ContentEventHandler::OnQueryCaretRect() converts native text offset with wrong node
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(7 files, 7 obsolete files)
34.01 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
21.98 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome
2.90 KB,
patch
|
Margaret
:
review+
capella
:
feedback+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
Details | Diff | Splinter Review | |
22.46 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
8.48 KB,
patch
|
Details | Diff | Splinter Review |
This bug causes crash on debug build in TSF mode. > uint32_t offset; > rv = GetFlatTextOffsetOfRange(mRootContent, mFirstSelectedRange, &offset); > NS_ENSURE_SUCCESS(rv, rv); > // strip out native new lines, we want the non-native offset. The offsets > // handed in here are from selection, caretPositionFromPoint, and editable > // element offset properties. We need to match those or things break. > nsINode* startNode = mFirstSelectedRange->GetStartParent(); > if (startNode && startNode->IsNodeOfType(nsINode::eTEXT)) { > offset = ConvertToXPOffset(static_cast<nsIContent*>(startNode), offset); > } For example, on twitter.com, the editor for tweet is created by <div contenteditable>. Then, GetFlatTextOffsetOfRange() computes the offset with children of the <div>. However, ConvertToXPOffset() only refers currently editing text node. If there is composition string, the startNode is dummy text node for the composition string. I.e., it doesn't contain the text before the startNode. Additionally, WidgetQueryContentEvent::mInput::mOffset must be native offset. I don't understand why Jimm and Ehsan changed this in bug 858526, though. I believe that we should backout the patch for bug 858526 and if it's needed, bug 858526 should be fixed with another approach.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=0af81c9ffee5
Assignee | ||
Comment 4•10 years ago
|
||
Perhaps, I'll separate part.1 for easier to review.
Assignee | ||
Comment 5•10 years ago
|
||
Automated test for reproducing this crash bug.
Attachment #8409634 -
Attachment is obsolete: true
Attachment #8409635 -
Attachment is obsolete: true
Attachment #8409636 -
Attachment is obsolete: true
Attachment #8410153 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•10 years ago
|
||
The cause of this regression is, JS component may want to use nsIDOMWindowUtils.sendQueryContentEvent() and nsIDOMWindowUtils.sendSelectionSetEvent() with XP line break since native line break isn't useful for JS. This patch makes ContentEventHandler can work with both native line break and XP line break. If mUseNativeLineBreak of WidgetQueryContentEvent or WidgetSelectionEvent is true, the offset and length are treated as in string including native line breaks. And the result string is generated with native line breaks. Otherwise, the offset and length are treated as in string including XP line breaks (\n). And the result string is generated with XP line breaks. For easy to read the callers, some methods of ContentEventHandler takes LineBreakType instead of boolean.
Attachment #8410161 -
Flags: review?(bugs)
Assignee | ||
Comment 7•10 years ago
|
||
This patch sorts out variable names, offset and length in ContentEventHandler. If they are ambitious, i.e., it may be native offset/length and XP offset/length which are switched by LineBreakType argument, the name should be just offset/length. If they are XP offset/length, the names should be xpOffset/xpLength. If they are native offset/length, the names should be nativeOffset/nativeLength. If they are node offset/length, the names should be nodeOffset/nodeLength.
Attachment #8410165 -
Flags: review?(bugs)
Assignee | ||
Comment 8•10 years ago
|
||
This patch adds aAdditionalFlags to nsIDOMWindowUtils.sendQueryContentEvent() and nsIDOMWindowUtils.sendSelectionSetEvent(). This change does NOT break compatibility with old style. The compatibility is tested in test_sendQueryContentAndSelectionSetEvent.html.
Attachment #8410166 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
Fixes the tests added by bug 858526 with additional flags. The tests want nsIDOMWindowUtils.sendQueryContentEvent() work with XP offset. And also improves EventUtils.js and ChromeUtils.js by explicitly specifying additional flags.
Attachment #8410168 -
Flags: review?(bugs)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Comment on attachment 8410153 [details] [diff] [review] part.1 Add test for this crash on debug build Review of attachment 8410153 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +1000,5 @@ > +function synthesizeQueryCaretRect(aOffset, aWindow) > +{ > + var utils = _getDOMWindowUtils(aWindow); > + if (!utils) { > + return nullptr; Not sure who put this in, but s/ptr//. ::: widget/tests/test_bug998188.html @@ +8,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > +</head> > +<body onload="runTests();"> Instead of this, please use waitForFocus().
Attachment #8410153 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Thank you, Ehsan. (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12) > ::: widget/tests/test_bug998188.html > @@ +8,5 @@ > > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > > + <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> > > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > > +</head> > > +<body onload="runTests();"> > > Instead of this, please use waitForFocus(). Yeah, I did it. However, on Mac OS X 10.6, it causes permanent orange. See https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c1231c979bb8 > 23:57:05 INFO - 2326 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_badMimeType.html > 23:57:05 INFO - 2327 INFO TEST-INFO | MEMORY STAT vsize after test: 3973943296 > 23:57:05 INFO - 2328 INFO TEST-INFO | MEMORY STAT residentFast after test: 450830336 > 23:57:05 INFO - 2329 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 95835128 > 23:57:05 INFO - 2330 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_badMimeType.html | finished in 95ms > 23:57:05 INFO - 2331 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml > 23:57:06 INFO - 2332 INFO TEST-INFO | MEMORY STAT vsize after test: 3976806400 > 23:57:06 INFO - 2333 INFO TEST-INFO | MEMORY STAT residentFast after test: 449814528 > 23:57:06 INFO - 2334 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 106153360 > 23:57:06 INFO - 2335 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | finished in 755ms > 23:57:09 INFO - 2336 INFO Error: Unable to restore focus, expect failures and timeouts. > 23:57:09 INFO - 2337 INFO TEST-START | /tests/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml > 23:57:12 INFO - 2338 INFO Error: Unable to restore focus, expect failures and timeouts. > 23:57:13 INFO - 2339 INFO TEST-INFO | MEMORY STAT vsize after test: 3968774144 > 23:57:13 INFO - 2340 INFO TEST-INFO | MEMORY STAT residentFast after test: 439832576 > 23:57:13 INFO - 2341 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 92097848 > 23:57:13 INFO - 2342 INFO TEST-END | /tests/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml | finished in 4331ms > 23:57:16 INFO - 2343 INFO Error: Unable to restore focus, expect failures and timeouts. > 23:57:16 INFO - 2344 INFO TEST-START | /tests/widget/tests/test_bug998188.html > 23:57:19 INFO - 2345 INFO Error: Unable to restore focus, expect failures and timeouts. > 00:02:30 INFO - 2346 INFO TEST-INFO | dumping last 2 message(s) > 00:02:30 INFO - 2347 INFO TEST-INFO | if you need more context, please use SimpleTest.requestCompleteLog() in your test > 00:02:30 INFO - 2348 INFO TEST-INFO | /tests/widget/tests/test_bug998188.html | must wait for load > 00:02:30 INFO - 2349 INFO TEST-INFO | /tests/widget/tests/test_bug998188.html | must wait for focus > 00:02:31 INFO - TEST-INFO | screencapture: exit 0 > 00:02:31 INFO - 2350 INFO TEST-UNEXPECTED-FAIL | /tests/widget/tests/test_bug998188.html | Test timed out. > 00:02:31 INFO - 2351 INFO TEST-UNEXPECTED-FAIL | /tests/widget/tests/test_bug998188.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run. Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0 After test_handlerApps.xhtml, looks like our window never becomes active. I think that this is bug 933303. So, I think, we have two choice. 1. Use onload event. 2. Disables the test on Mac. The test doesn't need native focus. Therefore, I chose #1. How do you think about this?
Flags: needinfo?(ehsan)
Comment 14•10 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13) > After test_handlerApps.xhtml, looks like our window never becomes active. I > think that this is bug 933303. Yes indeed. Looking at the screenshot, that's what it is. > So, I think, we have two choice. > > 1. Use onload event. > 2. Disables the test on Mac. > > The test doesn't need native focus. Therefore, I chose #1. How do you think > about this? Well, we have a third option, move the test to somewhere before test_handlerApps.xhtml to make sure that we're not affected by this problem right? Please feel free to move it to somewhere in editor/.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 15•10 years ago
|
||
Okay, I'll move it, thanks again.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > Created attachment 8410165 [details] [diff] [review] > part.3 Sort out offset/length variable names in ContentEventHandler > > If they are ambitious, i.e., it may be native offset/length and XP > offset/length which are switched by LineBreakType argument, the name should > be just offset/length. Ugh, I meant "If they are ambiguous, ..."
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8410153 -
Attachment is obsolete: true
Comment 18•10 years ago
|
||
Comment on attachment 8410161 [details] [diff] [review] part.2 ContentEventHandler should support 2 modes, native line break mode and XP line break mode // true if use native line breaks for mOffset and mLength true if native line breaks are used for mOffset and mLength
Attachment #8410161 -
Flags: review?(bugs) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8410165 [details] [diff] [review] part.3 Sort out offset/length variable names in ContentEventHandler I guess this is fine. offset naming is still a bit odd, but we have so many different offsets.
Attachment #8410165 -
Flags: review?(bugs) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8410166 [details] [diff] [review] part.4 nsIDOMWindowUtils.sendQueryContentEvent() and .sendSelectionSetEvent() should take additional flags with its argument for making callers selectable native line break mode or XP line break mode >+ is(caretXP.top, caretNative.top, >+ "The caret top should be same (got: XP, expected: Native)"); I don't understand this types of messages. Why the got-expected part? I would just drop that here and elsewhere > /** > * Synthesize a query content event. Note that the result value returned here > * is in LayoutDevice pixels rather than CSS pixels. > * > * @param aType One of the following const values. And see also each comment > * for the other parameters and the result. >+ * @param aAdditionalFlags See following QUERY_CONTENT_FLAG_* flags. Could you define the flags before using them > */ >+ /** >+ * If SELECTION_SET_FLAG_REVERSE is set, the selection set from >+ * |aOffset + aLength| to |aOffset|. Otherwise, set from |aOffset| to >+ * |aOffset + aLength|. >+ */ >+ const unsigned long SELECTION_SET_FLAG_REVERSE = 0x0001; >+ /** >+ * If SELECTION_SET_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags, >+ * plain text generated from content is created with "\n". Otherwise, >+ * platform dependent. E.g., on Windows, "\r\n" is used. >+ * aOffset and aLength are offset and length in/of the plain text content. >+ */ >+ const unsigned long SELECTION_SET_FLAG_USE_NATIVE_LINE_BREAK = 0x0000; >+ const unsigned long SELECTION_SET_FLAG_USE_XP_LINE_BREAK = 0x0010; I don't understand which flag ordering, I'd start from 0000 and go from there.
Attachment #8410166 -
Flags: review?(bugs) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8410168 [details] [diff] [review] part.5 Fix orange and improve EventUtils.js and ChromeUtils.js update the flags in EventUtils.js to follow nsIDOMWindowUtils flags.
Attachment #8410168 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 22•10 years ago
|
||
About SELECTION_SET_FLAG_* definition in nsIDOMWindowUtils.idl, do you expect this style? I.e., the order is, const unsigned long SELECTION_SET_FLAG_USE_NATIVE_LINE_BREAK = 0x0000; const unsigned long SELECTION_SET_FLAG_REVERSE = 0x0001; const unsigned long SELECTION_SET_FLAG_USE_XP_LINE_BREAK = 0x0010; Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This keeps the old style code behavior.
Attachment #8410166 -
Attachment is obsolete: true
Attachment #8411464 -
Flags: feedback?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8410169 [details] [diff] [review] part.6 MetroFirefox should use XP line break at calling sendQueryContentEvent() in its chrome I think that this is what you expected and wanted change in bug 858526.
Attachment #8410169 -
Flags: review?(jmathies)
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8410170 [details] [diff] [review] part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome These callers must expect the offset/length and the string of its result are generated with "\n". Although, "\n" is used on Android when QUERY_CONTENT_FLAG_USE_NATIVE_LINE_BREAK, let's use right argument in its logic.
Attachment #8410170 -
Flags: review?(margaret.leibovic)
Comment 25•10 years ago
|
||
> Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This
> keeps the old style code behavior.
Oh, that is error prone. Can we really not just fix all the callers to use flags, and not bool params.
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #25) > > > Note that SELECTION_SET_FLAG_REVERSE needs to be 1 since true is 1. This > > keeps the old style code behavior. > Oh, that is error prone. Can we really not just fix all the callers to use > flags, and not bool params. I fixed all callers in our tree by the patches. However, it's possible to be used by add-ons...
Comment 27•10 years ago
|
||
https://mxr.mozilla.org/addons/ doesn't seem to reveal such addons.
Updated•10 years ago
|
Attachment #8410169 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 28•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #27) > https://mxr.mozilla.org/addons/ doesn't seem to reveal such addons. Oh, I've not known that. Hmm, then, XP_LINE_BREAK should be 0x0001. It's same value as for sendQueryContentEvent().
Comment 29•10 years ago
|
||
Comment on attachment 8410170 [details] [diff] [review] part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome Review of attachment 8410170 [details] [diff] [review]: ----------------------------------------------------------------- I also want capella to take a look at this.
Attachment #8410170 -
Flags: feedback?(markcapella)
Comment 30•10 years ago
|
||
Comment on attachment 8410170 [details] [diff] [review] part.7 Firefox for Android should use XP line break at calling sendQueryContentEvent() in its chrome Review of attachment 8410170 [details] [diff] [review]: ----------------------------------------------------------------- wfm - I wouldn't mind a brief comment before the consumers along the lines of "Chrome observes XP styled line-breaks", but I'm overly obsessive and it's not a deal breaker.
Attachment #8410170 -
Flags: feedback?(markcapella) → feedback+
Assignee | ||
Comment 31•10 years ago
|
||
jst: Could you sr the interface change? nsIDOMWindowUtils.sendSelectionSet() breaks compatibility with current build. However, we confirmed that this method isn't used in any addons which can be checked in mxr.
Attachment #8411464 -
Attachment is obsolete: true
Attachment #8411464 -
Flags: feedback?(bugs)
Attachment #8412436 -
Flags: superreview?(jst)
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8410168 -
Attachment is obsolete: true
Comment 33•10 years ago
|
||
Comment on attachment 8412436 [details] [diff] [review] part.4 nsIDOMWindowUtils.sendQueryContentEvent() and .sendSelectionSetEvent() should take additional flags with its argument for making callers selectable native line break mode or XP line break mode Looks good, just a couple of comment tweak suggestions below: /** + * If QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags of + * sendQueryContentEvent(), plain text generated from content is created with Maybe word that something like: If sendQueryContentEvent()'s aAdditionalFlags argument is QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK plain text generated from content is created with... /** + * If QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK is set to aAdditionalFlags of + * sendSelectionSetEvent(), aOffset and aLength are offset and length in/of Same here (language wise), and replace QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK with SELECTION_SET_FLAG_USE_XP_LINE_BREAK. const unsigned long QUERY_CHARACTER_AT_POINT = 3208; + /** Remove the addition of that empty line. + /** + * If SELECTION_SET_FLAG_REVERSE is set, the selection set from Add "is" between selection and is. + * |aOffset + aLength| to |aOffset|. Otherwise, set from |aOffset| to Add "it is" before "set". + * @param aAdditionalFlags See following SELECTION_SET_FLAG_*. Maybe say "See the description of SELECTION_SET_FLAG_*."
Attachment #8412436 -
Flags: superreview?(jst) → superreview+
Updated•10 years ago
|
Attachment #8410170 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9e0440e619 https://hg.mozilla.org/integration/mozilla-inbound/rev/394ef98ad574 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac566b771d7b https://hg.mozilla.org/integration/mozilla-inbound/rev/776018f74fed https://hg.mozilla.org/integration/mozilla-inbound/rev/8305804b3099 https://hg.mozilla.org/integration/mozilla-inbound/rev/93b58802f743 https://hg.mozilla.org/integration/mozilla-inbound/rev/cf785bf038ed Thank you, everybody!
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f9e0440e619 https://hg.mozilla.org/mozilla-central/rev/394ef98ad574 https://hg.mozilla.org/mozilla-central/rev/ac566b771d7b https://hg.mozilla.org/mozilla-central/rev/776018f74fed https://hg.mozilla.org/mozilla-central/rev/8305804b3099 https://hg.mozilla.org/mozilla-central/rev/93b58802f743 https://hg.mozilla.org/mozilla-central/rev/cf785bf038ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Updated•10 years ago
|
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•