440614 - text entry field unable to take focus

Status: RESOLVED WORKSFORME

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

Description

[u314746]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

I'm unable to make the text entry field take the focus via mouse click and enter text.  I can click in other fields and when I try to tab to this field, it doesn't take focus either and as such am unable post to this forum.

Reproducible: Always

Steps to Reproduce:
1.click on field to enter text
2.
3.
Actual Results:  
field doesn't take focus and can't enter text

Expected Results:  
expected field to take focus and allow text entry

This bug was present a few months ago in a FF 3 beta but wasn't entered.  

All extensions have been disabled without effect.  Tried with IE-Tab only, no change.  This website worked with FF 2.0X and works with IE

MTBR.com is a website that requires registration to post to the forum.  If you need access to the site, you would need to register or if you prefer I can zip up the page and send it.  Thought there was a place to attach on this page, but don't see it now.

Comment 1

[u314746]

Attachment: page in question

Comment 2

[u314746]

I now see where to attach the page in question and have done so.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Thanks for the attached file. I could not consistently reproduce the inability to enter text into the field in the latest builds, only in the older ones. But the cursor in any case is still absent so this is a regression from Firefox 2.

Regression range is:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1187110440&maxdate=1187117459
There are 4 bugs in it: Bug 391847, Bug 391221, Bug 374013 and Bug 391992.

Comment 4

[u314746]

The build I'm running is the one I downloaded on the day of the 3.0 release.  I didn't see this behavior of FF 2.0.0.14 and started up FF 2.0.0.14 in a VM to double check.  If I should test with a newer build, do you have a link which one I should try?

Comment 5

[Ria Klaassen (not reading all bugmail)]

Latest branch build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.9.0/
There can't be much difference between the latest branch build and the one you are using.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Odd.  None of the bugs in that range should affect it.

It looks like the editor is never becoming enabled, right?  Can someone try reducing the testcase?

Comment 7

[Ria Klaassen (not reading all bugmail)]

Sorry Boris, I retested it a few times before posting it but that range is not correct. I think the results depend on the ads. I'll retest it tomorrow.

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase

Ok, this is a minimal testcase, based from the page in question.
I guess this might started appearing when bug 237694 was fixed (contenteditable bug).

Comment 9

[Martijn Wargers (dead)]

Er, I meant bug 237964.
Indeed, this regressed between 2007-06-27 and 2007-06-28, which seems to indicate this is a regression from bug 237964.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Does taking out the .contentEditable = true in that last testcase make things better?  Or is it the write/close combination that is being a problem?

Comment 11

[Martijn Wargers (dead)]

Yes, taking out the contentEditable thing makes things ok.
Actually, the write/close combination seems to make things work, without it, it wouldn't work at all.

Comment 12

[Martijn Wargers (dead)]

Attachment: testcase2

This is probably bug 191994.

Comment 13

[Damon Sicore (:damons)]

Martijn, if this is bug 191994, wouldn't it exist in Fx 2?

Comment 14

[Martijn Wargers (dead)]

Sorry, I wasn't clear enough, perhaps. The "testcase2" testcase is what bug 191994 is about.

Comment 15

[Damon Sicore (:damons)]

Marking this as wanted 1.9.0.x+ and wanted1.9.1+ P1.  Don't think it would block 1.9.1.  Re-nom if you disagree.

Comment 16

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v1

This seems to be bug 284245 (presShell blown away), except now with contentEditable. Now to force nsHTMLDocument to reconstruct the editor, we need to do doc->SetEditingState(nsIHTMLDocument::eOff); . Also made sure that this is done when designmode isn't on (just contentEditable).

There's also a fix for a random bug that I found while debugging when the designmode css override wasn't removed.

You also don't actually need designmode to be on to reproduce this bug. (i.e. contentEditable in iframe doesn't really work that well when it is followed by something that blows away the presShell)

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 333127 [details] [diff] [review]
Patch v1

Looks ok to me, but someone who knows more about editor init/teardown should look too. Peterv?

Comment 18

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v1.1

Wow, I'm sorry I think modified that patch one too many times and forgot to do s/doc/nsdoc/ .

Here's one that actually compiles...Really sorry about that.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So we used to never unappply designmode.css ?  That should be testable by an automated test, right?

Comment 20

[Mike Kaplinskiy [:mikekap] (at school, no time)]

(In reply to comment #19)
> So we used to never unappply designmode.css ?  That should be testable by an
> automated test, right?
> 

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#217 ensures that any override stylesheets are removed before the editor goes away. 

That is, assuming ~nsHTMLEditor is (almost) right after nsHTMLEditor::PreDestroy. (Isn't that almost like crossing your fingers and hoping there's no dangling references?)

Comment 21

[Boris Zbarsky [:bzbarsky]]

It should be pretty easy to just hold a reference to the editor.  Certainly in chrome code.  So it should be quite possible to write a test that calls nsHTMLDocument::TearingDownEditor without ever calling ~nsHTMLEditor until the test is done.

Comment 22

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v2

Here's a patch with a testcase. It tests that stylesheets aren't removable for both contentEditable and designMode. I couldn't find a way to actually get some sort of reference to an override stylesheet in js, so the test just tries to remove them. Sorry it took so long, I'm back at college with no time to do anything.

(In reply to comment #21)
What I meant to say in comment #20 is that maybe some of the code in ~nsHTMLEditor should be moved to PreDestroy. Then again, I don't really know much about the editor.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 337263 [details] [diff] [review]
Patch v2

Awesome.  Thanks for putting that test together!

Comment 24

[u314746]

I updated today to 3.0.2 and tried this issue out, so figured I'd give you an update.  I can now edit in the box and at least type.  However, if I select the text and backspace, rather than eliminating the selected text, it only removes the last character.  If I select and type, it only adds a single character.  Ctrl-x/c, neither work.

Comment 25

[Will T Smith]

Just verifying that this bug exists with MTBR.com.  I got the bug after I upgraded to Firefox 3.  Initially, there is window focus and there is a cursor.  Then it seems like Javascript goes to work and by the time things are done, there is no cursor in the window.  You can type, but it places the typing ... wherever.

Comment 26

[Peter Van der Beken [:peterv]]

Comment on attachment 337263 [details] [diff] [review]
Patch v2

Hmm, how about we add a method ReinitEditor to nsIHTMLDocument that calls TurnEditingOff() and then EditingStateChanged(). I think I like that better than piling on to the hack in nsSubDocumentFrame::ShowDocShell.

Comment 27

[Peter Van der Beken [:peterv]]

Though I guess on branch 1.9.0.x we're stuck with that hack because we shouldn't change nsIHTMLDocument there.

Comment 28

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v3, changing nsIHTMLDocument

As per peter's suggestion, this adds a ReinitEditor method in nsIHTMLDocument.

For the 1.9.0.x branch, Patch v1.1 or v2 will do, depending whether you want the testcase.

Comment 29

[u314746]

It was posted about on the MTBR group about this issue and someone posted a workaround.  

http://forums.mtbr.com/showthread.php?t=441273

Before pushing the "A", you can add text but only remove one letter at a time.  After selecting it, you can delete more than one at a time and things, in my quick look, seemed to work ok.

Comment 30

[Mike Kaplinskiy [:mikekap] (at school, no time)]

(In reply to comment #29)
> It was posted about on the MTBR group about this issue and someone posted a
> workaround.  
> 
> http://forums.mtbr.com/showthread.php?t=441273
> 
> Before pushing the "A", you can add text but only remove one letter at a time. 
> After selecting it, you can delete more than one at a time and things, in my
> quick look, seemed to work ok.

If you want a quick workaround, you can paste the following into your location bar, after the page loads:

javascript:(function() { for (var i = 0; i < window.frames.length; i++) { var frameDoc = window.frames[i].document; if (!frameDoc) continue; var dm = frameDoc.designMode; var ce = frameDoc.body.contentEditable; frameDoc.designMode='off'; frameDoc.body.contentEditable='false'; frameDoc.designMode=dm; frameDoc.body.contentEditable=ce; } })();

I think this will fix the problem on (most) sites that run into this issue.

You can also add the above line as a bookmark and then just click on the bookmark when a page needs ``fixing''. Not a very good workaround, but it works.

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 341220 [details] [diff] [review]
Patch v3, changing nsIHTMLDocument

>+  /**
>+   * Re-inits the editor (if midas is on). 

I'd say something like 'editing must be on from contentEditable or designMode when calling this'.

Comment 32

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v3.1 for checkin [Backout: Comment 35]

Unbitrotted and comment 31 addressed.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 351551 [details] [diff] [review]
Patch v3.1 for checkin
[Backout: Comment 35]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

Patch backed out, since it regressed bug 439965.

I would bet that the problem is that IsEditingOn() is returning false in ShowDocShell (and can I _please_ ask for diffs with more context and -p ?).  You'll probably want to debug why that happens, but I suspect it has to do with the fact that you're still setting up the presentation inside the frame or something.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 351551 [details] [diff] [review]
Patch v3.1 for checkin
[Backout: Comment 35]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/488ed6c3dd5d
http://hg.mozilla.org/mozilla-central/rev/7f80a677ca91

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228498362.1228500257.27636.gz
Linux mozilla-central moz2-linux-slave07 dep unit test on 2008/12/05 09:32:42

The image reports an exception actually:
NS_ERROR_FAILURE ... nsIDOMNSHTMLDocument.execCommand
}

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to comment #35)
> The image reports an exception actually:

Image is here:  http://preview.tinyurl.com/5ckm7l

I'm also suspicious of this checkin for causing this composer test failures:
*** 38421 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/composer/test/test_bug389350.html | undefined
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228495584.1228498256.22059.gz

We'll see soon if Serge's backout fixes this...

Comment 37

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v4.0

Sorry for the fiasco before, seems I forgot to run reftests... As bz says, IsEditingOn does not actually return the editing state according to designMode/contentEditable, but rather it's the current state of the editor. This patch attempts to fix this.

The main functionality of EditingStateChanged moved to EnsureEditor. An optional parameter controls whether we force editor re-initialization (ie teardown before init). This change prevents certain things like the editing state changed event being fired whenever the editor is initialized. It also takes advantage of editor methods not complaining on duplicate calls. However, considering I don't know the editor system as well as I would like, please question anything. This passes all mochitests, reftests and crashtests, but if someone could do a tryserver build I would be grateful. Also, there seems to be two assertions that sometimes get printed that (I think) are related to this patch somehow:

###!!! ASSERTION: Inner window detected in Equality hook!: 'win->IsOuterWindow()', file /home/mike/Stuff/firefox/src/dom/src/base/nsDOMClassInfo.cpp, line 6641                                                                                         
###!!! ASSERTION: Inner window detected in Equality hook!: 'other->IsOuterWindow()', file /home/mike/Stuff/firefox/src/dom/src/base/nsDOMClassInfo.cpp, line 6647

sadly I have no idea what these mean, or what could be going wrong.

I also noticed that you cannot run midas commands when the frame is hidden. The HTML5 spec for editing does not specify anything regarding editor visibility, so I assumed that it may in some cases be useful to be able to do operations on a hidden editor (maybe double buffering or html generation?).

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 361123 [details] [diff] [review]
Patch v4.0

Peterv is a better reviewer than I on this.

Also, see the part about -p in comment 34?

As for the assertions, I'd love to see stacks to those in email or pastebin...  Those don't look good.

Comment 39

[Peter Van der Beken [:peterv]]

Comment on attachment 361123 [details] [diff] [review]
Patch v4.0

Please clean this up before asking for review. There's unnecessary whitespace changes, unnecessary moving of code, unnecessary changes in moved code and inconsistent formatting which all make this a bigger change than it should be and harder to review. There are also changes that look unrelated to this bug (eg I don't think RemoveOverrideStyleSheet failures have anything to do with this bug, and I don't think early returns are needed when they happen).

Comment 40

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Start of different approach

I no longer have time to work on this bug, and hence leaving the last patch that I never finished for anyone who wants to pick this up. I don't know if the tests pass on it, and if I remember correctly, you may need to do a flush before calling EditingStateChanged when changing designMode/contentEditable.

Comment 41

[(mostly gone) XtC4UaLL [:xtc4uall]]

The "Testcase" Att. is WFM against Firefox 5 and upwards => Issue fixed?

Comment 42

[u314746]

I've gone back to the original site and it appears to work on there.  So if it works on the constructed test case too (meaning no changes in the original site fixed the problem), looks fixed to me.