Should not request text checking if location is in password field [chromium/src : main]
Siye Liu (Gerrit)
Attention is currently required from: Xiaocheng Hu.
Siye Liu would like Xiaocheng Hu to review this change.
Should not request text checking if location is in password field
In |HotModeSpellCheckRequester::CheckSpellingAt| we have existing logic
to limit text checking if the provided location is not enabled for spell
check and the root editable element has spellcheck attribute set to
false. This logic can allow spellchecking in password field:
<input type="password" />. The root editable element is an editable
element which doesn't have spellcheck attribute set. The if statement is
false and we continue to perform text checking.
We are relying on |CurrentWordIfTypingInPartialWord| to return a partial word to avoid performing spellcheck in password field which is incorrect.
We should instead check if current location is in password field.
Bug: 1363865
Change-Id: Iae9af8c3e03a49995ee5dece661345da1a0c5557
---
M third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc
1 file changed, 24 insertions(+), 0 deletions(-)
diff --git a/third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc b/third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc
index 382aabc..ab7edb9 100644
--- a/third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc
+++ b/third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc
@@ -109,6 +109,8 @@
return;
processed_root_editables_.push_back(root_editable);
+ if (IsInPasswordField(position))
+ return;
if (!root_editable->IsSpellCheckingEnabled() &&
!SpellChecker::IsSpellCheckingEnabledAt(position)) {
return;
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Siye Liu (Gerrit)
Attention is currently required from: Xiaocheng Hu.
Siye Liu (Gerrit)
Attention is currently required from: Xiaocheng Hu.
1 comment:
Patchset:
Kindly ping. This is a privacy issue. Thanks yoU!
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Xiaocheng Hu (Gerrit)
Attention is currently required from: Siye Liu.
1 comment:
Patchset:
(Sorry for the delay, OOO last week)
Is this a real bug? Do we ever create a spell check request for a password field?
SpellChecker::IsSpellCheckingEnabledAt() already returns false for password fields, so I don't think this patch is needed.
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Siye Liu (Gerrit)
Attention is currently required from: Xiaocheng Hu.
1 comment:
Patchset:
(Sorry for the delay, OOO last week) […]
Chromium doesn't create spellcheck request for password field. But we do have potential issue because we didn't use correct logic to determine password field.
|SpellChecker::IsSpellCheckingEnabledAt| does returns false for password fields. However, in |HotModeSpellCheckRequester| https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/editing/spellcheck/hot_mode_spell_check_requester.cc;l=112-113, the whole if statement returns false for password field so we never satisfy the if (|root_editable->IsSpellCheckingEnabled()| return true).
Since |SpellChecker::IsSpellCheckingEnabledAt| didn't filter out password field as intended, |CurrentWordIfTypingInPartialWord| returns non-null range in password field therefore we won't proceed to create spellcheck request for password field.
Instead of relying on |CurrentWordIfTypingInPartialWord|, we should use explicit check to filter out password fields.
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Xiaocheng Hu (Gerrit)
Attention is currently required from: Siye Liu.
1 comment:
Patchset:
Chromium doesn't create spellcheck request for password field. […]
I see.
My first reaction is to just remove the `root_editable->IsSpellCheckingEnabled()` check, but I'm not sure if this breaks mixed spellcheckability. So this patch should also work.
And this sounds like a real bug, as it's already sending password field contents for checking. We should create a regression test for it, using the `internals` functions to verify that no spell check requests are made:
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Xiaocheng Hu (Gerrit)
Patchset:
I see. […]
Hi Siye, after looking into it more, I think the real issue is that we didn't properly implement the default spell check behavior [1] for form control elements. So the right place to fix is in Element::IsSpellCheckingEnabled().
There's another patch working on a pretty much the issue, which I'm going to take over (crrev.com/c/3904367). Can I also take over your patch?
[1] https://html.spec.whatwg.org/#concept-spellcheck-default
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.
Siye Liu (Gerrit)
Siye Liu abandoned this change.
To view, visit change 3897474. To unsubscribe, or for help writing mail filters, visit settings.