|
|
Created:
4 years, 6 months ago by pals Modified:
3 years, 8 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpellchecking should not be cancelled if editing is done in between
existing content.
Spell checking is done when space is entered. While there
is "some text" after the caret position and there is no
space after the whole text, spell checking is cancelled.
Added an extra " " for spell checking to happen in all cases.
BUG=610557
Patch Set 1 #
Messages
Total messages: 16 (8 generated)
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2054723002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Spellchecking should not be cancelled if editing is done in between existing content. BUG=610557 ========== to ========== Spellchecking should not be cancelled if editing is done in between existing content. Spell checking is done when space is entered. While there is "some text" after the caret position and there is no " " after the whole text, spell checking is cancelled. Added an extra " " for spell checking to happen in all cases. BUG=610557 ==========
sanjoy.pal@samsung.com changed reviewers: + rouslan@chromium.org
Please review the approach. If it is ok, I can modify the test expectations.
Description was changed from ========== Spellchecking should not be cancelled if editing is done in between existing content. Spell checking is done when space is entered. While there is "some text" after the caret position and there is no " " after the whole text, spell checking is cancelled. Added an extra " " for spell checking to happen in all cases. BUG=610557 ========== to ========== Spellchecking should not be cancelled if editing is done in between existing content. Spell checking is done when space is entered. While there is "some text" after the caret position and there is no space after the whole text, spell checking is cancelled. Added an extra " " for spell checking to happen in all cases. BUG=610557 ==========
rouslan@chromium.org changed reviewers: + groby@chromium.org
-rouslan@ +groby@
On 2016/06/15 15:22:22, Rouslan (ツ) wrote: > -rouslan@ > +groby@ I'd prefer it if we didn't just modify the checked text to force a trigger. (For one, I have no idea if that will work in RTL languages) Do you know why/where things are cancelled? It might be better to fix that cancellation (which is clearly wrong)
On 2016/06/15 18:28:08, groby wrote: > On 2016/06/15 15:22:22, Rouslan (ツ) wrote: > > -rouslan@ > > +groby@ > > I'd prefer it if we didn't just modify the checked text to force a trigger. (For > one, I have no idea if that will work in RTL languages) > > Do you know why/where things are cancelled? It might be better to fix that > cancellation (which is clearly wrong) Here https://cs.chromium.org/chromium/src/chrome/renderer/spellchecker/spellcheck_... int code = 0; int length = static_cast<int>(text_length); U16_PREV(text.data(), 0, length, code); UErrorCode error = U_ZERO_ERROR; if (uscript_getScript(code, &error) != USCRIPT_COMMON) { completion->didCancelCheckingText(); return true; } Spell checking is cancelled due the above code. Here we are cancelling spell checking if last character typed is not USCRIPT_COMMON (e.g. " ", "." etc). This is valid if typing is happening at the end of the whole text. For this example <div contenteditable="true" style="width:400px; height:200px; outline:1px solid red;"><br>text</div> If we type above "text", the last character is "t" so, spell checking is cancelled. Hope I could explain it properly.
sanjoy.pal@samsung.com changed reviewers: - rouslan@chromium.org
On 2016/06/16 03:41:55, pals wrote: > On 2016/06/15 18:28:08, groby wrote: > > On 2016/06/15 15:22:22, Rouslan (ツ) wrote: > > > -rouslan@ > > > +groby@ > > > > I'd prefer it if we didn't just modify the checked text to force a trigger. > (For > > one, I have no idea if that will work in RTL languages) > > > > Do you know why/where things are cancelled? It might be better to fix that > > cancellation (which is clearly wrong) > > Here > https://cs.chromium.org/chromium/src/chrome/renderer/spellchecker/spellcheck_... > > int code = 0; > int length = static_cast<int>(text_length); > U16_PREV(text.data(), 0, length, code); > UErrorCode error = U_ZERO_ERROR; > if (uscript_getScript(code, &error) != USCRIPT_COMMON) { > completion->didCancelCheckingText(); > return true; > } > > Spell checking is cancelled due the above code. Here we are cancelling spell > checking if last character typed is not USCRIPT_COMMON (e.g. " ", "." etc). This > is valid if typing is happening at the end of the whole text. For this example > > <div contenteditable="true" style="width:400px; height:200px; outline:1px solid > red;"><br>text</div> > > If we type above "text", the last character is "t" so, spell checking is > cancelled. > > Hope I could explain it properly. We still shouldn't check on every single key press, which is what this modified code does. Also, if you edit existing content, why do we even end up in that code block? Doesn't the !request.compare(0, last_length, last_request_)) ensure that we only enter here on prefix match? Would you mind adding a test case that illustrates the problem?
rob.buis@samsung.com changed reviewers: + rob.buis@samsung.com
Sanjoy, it seems the bug is fixed, is this patch still needed? |