488420 - IME enabled state is not modified when a focused editor's readonly attribute is changed

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

I found a bug at creating tests at bug 460059.

> data:text/html,<textarea id="ta"></textarea><script type="text/javascript">document.getElementById("ta").focus();document.getElementById("ta").setAttribute("readonly", "readonly");</script>

When input or textarea have focus and their readonly attribute is changed, the IME state is not modified by the new editable state. Because we only control IME state at focus changing (nsIMEStateManager (ISM)). So, we need to notify the editable state changing to ISM too.

However, I'm not sure which way is best way. I have following ideas now:

1. nsIContent::UpdateEditableState notifies ISM directly.
2. ISM should create a document observer at a content being focused, and it notifies the editable state changing to ISM.

Comment 1

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

Ah, the type attribute of input element changing has same bug too.

Comment 2

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

The bug of readonly attribute change is easy. However, type attribute isn't so because it recreates its frame but the frame isn't initialized as the focused element. E.g., The caret isn't visible but we can input text on input[type="text"]. I tried to fix it but I have not found the way. Besides, the code shouldn't be related to the readonly patch. So, I separate the bugs to each attribute.

Comment 3

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

Attachment: Patch v1.0

I filed bug 559728 for type attribute. This patch fixes only readonly attribute changes.

When the editor has focus and SetFlags() is called, nsEditor sets new IME state via nsIMEStateManager (ISM).

Comment 4

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

FYI: nsEditor::HasFocus() will be used on other bugs.

Comment 5

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

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

>+void
>+nsIMEStateManager::ChangeIMEStateTo(PRUint32 aNewIMEState)
>+{
>+  if (!sContent || !sPresContext) {
>+    NS_ERROR("ISM doesn't know which editor has focus");
>+    return;
>+  }
>+  if (aNewIMEState == 0) {
>+    NS_ERROR("aNewIMEState doesn't specify new state.");
>+    return;
>+  }
Are both these really cases where NS_ERROR is needed?
Since you anyway handle the problematic cases, I'd prefer using NS_WARNING.
Or if those cases are really something which shouldn't happen, 
remove the ifs, and just have NS_ASSERTION checks.

nsEditor::SetFlags handling is a bit ugly, but I don't know what could be a better way.

>+PRBool
>+nsEditor::HasFocus()
>+{
>+  nsCOMPtr<nsPIDOMEventTarget> piTarget = GetPIDOMEventTarget();
>+  if (!piTarget) {
>+    return PR_FALSE;
>+  }
>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ENSURE_TRUE(fm, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMElement> element;
>+  fm->GetFocusedElement(getter_AddRefs(element));
>+  return SameCOMIdentity(element, piTarget);
>+}
Have you tested this all with contenteditable?
Seems like nsHTMLEditor handles designMode, but contenteditable is a different thing.

r- until I know whether this is tested with contenteditable.

Comment 6

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

Attachment: Patch v1.1

(In reply to comment #5)
> (From update of attachment 439511 [details] [diff] [review])
> >+void
> >+nsIMEStateManager::ChangeIMEStateTo(PRUint32 aNewIMEState)
> >+{
> >+  if (!sContent || !sPresContext) {
> >+    NS_ERROR("ISM doesn't know which editor has focus");
> >+    return;
> >+  }
> >+  if (aNewIMEState == 0) {
> >+    NS_ERROR("aNewIMEState doesn't specify new state.");
> >+    return;
> >+  }
> Are both these really cases where NS_ERROR is needed?
> Since you anyway handle the problematic cases, I'd prefer using NS_WARNING.
> Or if those cases are really something which shouldn't happen, 
> remove the ifs, and just have NS_ASSERTION checks.

O.K. The former could happen and if it failed, that causes crash (only sPresContext is null). So, if (!sPresContext), we should output a warning.

And I guess that there is !sContent case in designMode. When designMode editor ha focus, sContent is null. So, we shouldn't check it. However, I'm not sure whether this is called from HTML editor when it's focused.

> nsEditor::SetFlags handling is a bit ugly, but I don't know what could be a
> better way.

Should I create |virtual void OnFlagsChanged()| and should SetFlags() call it?

> >+PRBool
> >+nsEditor::HasFocus()
> >+{
> >+  nsCOMPtr<nsPIDOMEventTarget> piTarget = GetPIDOMEventTarget();
> >+  if (!piTarget) {
> >+    return PR_FALSE;
> >+  }
> >+
> >+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> >+  NS_ENSURE_TRUE(fm, PR_FALSE);
> >+
> >+  nsCOMPtr<nsIDOMElement> element;
> >+  fm->GetFocusedElement(getter_AddRefs(element));
> >+  return SameCOMIdentity(element, piTarget);
> >+}
> Have you tested this all with contenteditable?
> Seems like nsHTMLEditor handles designMode, but contenteditable is a different
> thing.

Yes. When an ediable element has focus, SelectAll() selects all contents of the editable element. E.g.,

<p id="p1" contenteditable="true"></p>
<p id="p2" contenteditable="true"></p>

when p#p1 has focus, GetSelectionRoot() returns #p1, not body (or root) element. If neither editor doesn't have focus, it returns body or root element. So, we can use the comparison of the result of GetSelectionRoot() and the application wide focused element.

Comment 7

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

Are there contenteditable tests also for cases when the editable content
has many child elements and some of those is being edited?

Comment 8

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

(In reply to comment #7)
> Are there contenteditable tests also for cases when the editable content
> has many child elements and some of those is being edited?

When I insert table to a contenteditable element, that works fine.

When I insert a input field, the focus is in the input field rather than the HTML editor. At this time, it returns FALSE. So, it works fine.

When I insert button element to a contenteditable element, I can input some characters by keyboard to the last caret position in the HTML editor but HasFocus() returns FALSE. However, I think that this is bug 389372. When button element in contenteditable element has focus, the editor shouldn't insert any characters by key events.

So, I think that nsHTMLEditor::HasFocus() works fine.

Comment 9

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

But are there tests for contenteditable? I mean mochitests.

Comment 10

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

...since if there aren't, we really need some so that we can prevent
regressions.

Comment 11

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

http://mxr.mozilla.org/mozilla-central/search?string=contenteditable&find=test

looks like there are no overall tests of contenteditable...

Comment 12

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

Could you then please add few for this bug. Please.

Comment 13

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

Comment on attachment 439896 [details] [diff] [review]
Patch v1.1

I really really would like have some tests for contentEditable.
Clearing review? until there are at least some tests.

Comment 14

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

Sorry for the delay. I'm trying to create the tests. But there are some other bugs in both editor and tests... I need a couple of days to finish this bug.

Comment 15

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

Attachment: Patch v2.0

Sorry for the delay. This patch adds some tests for contenteditable.

Summaries of changes from the previous patch:

* nsHTMLEditor::GetPreferredIMEState() is implemented.

The patch sets IME state directly with nsIEditorIMESupports::GetPreferredIMEState(). But it returns wrong state when it's HTML editor. CSS ime-mode shouldn't be applied to HTML editor, but it's applied if we only implement nsEditor::GetPreferredIMEState().

* nsIContent::GetDesiredIMEState() uses nsIEditorIMESupports::GetPreferredIMEState() when the content is editable.

For the IME state consistency, content should use nsIEditorIMESupports::GetPreferredIMEState() of the editor of the document. The editor might be readonly, if so, IME state should be disabled.

* Adding 3 XXX comments to nsEditorEventListener.cpp.

When I tried to understand what are problem for the new tests, I found these strange points. When there is one or more contenteditable element, HTML editor is always listening all events, however, looks like the listener doesn't think that the events also come when the editor doesn't have focus. I think a major bug by this is bug 389372. I'll work on this.

* Changes nsHTMLEditor::HasFocus() logic for contenteditable.

The previous patch used selection root element which is found from current selection, but that has some problems. The new code is:

> +  // If there is no focused content, or the focused content isn't editable,
> +  // or it has independent selection, we don't have focus.
> +  if (!focusedContent || !focusedContent->HasFlag(NODE_IS_EDITABLE) ||
> +      IsIndependentSelectionContent(focusedContent)) {
> +    return PR_FALSE;
> +  }
> +  nsCOMPtr<nsIContent> rootContent = do_QueryInterface(GetRoot());
> +  if (!rootContent) {
> +    return PR_FALSE;
> +  }
> +  // If the focused content is in our document root, we're focused.
> +  return nsContentUtils::ContentIsDescendantOf(focusedContent, rootContent);

Just checking whether the focused node which it gets from nsFocusManager is:
1. editable or not
2. input field (or textarea) or not
3. descendant of the editor root element or not

* test_imestate.html has some bugs.

In runBasicTest(), the tests aren't work fine on current trunk. When the form controls parent is editable, moveFocus() doesn't move focus actually. And test() checks meaningless state. The new code checks whether the focus moves to each controls or not. And also before it moves focus, it moves focus to another control which sets different IME state than expected result.

runComplexContenteditableTests() changes HTML editor's flag directly, it's not realistic situation, but it's enough for the tests. And there are some todo, but they should be bugs of focus management, not editor. So, it should be fixed on another bug.

Comment 16

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

(In reply to comment #15)
> The patch sets IME state directly with
> nsIEditorIMESupports::GetPreferredIMEState(). But it returns wrong state when
> it's HTML editor.
So what do you mean here? You add some method which does something wrong?
Based on the comment in the patch, it returns something because of
compatibility with IE.

> And there are some todo,
> but they should be bugs of focus management, not editor. So, it should be fixed
> on another bug.
What bugs? Please file followup bugs for all the problematic cases.
And for all the focus handling related bugs, please CC Enn.

Comment 17

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

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

>+PRUint32
>+nsIContent::GetDesiredIMEState()
>+{
...
>+  PRUint32 state;
Could you initialize state with some reasonable value?
>+  nsresult rv = imeEditor->GetPreferredIMEState(&state);
>+  NS_ENSURE_SUCCESS(rv, IME_STATUS_DISABLE);
>+  return state;
>+}




>+  // Might be changing editable state, so, we need to reset current IME state
>+  // if we're focused.
>+  if (HasFocus()) {
>+    PRUint32 newState;
Also here, could you initialize newState
>+    rv = GetPreferredIMEState(&newState);
>+    if (NS_SUCCEEDED(rv)) {
>+      nsIMEStateManager::ChangeIMEStateTo(newState);
>+    }
>+  }
>+



>+PRBool
>+nsEditor::HasFocus()
>+{
>+  nsCOMPtr<nsPIDOMEventTarget> piTarget = GetPIDOMEventTarget();
>+  if (!piTarget) {
>+    return PR_FALSE;
>+  }
>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ENSURE_TRUE(fm, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMElement> element;
>+  fm->GetFocusedElement(getter_AddRefs(element));
>+  return SameCOMIdentity(element, piTarget);
Since this code is inside gklayout, you could use faster
nsFocusManager::GetFocusedContent()


>+PRBool
>+nsHTMLEditor::HasFocus()
>+{
>+  NS_ENSURE_TRUE(mDocWeak, PR_FALSE);
>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ENSURE_TRUE(fm, PR_FALSE);
>+
>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
GetFocusedContent()

>+PRBool
>+nsHTMLEditor::IsIndependentSelectionContent(nsIContent* aContent)
>+{
>+  NS_PRECONDITION(aContent, "aContent must not be null");
>+  nsIFrame* frame = aContent->GetPrimaryFrame();
>+  return (frame && (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION));
>+}
So this all doesn't break textformcontrols, like <input type="text"> or <textarea>?


>+  // XXX if there is a contenteditable element, HTML editor sets dom selection
>+  // to first editable node, but this makes inconsistency with normal document
>+  // behavior.
And what is the normal document behavior?
To set caret to the last node? IIRC there the behavior was changed for awhile, but
the patch caused too many regressions so it was backed out.


This all is rather regression-risky. And even the tests seems 
error-prone because they involve focus handling.
Could you please post the patch to tryserver before pushing to m-c.

Comment 18

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

(In reply to comment #16)
> (In reply to comment #15)
> > The patch sets IME state directly with
> > nsIEditorIMESupports::GetPreferredIMEState(). But it returns wrong state when
> > it's HTML editor.
> So what do you mean here? You add some method which does something wrong?
> Based on the comment in the patch, it returns something because of
> compatibility with IE.

IE only applies CSS ime-mode which specifies IME state of each element only to input[type=text] and textarea. So, IE doesn't applied it to HTML editor.

nsEditor::GetPreferredIMEState() computes the IME state with its root element's ime-mode. So, HTML editor also refers ime-mode value. However, HTML editor's nsEditor::GetPreferredIMEState() has never called because nsIContent::GetDesiredIMEState() uses only form control's editor's method.

But by this patch, the method is also used on HTML editor if the flag was changed when it's focused.

So, my patch doesn't change actual behavior, but it's needed change.

> > And there are some todo,
> > but they should be bugs of focus management, not editor. So, it should be fixed
> > on another bug.
> What bugs? Please file followup bugs for all the problematic cases.
> And for all the focus handling related bugs, please CC Enn.

See todo_is() in the tests. However, I rechecked it but it might be a test API bug, it might be caused by pending reflow. I'll check it.

(In reply to comment #17)
> >+PRBool
> >+nsHTMLEditor::IsIndependentSelectionContent(nsIContent* aContent)
> >+{
> >+  NS_PRECONDITION(aContent, "aContent must not be null");
> >+  nsIFrame* frame = aContent->GetPrimaryFrame();
> >+  return (frame && (frame->GetStateBits() & NS_FRAME_INDEPENDENT_SELECTION));
> >+}
> So this all doesn't break textformcontrols, like <input type="text"> or
> <textarea>?

The textform controls in contenteditable editors can have focus normally. I.e., users can focus to them and edit their value. At this time, the control's plaintext editor is used, not the HTML editor for the contenteditable. So, if this returns true with focused element, the HTML editor should do nothing.

> >+  // XXX if there is a contenteditable element, HTML editor sets dom selection
> >+  // to first editable node, but this makes inconsistency with normal document
> >+  // behavior.
> And what is the normal document behavior?
> To set caret to the last node? IIRC there the behavior was changed for awhile,
> but
> the patch caused too many regressions so it was backed out.

I think there is no selection range in the document until clicking or pressing some keys. But HTML editor creates a selection range to first editable node. If I don't run the creating selection range in BeginOfDocuemnt(), e.g., key event handler puts assertions.

I'm not sure what differences there are except the javascript behavior.

> This all is rather regression-risky. And even the tests seems 
> error-prone because they involve focus handling.
> Could you please post the patch to tryserver before pushing to m-c.

I request reviews only after I checked the regressions on tryserver ;-)

Comment 19

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

Attachment: Patch v2.1

Comment 20

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

http://hg.mozilla.org/mozilla-central/rev/cbb8d8caed8a

I'll file the follow up bugs for new todo_is() in the tests.