287707 - After page had designmode on, there are still several issues

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

- You can click on links, but the next page shows up blank.
- No focus outline for any of the elements that can receive focus.
- Cannot type in any textarea or text input

I've now turned off designmode with the url, but turning off designmode by
visiting another page also shows the same issue. In fact, every page you visit
hereafter shows this problem.
I've not narrowed the regression period, but I'm fairly certain this has started
to happen with the fix for bug 209020 (the situation before that was off course
much worse).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Nominating.  Things are better, but by no means ok (testing in an Apr 13 build
here).  jst, brade, any idea what's going on?

Comment 2

[Johnny Stenback (:jst)]

Attachment: Resoter link handler and remove loaded override stylesheet on HTML editor destruction.

This fixes *most* of the remaining problems, the only remaining thing that I
can see is changed selection behaviour, i.e. table cell celection, and form
control selection differences.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 182702 [details] [diff] [review]
Resoter link handler and remove loaded override stylesheet on HTML editor destruction.

>Index: editor/libeditor/html/nsHTMLEditor.cpp

>+nsHTMLEditor::Init(nsIDOMDocument *aDoc, nsIPresShell *aPresShell,
>+      context->SetLinkHandler(0);

how about nsnull?  ;)

Test to make sure the restoring code runs _before_ any destruction code on the
prescontext or doesn't run at all?  Prescontext holds a weak ref to the link
handler, so we don't want to set that pointer if the ref has already been
cleared by the link handler going away...

Comment 4

[Kathleen :Brade]

Comment on attachment 182702 [details] [diff] [review]
Resoter link handler and remove loaded override stylesheet on HTML editor destruction.

All editor code should use the following style for curly braces (please fix the
lines you touch):
  if (xyz)
  {
    abc();
  }

I'm sad to see webshell return.

In the while loop for removing override style sheets, is it possible to cause
an infinite loop if removal fails?  I think
nsHTMLEditor::RemoveOverrideStyleSheet() needs to be written to guarantee the
item is removed from the internal list.

nsHTMLEditor::RemoveStyleSheetFromList() should be rv &= NS_ERROR_FAILURE;
please fix that too.

(setting r- on this patch; r=brade on an updated patch with fixes)

Comment 5

[Johnny Stenback (:jst)]

Attachment: Safer change, includes brade's suggestions

Brade, how about this then? This does that, and some other random cleanup (and
fixes a transaction object leak in some error cases too).

Comment 6

[Kathleen :Brade]

Comment on attachment 183250 [details] [diff] [review]
Safer change, includes brade's suggestions

r=brade

This line seems odd to me:
+    rv |= RemoveStyleSheetFromList(aURL);


I'm surprised to see an nsresult or'd with another nsresult.  Wouldn't it be
better to just assign rv (the previous code threw away rv) or use another local
variable and conditionally return the appropriate rv?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183250 [details] [diff] [review]
Safer change, includes brade's suggestions

Nit from comment 3 still applies; sr=bzbarsky with that fixed.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 183250 [details] [diff] [review]
Safer change, includes brade's suggestions

a=me with nits fixed, in particular brade's point about |= is an issue if the
previous value really is a failure code -- bitwise or will make a mess.  At
least the result of | will also be a failure code such that NS_FAILED(rv) is
true, but shouldn't we be returning a known-good result?

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> true, but shouldn't we be returning a known-good result?

I mean a valid nsresult, even if a failure code (not a success code, sorry for
the "good" abusage there ;-).

/be

Comment 10

[Johnny Stenback (:jst)]

Attachment: Fix nits.

Comment 11

[Johnny Stenback (:jst)]

Fix landed. The remaining issues I've seen are focus problems (can't click in
text inputs etc) and different selection behaviour after midas has been turned
on (table cell selection instead of just table content selection...).

Comment 12

[Asa Dotzler [:asa]]

If there are issues that could be filed as new bugs, that might be good to do so
we can close this out and verify what's actually been fixed so far.

Comment 13

[Martijn Wargers (dead)]

The remaining issues in this bug that needs to be fixed (can't click in text
input's, no focus outline when focusing links, etc), you also get when you have
visited a page with designMode on, and then visiting another url.

Comment 14

[Adrian Yee]

After clicking on the test URL js, clicking on any links will kill my firefox:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050721 Firefox/1.0+

The spinner will go for about 2 seconds, then stop and firefox will be
non-responsive from then on (I have to kill it).

If I click on the url, and then visit a bookmark, it loads fine, but then it
also dies after clicking on a link on the page.

Comment 15

[Adrian Yee]

(In reply to comment #14)

Played with this a little more.  Try this:
1) Open a new blank window (see below for why)
2) Open a second blank window and view this bug
3) Click on the test URL link
4) Click on any link on the page (eg. Vote for this bug)

The page now opens in the first browser window you have open (the blank window
if you didn't have anything else open).

Now try this:
1) View this bug and close all other browser windows
2) Click on the test URL link
3) Click on any link on the page

If you're using a Windows build, you get a blank page.  If you're using a Linux
build, the browser locks up (like I experienced in my last comment).

Browsers tested with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050726 Firefox/1.0+
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8b4) Gecko/20050726
Firefox/1.0+

Comment 16

[Martijn Wargers (dead)]

Attachment: fix selection behavior

This fixes the different selection behavior for me (but that is just a minor issue, compared to the other ones).

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #16)
> Created an attachment (id=219352) [edit]
> fix selection behavior
> 
> This fixes the different selection behavior for me (but that is just a minor
> issue, compared to the other ones).

Did you mean to solicit review? 

Comment 18

[Martijn Wargers (dead)]

Attachment: fix selection and focus behavior

(In reply to comment #17)
> Did you mean to solicit review? 
Not with that patch, but this patch is probably worth the trouble.

This patch fixes the selection and focus issues for me. But I can still see the first issue as mentioned in comment 0 with this patch.

I think the MakeEditable function is broken. It sets mMakeEditable, which is used by GetEditable, but mMakeEditable is never set back to PR_FALSE. This patch does that.

Comment 19

[Martijn Wargers (dead)]

Actually, the first issue mentioned in comment 0 has changed somewhat.
Instead of openinge a blank page when clicking on a link, it now opens the link in a new window. This changed between 2006-04-26 and 2006-04-27, probably something that changed with bug 334515.

Comment 20

[Martijn Wargers (dead)]

Removing these 2 lines:
rv = listener->SetParentContentListener(this);
NS_ENSURE_SUCCESS(rv, rv);
http://lxr.mozilla.org/seamonkey/source/editor/composer/src/nsEditingSession.cpp#153

seems to fix that last issue.
Apparently, this is needed for Composer, to control where links from the sidebar should be opened in.
But that isn't really relevant for Firefox, since it doesn't have a Composer (and current trunk builds even don't have a sidebar).

Comment 21

[Martijn Wargers (dead)]

Comment on attachment 222113 [details] [diff] [review]
fix selection and focus behavior

Neil, maybe you are able to review this?

Comment 22

[[e-mail]]

Comment on attachment 222113 [details] [diff] [review]
fix selection and focus behavior

Sorry, I don't understand this code well enough. Perhaps it be acceptable to clear mMakeEditable from editorDocShell::SetEditor(nsnull) instead?

Comment 23

[Kathleen :Brade]

Comment on attachment 222113 [details] [diff] [review]
fix selection and focus behavior

Something about this patch doesn't seem right to me (will it break iframes or framesets or ???; maybe I've been away too long?)

I'd suggest Daniel Glazman and/or PeterV review.

Comment 24

[Martijn Wargers (dead)]

Attachment: Fix selection and focus behavior v2 (backed out)

Ok, the suggestion from Neil is working, thanks Neil!
This doesn't change as drastically as the previous patch, so is probably much safer.
(although the whole MakeEditable doesn't seem to me like a good thing to have, though).

Comment 25

[Martijn Wargers (dead)]

Attachment: fix link opening problem

This fixes the link opening problem. This is basically a backout of the patch from bug 52548.
I've checked whether it regresses that bug, but it seems like it doesn't. Neil, you have an idea about this?

Comment 26

[Darin Fisher]

-> martijn

Comment 27

[Darin Fisher]

*** Bug 322740 has been marked as a duplicate of this bug. ***

Comment 28

[[e-mail]]

(In reply to comment #25)
>I've checked whether it regresses that bug, but it seems like it doesn't.
I tried the patch and it regressed both tests from bug 52548 comment #56.

Comment 29

[Martijn Wargers (dead)]

Ok, thanks Neil. For some reason, I can't reproduce that at all :(

Comment 30

[Darin Fisher]

Webapps will still have to workaround existing problems in FF 1.0 and 1.5 when they use designMode, so not going to block FF 2 for this, but it would certainly be nice to get to a better world where webapps can rely on these bugs being fixed.  Patches definitely welcome for FF 2, but not going to block the release for this.

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 226510 [details] [diff] [review]
Fix selection and focus behavior v2 (backed out)

I don't understand the mess in nsDocShellEditorData, but assuming this fixes the bug: sr=peterv.

Comment 32

[Tracy Walker [:tracy]]

might bug 347257 be related to this bug?

Comment 33

[Martijn Wargers (dead)]

Attachment: testcase

Comment 34

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 226510 [details] [diff] [review]
Fix selection and focus behavior v2 (backed out)

r=[e-mail]

Comment 35

[Martijn Wargers (dead)]

Checking in docshell/base/nsDocShellEditorData.cpp;
/cvsroot/mozilla/docshell/base/nsDocShellEditorData.cpp,v  <--  nsDocShellEditor
Data.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in editor/libeditor/html/nsHTMLEditor.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,v  <--  nsHTMLEditor.cpp

new revision: 1.542; previous revision: 1.541
done

Checked the "Fix selection and focus behavior v2" patch in.
Leaving bug open for the link opening problem.

Comment 36

[Scott MacGregor]

Hi Martijin, it looks like this change broke mail compose editor shells. The editor instance is now always disabled. See Bug 351057 for more details.

Comment 37

[Martijn Wargers (dead)]

Comment on attachment 226510 [details] [diff] [review]
Fix selection and focus behavior v2 (backed out)

I've backed this patch out, because of bug 351057 that it is causing.

Comment 38

[Martijn Wargers (dead)]

Attachment: patch

Ok, this fixes all the remaining issues, and it doesn't regress bug 351057 nor does it seem to regress bug 348981.

Comment 39

[Kathleen :Brade]

Comment on attachment 242489 [details] [diff] [review]
patch

in nsHTMLDocument.cpp, why are you setting mEditingIsOn before MakeWindowEditable() is called?  I think it would be beneficial to have a comment in the code to explain that.

Have you tested these things:
 * opening a document in Composer
 * new document in Composer
 * editing frames
 * "midas" in browser
 * mail compose (both rich text and plain text and both reply and new message)

Comment 40

[Martijn Wargers (dead)]

Comment on attachment 242489 [details] [diff] [review]
patch

Ok, switching between source and then html mode isn't working right on http://heydon.com.au/htmlareademo with the patch.

Comment 41

[Martijn Wargers (dead)]

Attachment: patch

Ok, this fixes the focus and selection part of the bug, but not the link opening problem (that's the urlcontentlistener thing).
I tested that this doesn't regress bug 351057, nor does it cause anything else bad, afaics. I tested at least the things in comment 39 and comment 40.

I'm not really sure why this is working, but I noticed that creating an editor happens at a strange moment:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/editor.xml#12
It seems to me, the editor document should be made editable after the content has been loaded.

Comment 42

[Martijn Wargers (dead)]

Attachment: fixes link opening

Ok, I can reproduce the problem now from comment 28 (I had to install a some sidebar item in Seamonkey). This patch fixes it.
This moves the uricontentlistener stuff into editor.xml. I haven't fixed the toolkit part yet, because I expect there will be issues with the patch that Neil will find.

Comment 43

[[e-mail]]

Comment on attachment 243193 [details] [diff] [review]
fixes link opening

>   // register as a content listener, so that we can fend off URL
>   // loads from sidebar
I think this comment can go ;-)

>Index: xpfe/global/resources/content/bindings/editor.xml
Toolkit also has one, although I expect the diff will be similar.

>+          function nsEditorContentListener( contentWindow)
>+          {
>+            this.init();
>+          }
>+
>+          /* implements nsIURIContentListener */
>+
>+          nsEditorContentListener.prototype =
>+          {
>+            init: function()
>+            {
>+              var uriLoader = Components.classes["@mozilla.org/uriloader;1"]
>+                              .getService(Components.interfaces.nsIURILoader);
>+              uriLoader.registerContentListener(this);
>+            },
>+            close: function()
>+            {
>+              var uriLoader = Components.classes["@mozilla.org/uriloader;1"].getService(Components.interfaces.nsIURILoader);
>+              uriLoader.unRegisterContentListener(this);
>+            },
I don't think this bit is necessary.

>+            doContent: function(contentType, isContentPreferred, request, contentHandler)
>+            {
>+              return true;
>+            },
The old editing session code returns false here.

>+            convertWindowToDocShell: function(win) 
>+            {
>+              return null;
>+            },
Unused?

>+          this.editorContentListener = new nsEditorContentListener();
Rather than writing it like this you could just make it a field directly.

Comment 44

[Martijn Wargers (dead)]

Attachment: fixes link opening v2

I think I've addressed all review comments in comment 43 here.
Yes, I'll patch the toolkit version also, when I'm certain the xpfe version is correct (the toolkit version will be identical to the xpfe version anyway).

Comment 45

[[e-mail]]

Comment on attachment 243201 [details] [diff] [review]
fixes link opening v2

>+      <field name="editorContentListener">
>+        <![CDATA[
>+          function nsEditorContentListener( contentWindow)
>+          {
>+            this.init();
>+          }
>+
>+          /* implements nsIURIContentListener */
>+
>+          nsEditorContentListener.prototype =
>+          {
Actually that's not quite what I meant... I was thinking of
<field name="_editorContentListener">
  <![CDATA[
    ({
...
    })
  ]]>
</field>
i.e. an inline JavaScript object.

>+            init: function()
>+            {
>+              var uriLoader = Components.classes["@mozilla.org/uriloader;1"]
>+                              .getService(Components.interfaces.nsIURILoader);
>+              uriLoader.registerContentListener(this);
>+            },
Sorry, I still don't see why you added this.

Comment 46

[Martijn Wargers (dead)]

Attachment: fixes link opening v3

Comment 47

[Martijn Wargers (dead)]

Attachment: toolkit version

Toolkit part that also needs fixing.

Comment 48

[Johnny Stenback (:jst)]

Comment on attachment 243321 [details] [diff] [review]
fixes link opening v3

sr=jst

Comment 49

[Johnny Stenback (:jst)]

Comment on attachment 243408 [details] [diff] [review]
toolkit version

r+sr=jst

Comment 50

[Martijn Wargers (dead)]

Checking in editor/composer/src/nsEditingSession.h;
/cvsroot/mozilla/editor/composer/src/nsEditingSession.h,v  <--  nsEditingSession
.h
new revision: 1.19; previous revision: 1.18
done
Checking in editor/composer/src/nsEditingSession.cpp;
/cvsroot/mozilla/editor/composer/src/nsEditingSession.cpp,v  <--  nsEditingSessi
on.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in xpfe/global/resources/content/bindings/editor.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/editor.xml,v  <--  edito
r.xml
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/content/widgets/editor.xml;
/cvsroot/mozilla/toolkit/content/widgets/editor.xml,v  <--  editor.xml
new revision: 1.7; previous revision: 1.6
done

Checked in "fixes link opening v3" and "toolkit version" patches.

Comment 51

[Peter Van der Beken [:peterv]]

Comment on attachment 242986 [details] [diff] [review]
patch

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>===================================================================

>@@ -178,20 +178,28 @@ nsHTMLEditor::~nsHTMLEditor()

>+  nsCOMPtr<nsISelectionController> selCon;
>+  nsresult result = GetSelectionController(getter_AddRefs(selCon));
>+  if (NS_SUCCEEDED(result) && selCon)
>+  {
>+    selCon->SetSelectionFlags(
>+      nsISelectionDisplay::DISPLAY_TEXT | nsISelectionDisplay::DISPLAY_IMAGES);
>+  }

Wouldn't it make sense to store the value that the selection flags had before the editor changed them and restore those here?

Comment 52

[Peter Van der Beken [:peterv]]

Are there still issues remaining here?

Comment 53

[Martijn Wargers (dead)]

The focus issue seems fixed.
I can still see the selection issue, I think.
But this bug gets a bit confusing now, I think, so I've filed bug 386454 for the selection issue.
So this bug can be called fixed (for the focus issue).