552914 - nsEditor::mFlags is never modified by SetFlags()

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Attachment: Patch v1.0

nsEditor::mFlags is set only when nsEditor::Init() is called.

nsPlaintextEditor::SetFlags() and nsHTMLEditor::SetFlags() don't call nsEditor::SetFlags(), so, nsEditor::mFlags are not modified forever.

I guess that this makes a problem after type attribute or readonly attribute of <input> element is modified and some code path which refer nsEditor::mFlags run. However, I'm not sure there are actual problems.

My patch removes nsEditor::mFlags. It's not needed. And the code should check the flags via GetFlags() method.

My patch also removes a special code for unix password editor in ForceCompositionEnd() which has never needed right now because we support ime-mode CSS property. So, even if the editor is password field, nsIWidget::ResetInputState() should be called at that time.

Comment 1

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

Comment on attachment 433027 [details] [diff] [review]
Patch v1.0

smaug, can you review this? I requested to perterv, but there are no replies...

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 433027 [details] [diff] [review]
Patch v1.0


>+
>+void
>+nsEditor::OnFlagsChanged()
>+{
>   // Changing the flags can change whether spellchecking is on, so re-sync it
>   SyncRealTimeSpell();
>-  return NS_OK;
> }
I don't understand this method. Nothing seems to call it ever.


> 
> NS_IMETHODIMP
> nsEditor::GetIsDocumentEditable(PRBool *aIsDocumentEditable)
> {
>   NS_ENSURE_ARG_POINTER(aIsDocumentEditable);
>   nsCOMPtr<nsIDOMDocument> doc;
>   GetDocument(getter_AddRefs(doc));
>@@ -2177,21 +2187,16 @@ nsEditor::ForceCompositionEnd()
> // flag for Unix.
> // We should use nsILookAndFeel to resolve this
> 
> #if defined(XP_MAC) || defined(XP_MACOSX) || defined(XP_WIN) || defined(XP_OS2)
>   if(! mInIMEMode)
>     return NS_OK;
> #endif
> 
>-#ifdef XP_UNIX
>-  if(mFlags & nsIPlaintextEditor::eEditorPasswordMask)
>-	return NS_OK;
>-#endif
So you are, at least in theory, changing the behavior here.
Have you tested password fields with IME?


I think I would do this in different way.
Really start use nsEditor::mFlags always, and remove mFlags from nsTextEditRules.
nsTextEditRules can always access nsEditor::mFlags when needed.

Or is there some reason to not use nsEditor::mFlags, but nsTextEditRules::mFlags?

Comment 3

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

Attachment: Patch v2.0

> >-#ifdef XP_UNIX
> >-  if(mFlags & nsIPlaintextEditor::eEditorPasswordMask)
> >-	return NS_OK;
> >-#endif
> So you are, at least in theory, changing the behavior here.
> Have you tested password fields with IME?

yeah, but I removed the change from the patch. I'll post the patch in separated bug.

> I think I would do this in different way.
> Really start use nsEditor::mFlags always, and remove mFlags from
> nsTextEditRules.

I agree.

And I found a bug:

>  nsTextEditRules::DidInsertBreak(nsISelection *aSelection, nsresult aResult)
>  {
>    // we only need to execute the stuff below if we are a plaintext editor.
>    // html editors have a different mechanism for putting in mozBR's
>    // (because there are a bunch more places you have to worry about it in html) 
> -  if (!nsIPlaintextEditor::eEditorPlaintextMask & mFlags) return NS_OK;
> +  if (!IsPlaintextEditor()) {
> +    return NS_OK;
> +  }

The condition is always false, but the comment doesn't expect so, of course. I replaced all flag checking to the new accessor methods for suppressing the mistake.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 435856 [details] [diff] [review]
Patch v2.0


>+  PRBool IsAsyncUpdate() const
>+  {
>+    return (mFlags & nsIPlaintextEditor::eEditorUseAsyncUpdatesMask) != 0;
>+  }
Perhaps this could be called
PRBool UseAsyncUpdate() const

>+  PRBool IsNoCSS() const
>+  {
>+    return (mFlags & nsIPlaintextEditor::eEditorNoCSSMask) != 0;
>+  }
Maybe just
PRBool NoCSS() const

>+  PRBool IsDontEchoPassword() const
>+  {
>+    return (mFlags & nsIPlaintextEditor::eEditorDontEchoPassword) != 0;
>+  }
Maybe just 
PRBool DontEchoPassword() const
>--- a/editor/libeditor/base/nsEditorEventListener.cpp
>+++ b/editor/libeditor/base/nsEditorEventListener.cpp
>@@ -175,40 +175,30 @@ nsEditorEventListener::KeyPress(nsIDOMEv
> 
>   nsCOMPtr<nsIDOMKeyEvent>keyEvent = do_QueryInterface(aKeyEvent);
>   if (!keyEvent) 
>   {
>     //non-key event passed to keypress.  bad things.
>     return NS_OK;
>   }
> 
>-  // we should check a flag here to see if we should be using built-in key bindings
>-  // mEditor->GetFlags(&flags);
>-  // if (flags & ...)
>-
>   PRUint32 keyCode;
>   keyEvent->GetKeyCode(&keyCode);
> 
>   // if we are readonly or disabled, then do nothing.
>-  PRUint32 flags;
>-  if (NS_SUCCEEDED(mEditor->GetFlags(&flags)))
>+  nsEditor* editor = static_cast<nsEditor*>(mEditor);
I think you could make nsEditorListener::mEditor to be
type nsEditor. Currently it is nsIEditor. That way you wouldn't need the static_cast.
Though, it is possible that you'd need a static_cast then elsewhere. But try and see
what leads to easiest-to-read code.

>    nsCOMPtr<nsIEditorIMESupport> imeEditor = do_QueryInterface(mEditor, &result);
If mEditor was nsEditor, this QI wouldn't be needed.


>+  PRBool IsDontEchoPassword() const
>+  {
>+    return mEditor ? mEditor->IsDontEchoPassword() : PR_FALSE;
>+  }
DontEchoPassword()?

Comment 5

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

Attachment: Patch v3.0

> I think you could make nsEditorListener::mEditor to be
> type nsEditor. Currently it is nsIEditor. That way you wouldn't need the
> static_cast.
> Though, it is possible that you'd need a static_cast then elsewhere. But try
> and see
> what leads to easiest-to-read code.

O.K. But I'll post the patch in another new bug.

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/97aec61ae55f

I'll file a bug for the previous comment.