Closed Bug 544277 Opened 15 years ago Closed 15 years ago

IME became unusable when switching focus on Gmail RTF Editor

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a5
Tracking Flags:
Tracking Status
status1.9.2 --- wontfix
status1.9.1 --- unaffected

People

(Reporter: sst.dreams, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod, regression)

Attachments

(4 files, 9 obsolete files)

Testcase for possible problem
(deleted), text/html
Details
test changes
(deleted), patch
Details | Diff | Splinter Review
Patch v4.3
(deleted), patch
Details | Diff | Splinter Review
patch rev 1
(deleted), patch
enndeakin
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; zh-TW; rv:1.9.2) Gecko/20100115 Firefox/3.6 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; zh-TW; rv:1.9.2) Gecko/20100115 Firefox/3.6 When using mail editing interface on Gmail, switching focus on Gmail RTF Editor in a specific way will make IME unusable. Reproducible: Always Steps to Reproduce: 1. Go to Gmail. 2. Click "Compose Mail". 3. Use any IME to input some characters on Subject field. 4. Press Tab to switch focus to the content field. Actual Results: IME will become unusable. Expected Results: IME should still work. It is somehow like Bug 536058, but I believe they are different. In my opinion, The problem may be caused by switching between a input box and a iframe with contenteditable body. I had written a simple testcase about it.
Attached file Testcase for possible problem (deleted) —
Version: unspecified → 1.9.2 Branch
confirmed on trunk 20100204.
Status: UNCONFIRMED → NEW
Component: General → DOM: Events
Ever confirmed: true
QA Contact: general → events
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
There are two bugs: 1. When I clicked in the iframe, nsIMEStateManager didn't check the body's contenteditable flag. 2. When I moved the focus by tab key (as comment 0), nsIMEStateManager::OnChangeFocus() is called only one time with the parent document's presContext and no content. I'll post a patch ASAP.
Assignee: nobody → masayuki
Component: DOM: Events → Event Handling
is Bug 544198 a dupe of this issue?
I think we should fix this bug on 1.9.2 branch too. The patch shouldn't become risky.
Severity: normal → major
Status: NEW → ASSIGNED
status1.9.1: --- → unaffected
status1.9.2: --- → ?
Attached patch Patch v1.0 (obsolete) (deleted) — — Splinter Review
The testcase is great, it includes special case :-) Enn: I think that the focus manager should set focus to editor's root element when focus moves to a root element of an editable document. And also we should fire focus events when an editable root element gets focus.
Attachment #428694 - Flags: review?(enndeakin)
Comment on attachment 428694 [details] [diff] [review] Patch v1.0 > if (subWindow) { > contentToFocus = GetFocusedDescendant(subWindow, PR_TRUE, getter_AddRefs(newWindow)); >+ // If there are no focused content in the sub-window but the document root >+ // is ediable, we should set focus to it. >+ if (!contentToFocus) { >+ contentToFocus = GetRootEditableContent(newWindow); >+ } This doesn't make sense. If the element returned by GetRootEditableContent is what is really supposed to be focused, then it should have been the focused content to begin with.
Do you think that nsPIDOMWindow::GetFocusedNode() should return the editor's root element? If so, seems that I should change all callers of nsPIDOMWindow::SetFocusedNode()...
It would help me if you'd describe what the bug I should be seeing in the testcase is. Also, what element in the testcase is "the editor's root element"? While the <input> is focused, pressing Tab switches focus to the child frame's root element (the <html> element). Pressing Tab again switches focus to the <body> element. Are you expecting it to skip the <html> element when the body has contenteditable="true" and just focus the <body> directly?
(In reply to comment #10) > It would help me if you'd describe what the bug I should be seeing in the > testcase is. You can set focus to the HTML element, then, IME cannot be used because it's not editable (This cannot be confirm without IME). However, you can type ASCII characters at that time (the focused element isn't editable). Of course, at this time, you cannot see the caret. I think that this isn't intentional behavior of nsHTMLEditor. > Also, what element in the testcase is "the editor's root element"? The BODY element is. Normally, nsEditor holds BODY element. If there are no BODY element, e.g., it was removed by script, it gets and holds HTML element. > While the <input> is focused, pressing Tab switches focus to the child frame's > root element (the <html> element). Pressing Tab again switches focus to the > <body> element. Are you expecting it to skip the <html> element when the body > has contenteditable="true" and just focus the <body> directly? Yes. Is there a reason HTML element should get focus at the testcase? I have no idea.
> Yes. Is there a reason HTML element should get focus at the testcase? I have no > idea We always put the root element in the tab order. Is there a reason to behave differently when the body is editable? One thing to note is that just loading the following: <body> <p>Some text</p> <p contenteditable="true">Other text</p> </body> allows editing immediately even when nothing is clicked or focused (or if the root is focused). No caret appears here either, but typed characters still appear in the second paragraph. Thus, I don't think the <body> is being handled specially here. I would expect the root element to be focused first, where pressing cursor keys scroll the document. To edit, press tab to switch to the editable <p> element and typing edits instead.
This is same cause as bug 442808. (In reply to comment #12) > > Yes. Is there a reason HTML element should get focus at the testcase? I have no > > idea > > We always put the root element in the tab order. Is there a reason to behave > differently when the body is editable? Yes, I think so. The BODY element of HTML is a special element. It is used like root element. E.g., its background spreads to all of the view even if there are not enough contents. I guess that if web page authors set contentediable="true" on BODY element, they wanted all contents becoming editable. If so, there are no reasons we set focus to the HTML element. > One thing to note is that just loading the following: > > <body> > <p>Some text</p> > <p contenteditable="true">Other text</p> > </body> > > allows editing immediately even when nothing is clicked or focused (or if the > root is focused). No caret appears here either, but typed characters still > appear in the second paragraph. Thus, I don't think the <body> is being handled > specially here. Interesting. I think that this is a bug same as my previous comment. E.g., you cannot use "/" shortcut key for FAYT. In |nsHTMLEditRules::WillDoAction()|, http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#585 The selection range pointed the editable text node, however, the actual focus is in the root element immediately after loading the page. So, there is a conflict between focus and selection. I'm not sure whether such situation is valid or not. But even if it's valid, nsHTMLEditor should check focus in the window. If focused element in the window isn't editable, editor should pass the key events to the default handlers rather than they consume them. This was filed as bug 389372. > I would expect the root element to be focused first, where pressing cursor keys > scroll the document. To edit, press tab to switch to the editable <p> element > and typing edits instead. I still don't think it's true when the focus redirect from the iframe which contains the editable document.
Blocks: 442808
(In reply to comment #13) > > We always put the root element in the tab order. Is there a reason to behave > > differently when the body is editable? > > Yes, I think so. The BODY element of HTML is a special element. It is used like > root element. E.g., its background spreads to all of the view even if there are > not enough contents. I guess that if web page authors set contentediable="true" > on BODY element, they wanted all contents becoming editable. If so, there are > no reasons we set focus to the HTML element. If that's the case, you should be able to handle this entirely in GetRootForFocus. > I still don't think it's true when the focus redirect from the iframe which > contains the editable document. It should in the example I gave, or the document cannot otherwise be scrolled by the keyboard.
(In reply to comment #14) > (In reply to comment #13) > > I still don't think it's true when the focus redirect from the iframe which > > contains the editable document. > > It should in the example I gave, or the document cannot otherwise be scrolled > by the keyboard. I think when the HTML editor has focus, user should use PageUp/PageDown keys for scrolling.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > I still don't think it's true when the focus redirect from the iframe which > > > contains the editable document. > > > > It should in the example I gave, or the document cannot otherwise be scrolled > > by the keyboard. > > I think when the HTML editor has focus, user should use PageUp/PageDown keys > for scrolling. Yes, I don't think that scrolling in "editable documents" should happen like normal documents at all. It seems that Webkit also agrees with this, but I haven't tested IE yet. And by "editable documents", I mean those documents who have contenteditable=true on the body element. If the document looks like this: |<html><p contenteditable=true>foo</p></html>|, then scrolling should happen like normal.
And bug 442808 seems to suggest that IE has the same behavior that I described above.
(In reply to comment #16) > Yes, I don't think that scrolling in "editable documents" should happen like > normal documents at all. It seems that Webkit also agrees with this, but I > haven't tested IE yet. I checked the behavior of WebKit (Google Chrome) and IE8. When press tab key on the parent document: WebKit: setting focus to the editable body, showing caret at start of the body. IE8: same. When clicking on out of body element (i.e., clicking HTML element): WebKit: setting focus to the editable body, showing caret and the position is related to the clicked point. IE8: same. focus() method of iframe: WebKit: setting focus to the root element (HTML element), then, typing tab key moves the focus to the body and becoming editable. IE8:setting focus to the iframe? then, typing tab key the focus moves to the root element (HTML?), but it doesn't become editable. And typing tab key again, the editor looses focus. I think we need to fix the behavior of tabbing and clicking. focus() method isn't matter. > And by "editable documents", I mean those documents who have > contenteditable=true on the body element. If the document looks like this: > |<html><p contenteditable=true>foo</p></html>|, then scrolling should happen > like normal. Of course. The p element should behave like textarea.
Attached patch Patch v2.0 (obsolete) (deleted) — — Splinter Review
Attachment #428694 - Attachment is obsolete: true
Attachment #428694 - Flags: review?(enndeakin)
Attached patch Patch v2.0.1 (obsolete) (deleted) — — Splinter Review
Attachment #429054 - Attachment is obsolete: true
>+ nsIFrame* currFrame = mCurrentTarget; >+ >+ // When a root content which isn't editable but has an editable HTML >+ // <body> element is clicked, we should redirect the focus to the >+ // the <body> element. E.g., when an user click bottom of the editor >+ // where is outside of the <body> element, the <body> should be focused >+ // and the user can edit immediately after that. >+ // >+ // NOTE: The newFocus isn't editable that also means it's not in >+ // designMode. In designMode, all contents are not focusable. >+ if (newFocus && !newFocus->IsEditable()) { >+ nsIDocument *doc = newFocus->GetCurrentDoc(); >+ if (doc && newFocus == doc->GetRootContent()) { >+ nsIContent *bodyContent = >+ nsLayoutUtils::GetRootContentOfContentEditable(doc->GetWindow()); >+ if (bodyContent) { >+ nsIFrame* bodyFrame = bodyContent->GetPrimaryFrame(); >+ if (bodyFrame) { >+ currFrame = bodyFrame; >+ newFocus = bodyContent; >+ } >+ } >+ } >+ } >+ This change seems redundant with the change in nsFocusManager::CheckIfFocusable. The latter will redirect the focus to the body on mouse click as well, no? >- // don't fire events on the root content >- PRBool isRootContent = aContent && >- aContent->IsInDoc() && >- aContent == aContent->GetCurrentDoc()->GetRootContent(); >- if (!isRootContent) { >+ // don't fire events on the root content which isn't editable >+ PRBool noFocusEventNeeded = aContent && >+ aContent->IsInDoc() && >+ !aContent->IsEditable() && >+ aContent == aContent->GetCurrentDoc()->GetRootContent(); >+ if (!noFocusEventNeeded) { Why is this change needed? It looks to fire a focus event on an editable <html> element. But there's no equivalent change to fire blur on it when losing focus. >@@ -2728,17 +2738,26 @@ nsFocusManager::GetRootForFocus(nsPIDOMW ... >- nsIContent *rootContent = aDocument->GetRootContent(); >+ // If the document is editable by contenteditable attribute >+ // (i.e., not in designMode), we should set focus to the editor's root >+ // element which is the <body> element in most cases. >+ nsIContent *rootContent = >+ nsLayoutUtils::GetRootContentOfContentEditable(aWindow); If rootContent is a <body>, this will make the remainder of the functions (the root frame and frameset parts) check the wrong thing. >+nsLayoutUtils::GetRootContentOfContentEditable(nsPIDOMWindow* aWindow) ... >+ nsCOMPtr<nsIDOMElement> editorRoot; >+ rv = editor->GetRootElement(getter_AddRefs(editorRoot)); >+ NS_ENSURE_SUCCESS(rv, nsnull); >+ NS_ENSURE_TRUE(editorRoot, nsnull); >+ >+ nsCOMPtr<nsIContent> content = do_QueryInterface(editorRoot); >+ if (!content->IsEditable()) { Move the null-check for editorRoot into a !content check in this condition. >+ // Be aware, the root element of the editor might not be editable. >+ // E.g., data:text/html,<html><p contenteditable="true"></p></html> >+ return nsnull; >+ } >+ >+ nsIDocument *doc = content->GetCurrentDoc(); >+ // If the document is in designMode we should return NULL. >+ if (!doc || doc->HasFlag(NODE_IS_EDITABLE)) { >+ return nsnull; >+ } >+ >+ return content; >+} >+ If seems like the method GetRootContentOfContentEditable really only needs to check something like the following: if (rootElement->GetEditable()) return rootElement; else htmlDocument.body.GetEditable() return body; else return null; which would be a lot simpler and faster. In the following example, once the body is focused, Shift+Tab doesn't work to switch back. <body contenteditable="true">Cats</body> Possibly only do the redirection in GetRootForFocus when going forward, or only the one caller in GetNextTabbableContent should do the redirection. It would be good if the test checked to make sure the right events were being fired as well.
(In reply to comment #22) > This change seems redundant with the change in > nsFocusManager::CheckIfFocusable. The latter will > redirect the focus to the body on mouse click as well, no? No, in the next while block, the currFrame will be null, then, ESM doesn't call nsFocusManager::SetFocus(). > >- // don't fire events on the root content > >- PRBool isRootContent = aContent && > >- aContent->IsInDoc() && > >- aContent == aContent->GetCurrentDoc()->GetRootContent(); > >- if (!isRootContent) { > >+ // don't fire events on the root content which isn't editable > >+ PRBool noFocusEventNeeded = aContent && > >+ aContent->IsInDoc() && > >+ !aContent->IsEditable() && > >+ aContent == aContent->GetCurrentDoc()->GetRootContent(); > >+ if (!noFocusEventNeeded) { > > Why is this change needed? It looks to fire a focus event on an editable <html> > element. > But there's no equivalent change to fire blur on it when losing focus. Becasue nsIMEStateManager needs to know the focus event on the root element. And also nsEditorEventListener handles focus event, so, nsEditor may need the event. But I agree that I should change the Blur() too.
Attached patch Patch v3.0 (obsolete) (deleted) — — Splinter Review
> This change seems redundant with the change in > nsFocusManager::CheckIfFocusable. The latter will > redirect the focus to the body on mouse click as well, no? Sure. I removed the change in nsFocusManager::CheckIfFocusable(). There is a bug. When focus moves from designMode ditor to parent's non-editable root element, the IME state isn't updated because nsIMEStateManager cannot detect whether the called nsIMEStateManager::OnChangeFocus() with aContent is NULL for focus or blur. This is a bug of nsIMEStateManager, however, even if I changed it, there is still a bug in nsFocusManager::Focus(). It doesn't notify the focus change to nsIMEStateManager. That makes another bug. For these issues, I need to change nsFocusManager::Focus(). And I'll work for the better nsIMEStateManager in another bug because if I separate the nsIMEStateManager::OnChangeFocus() to nsIMEStateManager::OnFocus() and nsIMEStateManager::OnBlur(), I can optimize OnChangeFocus() implementation, but it's risky and needs more tests. So, I think this patch's change is enough and better for now.
Attachment #429055 - Attachment is obsolete: true
Attachment #431292 - Flags: review?(enndeakin)
Comment on attachment 431292 [details] [diff] [review] Patch v3.0 sorry for the spam, this has a regression.
Attachment #431292 - Flags: review?(enndeakin) → review-
Ugh... I found bug 406596. Seems that root element of a document which is in designMode shouldn't be fired focus/blur events. My patch breaks the behavior but any automated tests don't fail by the reason. Peter, is that right? (i.e., root element in designMode shouldn't get focus/blur events)
And I guess that the root element which has contenteditable="true" and is not in designMode, it should be fired the events. Is this right?
Attached patch Patch v4.0 (obsolete) (deleted) — — Splinter Review
This patch changes: * No focus/blur events are fired when root element of designMode document gets/loses focus. I guess that this is expected behavior for bug 406596. * focus/blur events are fired when root element which is editable by contenteditable attribute gets/loses focus. I guess that the all editable root elements by contenteditable needs focus/blur events because they are really focusable elements. * Sets focus to body element if the root element isn't edtiable but the body element is editable * Always notifies the focus change to nsIMEStateManager because when a non-editable root element gets focus, it cannot disable IME state if the previous focused node is in designMode. (nsIMEStateManager cannot disable IME when focus moves from designMode editor because aContent of the methods always NULL) * Needs to move focus forcedly in nsFocusManager::GetNextTabbableContent() when the root element is editable and searching started from the root element. Without this change, tabbing cannot move focus from html[contenteditable=true] to next node. E.g., Load the following URL: > data:text/html,<input><iframe src="data:text/html,<html contenteditable='true'></html>"></iframe><input> Set focus to the first <input> and press tab key several times, but you cannot move focus to the last <input>.
Attachment #431292 - Attachment is obsolete: true
Attachment #432356 - Flags: review?(enndeakin)
Comment on attachment 432356 [details] [diff] [review] Patch v4.0 Some comments so far. >+PRBool >+nsFocusManager::IsContentVisible(nsIContent* aContent) >+{ >+ NS_PRECONDITION(aContent, "aContent must not be null"); >+ nsIDocument* doc = aContent->GetCurrentDoc(); >+ if (!doc) { >+ return PR_FALSE; >+ } >+ nsIPresShell* presShell = doc->GetPrimaryShell(); >+ return (presShell && aContent->GetPrimaryFrame()); >+} This method isn't necessary. Frames are now stored directly in nsIContent so we don't need to check the document or preshell anymore. aContent->GetPrimaryFrame() is all that is necessary. >+PRBool >+nsFocusManager::IsFocusBlurEventNeeded(nsIContent* aContent) >+{ This would be better called IsNonEditableRoot. >+ // non editable root elemnt and root element in designMode don't need >+ // the events. >+ if (aContent == doc->GetRootContent()) { >+ return PR_FALSE; >+ } >+ >+ // others need the events. >+ return PR_TRUE; >+} All this can just be: return (aContent != doc->GetRootContent()); >- if (!aWindowRaised) >+ if (updateCommdands) { > aWindow->UpdateCommands(NS_LITERAL_STRING("focus")); >+ updateCommdands = PR_FALSE; >+ } > > SendFocusOrBlurEvent(NS_FOCUS_CONTENT, presShell, aContent->GetCurrentDoc(), > aContent, aFlags & FOCUSMETHOD_MASK, aWindowRaised); > > nsIMEStateManager::OnTextStateFocus(presContext, aContent); >+ notifyToIMS = PR_FALSE; >+ } else { >+ // XXX Don't we need to update commands if IME state is actually changed? >+ updateCommdands = PR_FALSE; > } > } >- else { >+ >+ if (notifyToIMS) { > nsPresContext* presContext = presShell->GetPresContext(); > nsIMEStateManager::OnTextStateBlur(presContext, nsnull); > nsIMEStateManager::OnChangeFocus(presContext, nsnull); >+ } > >- if (!aWindowRaised) >- aWindow->UpdateCommands(NS_LITERAL_STRING("focus")); >+ if (updateCommdands) { >+ aWindow->UpdateCommands(NS_LITERAL_STRING("focus")); The code would be easier to read if you just left the block structure intact and repeated the needed calls to nsIMEStateManager. > nsIContent *rootContent = aDocument->GetRootContent(); > if (rootContent) { >- if (aCheckVisibility) { >- nsIPresShell* presShell = aDocument->GetPrimaryShell(); >- if (!presShell || !rootContent->GetPrimaryFrame()) >- return nsnull; >+ if (aCheckVisibility && !IsContentVisible(rootContent)) { >+ return nsnull; Really, just if (aCheckVisibility && !rootContent->GetPrimaryFrame()) { >+/* static */ >+nsIContent* >+nsLayoutUtils::GetEditableRootContentByContentEditable(nsIDocument* aDocument) ... >+ // contenteditable only works with HTML document. >+ // Note: Use nsIDOM*Document rather than nsI*Document because their GetBody() >+ // and GetDocumentElement() do something additional work for some >+ // cases and nsEditor uses them. >+ nsCOMPtr<nsIDOMHTMLDocument> domHTMLDoc = do_QueryInterface(aDocument); >+ if (!domHTMLDoc) { >+ return nsnull; >+ } >+ >+ nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(aDocument); >+ NS_ENSURE_TRUE(domDoc, nsnull); >+ >+ nsCOMPtr<nsIDOMElement> root; >+ nsresult rv = domDoc->GetDocumentElement(getter_AddRefs(root)); >+ nsCOMPtr<nsIContent> content = do_QueryInterface(root); >+ if (NS_SUCCEEDED(rv) && content && content->IsEditable()) { >+ return content; aDocument->GetRootContent() will get you the root content instead of doing all this. More review to come...
> This would be better called IsNonEditableRoot. I meant to say IsNonFocusableRoot is a better name.
(In reply to comment #28) > * No focus/blur events are fired when root element of designMode document > gets/loses focus. I guess that this is expected behavior for bug 406596. this should break accessibility. Whenever the focus is changed we need be notified. > * focus/blur events are fired when root element which is editable by > contenteditable attribute gets/loses focus. I guess that the all editable root > elements by contenteditable needs focus/blur events because they are really > focusable elements. > * Sets focus to body element if the root element isn't edtiable but the body > element is editable Now accessibility doesn't support focus event on root element or body element but we should fix this in bug 550338.
Btw, will we get two focus events (one is for document, another one is for focused element) with this patch when the window is focused first time, for example, when button inside an unfocused document is clicked?
The root element never receives a focus event targeted at it. Having the root element focused should be treated as equivalent to having nothing in the document focused. The only distinction is that the dotted outline appears around the document. You'll already need to handle nothing in the document being focused; you should receive the same sequence of events when the root is focused.
(In reply to comment #33) > The root element never receives a focus event targeted at it. What about nsIFocusManager? > Having the root > element focused should be treated as equivalent to having nothing in the > document focused. The only distinction is that the dotted outline appears > around the document. at odd moments perhaps "nothing" is not good term because the nothing is in navigation order what may confuse.
Attached patch Patch v4.1 (obsolete) (deleted) — — Splinter Review
Attachment #432356 - Attachment is obsolete: true
Attachment #432742 - Flags: review?(enndeakin)
Attachment #432356 - Flags: review?(enndeakin)
Enn? Would you review the latest patch?
Attached patch Patch v4.1.1 (obsolete) (deleted) — — Splinter Review
updating for latest trunk...
Attachment #432742 - Attachment is obsolete: true
Attachment #439145 - Flags: review?(enndeakin)
Attachment #432742 - Flags: review?(enndeakin)
Comment on attachment 439145 [details] [diff] [review] Patch v4.1.1 > diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp > --- a/dom/base/nsFocusManager.cpp > +++ b/dom/base/nsFocusManager.cpp > @@ -1268,16 +1268,44 @@ nsFocusManager::IsWindowVisible(nsPIDOMW ... > +PRBool > +nsFocusManager::IsNonFocusableRoot(nsIContent* aContent) > +{ > + if (!aContent) { > + return PR_FALSE; > + } Both callers already check this. > + nsIDocument* doc = aContent->GetCurrentDoc(); > + if (!doc) { > + return PR_FALSE; > + } > + > + // If aContent is in designMode, the root element is not focusable. > + // NOTE: in designMode, most elements are not focusable, just the document is > + // focusable. > + if (doc->HasFlag(NODE_IS_EDITABLE)) { > + return aContent == doc->GetRootContent(); > + } > + > + // If aContent is editable without designMode, it's focusable. > + if (aContent->IsEditable()) { > + return PR_FALSE; > + } > + > + // Otherwise, the root element must not be focusable. > + return aContent == doc->GetRootContent(); > +} > + All this could just be: if (doc && (doc->HasFlag(NODE_IS_EDITABLE) || !aContent->IsEditable())) return aContent == doc->GetRootContent(); return PR_FALSE; > @@ -1391,19 +1419,21 @@ nsFocusManager::Blur(nsPIDOMWindow* aWin ... > - PRBool isRootContent = content && content == content->GetCurrentDoc()->GetRootContent(); > + // Don't fire blur event on the root content which isn't editable. > + PRBool sendBlurEvent = > + content && content->IsInDoc() && !IsNonFocusableRoot(content); IsNonFocusableRoot already checks if content has a document. Maybe this could be combined somehow? > @@ -1623,16 +1651,21 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi > nsIMEStateManager::OnTextStateFocus(presContext, aContent); > + } else { > + nsPresContext* presContext = presShell->GetPresContext(); > + nsIMEStateManager::OnTextStateBlur(presContext, nsnull); > + nsIMEStateManager::OnChangeFocus(presContext, nsnull); > + // XXX if IME state is changed, don't we need to call UpdateCommands? Seems like UpdateCommands should be called, but only if aIsNewDocument is true. > - else if (!forward && startContent == rootContent) { > - doNavigation = PR_FALSE; > + else if (!forward) { > + // If focus moves to backward and when current focused node is root Remove 'to' in the comment > + * data:text/html,<html contenteditable="true"><body></body></html> > + * returns the <html>. > + * > + * data:text/html,<html><body contenteditable="true"></body></html> > + * data:text/html,<body contenteditable="true"></body> > + * With these cases, this returns the <body>. The parser will never create the latter though, and <html> is always the root so I wouldn't mention both here. > + * > + * data:text/html,<body><p contenteditable="true"></p></body> > + * returns NULL because <body> isn't editable. > + */ Remove the 'data:text/html,' part from these examples. > + function testTabKey(aForward, > + aNextFocusedNode, aWillFireFocusEvent, > + aNextBlurredNode, aWillFireBlurEvent, > + aIMEShouldBeEnabled, aTestingCaseDescription) > + { > + observeFocusBlur(aNextFocusedNode, aWillFireFocusEvent, > + aNextBlurredNode, aWillFireBlurEvent); > + synthesizeKey("VK_TAB", { shiftKey: !aForward }); > + var description = "Tab key test: "; > + if (!aForward) { > + description = "Shift-" + description; > + } > + description += aTestingCaseDescription + ": "; > + is(fm.focusedElement, aNextFocusedNode, > + description + "wasn't moved focus expectedly"); Change to: didn't move focus correctly Same in the other three places this is used. > + testMouseClick(editor, true, false, prev, true, true, "iframe_html"); > + resetFocusToInput("after click test for iframe_html"); > + > + iframe.style.display = "none"; > + > + // designMode should behave like <html contenteditable="true"></html> > + // but focus/blur events shouldn't be fired on its root element because > + // any elements shouldn't be focused state in designMode. > + iframe = document.getElementById("iframe_designMode"); > + iframe.style.display = "inline"; > + iframe.contentDocument.designMode = "on"; > + editor = iframe.contentDocument.getElementById("editor"); > + root = iframe.contentDocument.documentElement; > + resetFocusToInput("initializing for iframe_designMode"); Did the call to resetFocusToInput above already handle everything that needed to be? What does this second call do? > + testMouseClick(editor, false, true, prev, true, true, "iframe_designMode"); > + resetFocusToInput("after click test for iframe_designMode"); > + > + iframe.style.display = "none"; > + > + // When there is no HTML element but the BODY element is editable, > + // the body element should get focus and enables IME. > + iframe = document.getElementById("iframe_body"); > + iframe.style.display = "inline"; > + editor = iframe.contentDocument.getElementById("editor"); > + root = iframe.contentDocument.documentElement; > + resetFocusToInput("initializing for iframe_body"); Similar here. > + resetFocusToInput("initializing for iframe_p"); and here. Did you test document navigation for the design mode / editable cases? (F6 / Shift+F6) Sorry for the delay. I must have reviewed 80% of this 4 times before getting interrupted each time.
>> @@ -1623,16 +1651,21 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi >> nsIMEStateManager::OnTextStateFocus(presContext, aContent); >> + } else { >> + nsPresContext* presContext = presShell->GetPresContext(); >> + nsIMEStateManager::OnTextStateBlur(presContext, nsnull); >> + nsIMEStateManager::OnChangeFocus(presContext, nsnull); >> + // XXX if IME state is changed, don't we need to call UpdateCommands? > > Seems like UpdateCommands should be called, but only if aIsNewDocument is true. If I changed so, test_focus.xul failed by "tab key t13 events - got "commandupdate: cu blur: t12 blur: outer-document blur: outer-window focus: child-document focus: child-window commandupdate: cu", expected "commandupdate: cu blur: t12 blur: outer-document: blur: outer-window focus: child-document focus: child-window" Is it correct? I'm not sure what commandupdate does. > Did you test document navigation for the design mode / editable cases? (F6 / > Shift+F6) Looks most behavior are same as current trunk build except that when I load "data:text/html,<html contenteditable="true"><body></body></html>" and press F6 when focus is in location bar, focus is moved to the <html> and caret is visible (on trunk, focus is moved but caret is invisible). I tested designMode, html[contenteditable=true], body[contenteditable=true], iframe has designMode document, iframe has html[contenteditable=true], iframe has body[contenteditable=true]. (F6/Shift+F6 don't move focus to iframe)
(In reply to comment #39) > Is it correct? I'm not sure what commandupdate does. Yes. The commandupdate event updates the enabled/disabled state of the menus and similar ui. For example, load this page: <html><body onload="document.designMode = 'on';">This is some text.</body></html> Tabbing into this document results in focus events, but the Paste menuitem isn't enabled, yet it should be. If you prefer, if you supply an updated patch, I can update and check any changes needed to the test. > when focus is in location bar, focus is moved to the <html> and caret is > visible (on trunk, focus is moved but caret is invisible). So that sounds like it's working better than before.
Attached patch Patch v4.2 (obsolete) (deleted) — — Splinter Review
> If you prefer, if you supply an updated patch, I can update and check any changes needed to the test. thank you.
Attachment #439145 - Attachment is obsolete: true
Attachment #439145 - Flags: review?(enndeakin)
Comment on attachment 440157 [details] [diff] [review] Patch v4.2 >+ // If aContent is in designMode, the root element is not focusable. >+ // NOTE: in designMode, most elements are not focusable, just the document is >+ // focusable. >+ // Besides, if aContent is not editable but it isn't in designMode, it's not >+ // focusable. 'Besides' -> 'Also' nsIMEStateManager::OnChangeFocus(presContext, nsnull); >+ if (aIsNewDocument) { >+ aWindow->UpdateCommands(NS_LITERAL_STRING("focus")); >+ } OK, I'm not sure what I was talking about earlier. The condition should be the same check as the other cases: if (!aWindowRaised) aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
Attachment #440157 - Flags: review+
Attached patch test changes (deleted) — — Splinter Review
I only tested the patch on Mac.
Attached patch Patch v4.3 (obsolete) (deleted) — — Splinter Review
Thank you, Enn.
Attachment #440157 - Attachment is obsolete: true
Attached patch Patch v4.3 (deleted) — — Splinter Review
Attachment #440478 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/b5971d748a22 http://hg.mozilla.org/mozilla-central/rev/371a51a9724e http://hg.mozilla.org/mozilla-central/rev/e7e7a4fe417c This can be a major bug for Gmail users in CJK, however, the patch is pretty risky for branch. So, I don't think the patch should be landed on branch.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
status1.9.2: ? → wontfix
Keywords: regression
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attached patch patch rev 1 (deleted) — — Splinter Review
This is broken if you are building with MOZ_MEDIA disabled since nsIDOMHTMLElement.h is only included through nsHTMLVideoElement.h. This patch fixes it.
Attachment #440658 - Flags: review?(enndeakin)
Attachment #440658 - Flags: review?(enndeakin) → review+
(In reply to comment #47) > Created an attachment (id=440658) [details] > patch rev 1 > > This is broken if you are building with MOZ_MEDIA disabled since > nsIDOMHTMLElement.h is only included through nsHTMLVideoElement.h. This patch > fixes it. Pushed as http://hg.mozilla.org/mozilla-central/rev/06122cd5fb03
Thank you for creating and landing the patch.
Blocks: 543077
Blocks: 541042
No longer blocks: 541042
If the fix will not be in a branch, when can Firefox users expect to see this fix in a release build? I looked at 4.0 Beta 1 and the problem does not occur there. It is a major problem for Gmail users and we have seen a bunch of user reports about this from Firefox users in Japan. If we need to wait for the next release, when will that be coming? Info on this will be highly appreciated. Thanks!
See bug 543077, a simpler patch fixes the problem on 1.9.2.7. > It is a major problem for Gmail users and we have seen a bunch of user > reports about this from Firefox users in Japan. I don't think so, I think that most Japanese people don't use HTML mail and most HTML mail users in Japan don't use keyboard navigation. I watched this report only once in Japanese user community. Therefore, I think this isn't major in Japan. But I'm not sure for other countries including Europe (Mac users use IME for inputting diacriticals).
Depends on: 574353
No longer depends on: 574353
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: