text entry field unable to take focus

RESOLVED WORKSFORME

Status

()

Core
Editor
P1
normal
RESOLVED WORKSFORME
10 years ago
7 years ago

People

(Reporter: u314746, Unassigned)

Tracking

({regression, testcase})

Trunk
mozilla1.9.2a1
x86
Windows XP
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 325911 [details]
page in question
(Reporter)

Comment 2

10 years ago
I now see where to attach the page in question and have done so.
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
Component: General → General
Ever confirmed: true
Keywords: qawanted, regression
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Flags: blocking1.9.1?
(Reporter)

Comment 4

10 years ago
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?
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.
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?
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.
Created attachment 326050 [details]
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).
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.
Blocks: 237964
Component: General → Editor
Keywords: qawanted → testcase
QA Contact: general → editor
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?
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.
Created attachment 326088 [details]
testcase2

This is probably bug 191994.
Martijn, if this is bug 191994, wouldn't it exist in Fx 2?
Sorry, I wasn't clear enough, perhaps. The "testcase2" testcase is what bug 191994 is about.
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
Created attachment 333127 [details] [diff] [review]
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)
Attachment #333127 - Flags: review?(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?
Attachment #333127 - Flags: superreview?(peterv)
Attachment #333127 - Flags: review?(jst)
Attachment #333127 - Flags: review+
Created attachment 333709 [details] [diff] [review]
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.
Attachment #333127 - Attachment is obsolete: true
Attachment #333709 - Flags: superreview?
Attachment #333127 - Flags: superreview?(peterv)
Attachment #333709 - Flags: superreview? → superreview?(peterv)
So we used to never unappply designmode.css ?  That should be testable by an automated test, right?
(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?)
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.
Created attachment 337263 [details] [diff] [review]
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.
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 on attachment 337263 [details] [diff] [review]
Patch v2

Awesome.  Thanks for putting that test together!
Attachment #337263 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 24

10 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

9 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 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.
Though I guess on branch 1.9.0.x we're stuck with that hack because we shouldn't change nsIHTMLDocument there.
Created attachment 341220 [details] [diff] [review]
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.
Attachment #337263 - Attachment is obsolete: true
Attachment #341220 - Flags: superreview?(peterv)
Attachment #337263 - Flags: superreview?(peterv)
(Reporter)

Comment 29

9 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.
(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 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+
Keywords: checkin-needed
Whiteboard: [c-n: waiting for comment 31, or not ?]
Created attachment 351551 [details] [diff] [review]
Patch v3.1 for checkin
[Backout: Comment 35]

Unbitrotted and comment 31 addressed.
Attachment #341220 - Attachment is obsolete: true
Whiteboard: [c-n: waiting for comment 31, or not ?]
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]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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]
(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...
Created attachment 361123 [details] [diff] [review]
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?).
Attachment #351551 - Attachment is obsolete: true
Attachment #361123 - Flags: review?(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.
Attachment #361123 - Flags: review?(bzbarsky) → review?(peterv)
Attachment #361123 - Flags: review?(peterv) → review-
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).
Created attachment 382426 [details] [diff] [review]
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.
Assignee: mike.kaplinskiy → nobody
The "Testcase" Att. is WFM against Firefox 5 and upwards => Issue fixed?
(Reporter)

Comment 42

7 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

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.