1052286 - [TSF] nsTextStore::SetInputContext() should not ignore "password" state

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>   if (!ThinksHavingFocus() &&
>       aContext.mIMEState.mEnabled == IMEState::ENABLED) {
>     OnFocusChange(true, aWidget, aContext.mIMEState.mEnabled);
>   } else if (ThinksHavingFocus() &&
>              aContext.mIMEState.mEnabled != IMEState::ENABLED) {
>     OnFocusChange(false, aWidget, aContext.mIMEState.mEnabled);
>   }

When IME state is password, nsTextStore should have focus. Therefore, this must be wrong.

However, I've not found any wrong behavior caused by this yet.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

If IME state is changed from non-editable to editable or from editable to non-editable without focus change, nsTextStore needs to emulate focus change in SetInputContext(). Therefore, "password" state is handled same as "enabled".

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8472108 [details] [diff] [review]
Patch

Review of attachment 8472108 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIWidget.h
@@ +375,5 @@
>    }
> +
> +  bool IsEditable() const
> +  {
> +    return mEnabled == ENABLED || mEnabled == PASSWORD;

Unrelated to this bug but, it seems odd to me that we have a member variable named 'mEnabled' which contains an enum indicating 'enabled state' vs. a boolean flag. You might consider renaming this var to something more appropriate like 'mEnabledState'.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] from comment #2)
> Comment on attachment 8472108 [details] [diff] [review]
> Patch
> 
> Review of attachment 8472108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/nsIWidget.h
> @@ +375,5 @@
> >    }
> > +
> > +  bool IsEditable() const
> > +  {
> > +    return mEnabled == ENABLED || mEnabled == PASSWORD;
> 
> Unrelated to this bug but, it seems odd to me that we have a member variable
> named 'mEnabled' which contains an enum indicating 'enabled state' vs. a
> boolean flag. You might consider renaming this var to something more
> appropriate like 'mEnabledState'.

mEnabled means IME state (the struct name is IMEState). So, it's not indicate editor's state directly (but indirectly). Anyway, it should be another bug even if it's better to rename it. But I don't think so since IMEState::mEnabledState sounds redundant to me.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de467cc6208f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de467cc6208f

Comment 6

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> (In reply to Jim Mathies [:jimm] from comment #2)
> > Comment on attachment 8472108 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8472108 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/nsIWidget.h
> > @@ +375,5 @@
> > >    }
> > > +
> > > +  bool IsEditable() const
> > > +  {
> > > +    return mEnabled == ENABLED || mEnabled == PASSWORD;
> > 
> > Unrelated to this bug but, it seems odd to me that we have a member variable
> > named 'mEnabled' which contains an enum indicating 'enabled state' vs. a
> > boolean flag. You might consider renaming this var to something more
> > appropriate like 'mEnabledState'.
> 
> mEnabled means IME state (the struct name is IMEState). So, it's not
> indicate editor's state directly (but indirectly). Anyway, it should be
> another bug even if it's better to rename it. But I don't think so since
> IMEState::mEnabledState sounds redundant to me.

That's cool, it's your code. I just had a hard time wrapping my head around that var, it sounded like a bool to me. :)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] from comment #6)
> it sounded like a bool to me. :)

Yeah, it was a bool... "password" and "plugin" are appended later :-(