Closed
Bug 440614
Opened 16 years ago
Closed 13 years ago
text entry field unable to take focus
Categories
(Core :: DOM: Editor, defect, P1)
Tracking
()
RESOLVED
WORKSFORME
mozilla1.9.2a1
People
(Reporter: u314746, Unassigned)
References
()
Details
(Keywords: regression, testcase)
Attachments
(5 files, 5 obsolete files)
204.46 KB,
application/x-zip-compressed
|
Details | |
522 bytes,
text/html
|
Details | |
344 bytes,
text/html
|
Details | |
19.63 KB,
patch
|
peterv
:
review-
|
Details | Diff | Splinter Review |
9.57 KB,
patch
|
Details | Diff | Splinter Review |
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.
I now see where to attach the page in question and have done so.
Comment 3•16 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted,
regression
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Updated•16 years ago
|
Flags: blocking1.9.1?
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
This is probably bug 191994.
Comment 13•16 years ago
|
||
Martijn, if this is bug 191994, wouldn't it exist in Fx 2?
Comment 14•16 years ago
|
||
Sorry, I wasn't clear enough, perhaps. The "testcase2" testcase is what bug 191994 is about.
Comment 15•16 years ago
|
||
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.
Flags: wanted1.9.1+
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
Comment 16•16 years ago
|
||
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)
Attachment #333127 -
Flags: review?(jst)
Comment 17•16 years ago
|
||
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?
Attachment #333127 -
Flags: superreview?(peterv)
Attachment #333127 -
Flags: review?(jst)
Attachment #333127 -
Flags: review+
Comment 18•16 years ago
|
||
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.
Attachment #333127 -
Attachment is obsolete: true
Attachment #333709 -
Flags: superreview?
Attachment #333127 -
Flags: superreview?(peterv)
Updated•16 years ago
|
Attachment #333709 -
Flags: superreview? → superreview?(peterv)
Comment 19•16 years ago
|
||
So we used to never unappply designmode.css ? That should be testable by an automated test, right?
Comment 20•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee: nobody → mike.kaplinskiy
Attachment #333709 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337263 -
Flags: superreview?(peterv)
Attachment #337263 -
Flags: review?(bzbarsky)
Attachment #333709 -
Flags: superreview?(peterv)
Comment 23•16 years ago
|
||
Comment on attachment 337263 [details] [diff] [review] Patch v2 Awesome. Thanks for putting that test together!
Attachment #337263 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 24•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
Though I guess on branch 1.9.0.x we're stuck with that hack because we shouldn't change nsIHTMLDocument there.
Comment 28•16 years ago
|
||
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.
Attachment #337263 -
Attachment is obsolete: true
Attachment #341220 -
Flags: superreview?(peterv)
Attachment #337263 -
Flags: superreview?(peterv)
Reporter | ||
Comment 29•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
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'.
Attachment #341220 -
Flags: superreview?(peterv)
Attachment #341220 -
Flags: superreview+
Attachment #341220 -
Flags: review+
Updated•16 years ago
|
Keywords: checkin-needed
Updated•16 years ago
|
Whiteboard: [c-n: waiting for comment 31, or not ?]
Comment 32•16 years ago
|
||
Unbitrotted and comment 31 addressed.
Attachment #341220 -
Attachment is obsolete: true
Updated•16 years ago
|
Whiteboard: [c-n: waiting for comment 31, or not ?]
Comment 33•16 years ago
|
||
Comment on attachment 351551 [details] [diff] [review] Patch v3.1 for checkin [Backout: Comment 35] http://hg.mozilla.org/mozilla-central/rev/d98cdb0cdd15
Attachment #351551 -
Attachment description: Patch v3.1 for checkin → Patch v3.1 for checkin
[Checkin: Comment 33]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment 34•16 years ago
|
||
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.
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 35•16 years ago
|
||
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 }
Attachment #351551 -
Attachment description: Patch v3.1 for checkin
[Checkin: Comment 33] → Patch v3.1 for checkin
[Backout: Comment 35]
Comment 36•16 years ago
|
||
(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•15 years ago
|
||
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?).
Attachment #351551 -
Attachment is obsolete: true
Attachment #361123 -
Flags: review?(bzbarsky)
Comment 38•15 years ago
|
||
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.
Attachment #361123 -
Flags: review?(bzbarsky) → review?(peterv)
Updated•15 years ago
|
Attachment #361123 -
Flags: review?(peterv) → review-
Comment 39•15 years ago
|
||
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•15 years ago
|
||
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.
Updated•15 years ago
|
Assignee: mike.kaplinskiy → nobody
Comment 41•13 years ago
|
||
The "Testcase" Att. is WFM against Firefox 5 and upwards => Issue fixed?
Reporter | ||
Comment 42•13 years ago
|
||
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.
Updated•13 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 13 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•