542317 - Caret missing when an iframe is put into design mode when it already has focus

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Ria Klaassen (not reading all bugmail)]

Str:

- go to http://thewestistheverybest.punt.nl/?id=500029&r=1&tbl_archief=&
- scroll down until you see a comment box
- left click in this box

Actual result: caret/cursor missing although I can type
Expected result: caret/cursor present in input field

This works fine with 3.5 but is broken in trunk and Firefox 3.6

Comment 1

[Matt]

Attachment: Reduced testcase

Reduced testcase for that page.

Comment 2

[Matt]

Regression range:

works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Minefield/3.6a1pre
2009-06-10-04-mozilla-central
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9

broken:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre
2009-06-11-04-mozilla-central
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad

Comment 3

[Ria Klaassen (not reading all bugmail)]

Thanks Matt, then this should be the query:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad

Comment 4

[Neil Deakin]

I don't see this bug. Reduced testcases would be nice. This may be duplicate of 534562 or 539323.

Comment 5

[Matt]

Attachment: Smaller reduced testcase

Smaller reduced testcase.

Comment 6

[Neil Deakin]

Attachment: super reduced testcase

Comment 7

[Neil Deakin]

Seems like the caret is not being made visible properly when design mode is enabled while the iframe is already focused. A simple hack to make the caret visible at the end of SetDesignMode works ok, but I don't know what the real fix is here.

I suspect bug 534562 may be the same issue.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

We'd consider a reviewed and baked patch.

Comment 9

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

bug 544277 will fix similar problem. but it doesn't care the focus() method of iframe.

Comment 10

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a5pre) Gecko/20100421 Minefield/3.7a5pre

Problem is still happening; bug 544277 hasn't fixed it.

Comment 11

[(no longer active)]

Attachment: Patch (v1)

The problem here is that nsHTMLEditor::HasFocus calls nsHTMLEditor::OurWindowHasFocus if the focused content is null, so if that function returns true, the contents of this if block are skipped <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#320>.  Initializing the editor selection (which causes the initialization of the caret) is inside this block.

The fix here is to return the focused content node (or the document element node if OurWindowHasFocus returns true when focused content is null).  I also renamed HasFocus to GetFocusedContent because it makes more sense that way.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 486791 [details] [diff] [review]
Patch (v1)

+  nsCOMPtr<nsIContent> focusedContent = dont_AddRef(GetFocusedContent());

Why these dont_AddRef calls?

Comment 13

[(no longer active)]

(In reply to comment #12)
> Comment on attachment 486791 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=486791
> Patch (v1)
> 
> +  nsCOMPtr<nsIContent> focusedContent = dont_AddRef(GetFocusedContent());
> 
> Why these dont_AddRef calls?

GetFocusedContent returns already_AddRefed<nsIContent>.  Without dont_AddRef, we'll be leaking one reference when calling GetFocusedContent.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No we won't, because assigning an already_AddRefed to an nsCOMPtr doesn't add a ref.

Comment 15

[(no longer active)]

Attachment: Patch (v2)

Oh, right.  Sorry, my bad.  This patch removes the dont_AddRef calls.

Comment 16

[(no longer active)]

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