Closed
Bug 1026397
Opened 10 years ago
Closed 10 years ago
Should not truncate input string between a surrogate pair at handling maxlength
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Keywords: intl)
Attachments
(2 files, 1 obsolete file)
1.61 KB,
patch
|
smontagu
:
review+
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
5.13 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
This is a part of bug 670837. At least, we shouldn't separate a surrogate pair at handling maxlength in editor. Note that the goal of this bug isn't an ideal goal of this issue. For example, coming patch won't care IVS.
Assignee | ||
Comment 1•10 years ago
|
||
Simon, this tests must represent what you mentioned in bug 670837 comment 8. This drops high surrogate if the max length boundary causes separating a surrogate pair.
Attachment #8441466 -
Flags: feedback?(smontagu)
Assignee | ||
Comment 2•10 years ago
|
||
And the actual logic to drop high surrogate which is remaining by truncating input string in editor. I'll request review to Ehsan if Simon agree with this behavior.
Attachment #8441469 -
Flags: review?(smontagu)
Comment 3•10 years ago
|
||
I think this behavior is fine.
Comment 4•10 years ago
|
||
Comment on attachment 8441466 [details] [diff] [review] part.1 Add tests for inputting surrogate pair character Review of attachment 8441466 [details] [diff] [review]: ----------------------------------------------------------------- feedback+ on the behaviour, but I would like the tests better if you used a helper function instead of repeating so much.
Attachment #8441466 -
Flags: feedback?(smontagu) → feedback+
Updated•10 years ago
|
Attachment #8441469 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Simon Montagu :smontagu from comment #4) > Comment on attachment 8441466 [details] [diff] [review] > part.1 Add tests for inputting surrogate pair character > > Review of attachment 8441466 [details] [diff] [review]: > ----------------------------------------------------------------- > > feedback+ on the behaviour, but I would like the tests better if you used a > helper function instead of repeating so much. Thanks, I'll recreate it.
Assignee | ||
Comment 6•10 years ago
|
||
Updated the tests and add some more tests. https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b349f0e6abf7
Attachment #8441466 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8441469 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Attachment #8442835 -
Flags: review?(ehsan)
Comment 7•10 years ago
|
||
Comment on attachment 8441469 [details] [diff] [review] part.2 Shouldn't separate input string between a surrogate pair at handling maxlength Review of attachment 8441469 [details] [diff] [review]: ----------------------------------------------------------------- Also, please update the comment earlier in this function. ::: editor/libeditor/text/nsTextEditRules.cpp @@ +1249,5 @@ > { > + int32_t oldLength = aOutString->Length(); > + if (oldLength + resultingDocLength > aMaxLength) { > + int32_t newLength = aMaxLength - resultingDocLength; > + if (newLength > 0) { Based on the if condition above, this should always be true, so you can just MOZ_ASSERT it. @@ +1260,5 @@ > + } > + // XXX What should we do if we're removing IVS and its preceding > + // character won't be removed? > + } > + MOZ_ASSERT(newLength >= 0); And then you can remove this MOZ_ASSERT.
Attachment #8441469 -
Flags: review?(ehsan) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8442835 [details] [diff] [review] part.1 Add tests for inputting surrogate pair character Review of attachment 8442835 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/text/tests/test_bug1026397.html @@ +18,5 @@ > +<pre id="test"> > +<script type="application/javascript"> > + > +/** Test for Bug 1026397 **/ > +SimpleTest.expectAssertions(10); Please annotate this with a bug # about the assertions.
Attachment #8442835 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be72c3a82c8b https://hg.mozilla.org/integration/mozilla-inbound/rev/bd91cb6f8ee3 (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8) > Comment on attachment 8442835 [details] [diff] [review] > part.1 Add tests for inputting surrogate pair character > > Review of attachment 8442835 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: editor/libeditor/text/tests/test_bug1026397.html > @@ +18,5 @@ > > +<pre id="test"> > > +<script type="application/javascript"> > > + > > +/** Test for Bug 1026397 **/ > > +SimpleTest.expectAssertions(10); > > Please annotate this with a bug # about the assertions. Filed bug 1028485. Although, I already have a patch for it, I'd like to create automated tests for it.
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/be72c3a82c8b https://hg.mozilla.org/mozilla-central/rev/bd91cb6f8ee3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•