Closed Bug 287707 Opened 19 years ago Closed 17 years ago

After page had designmode on, there are still several issues

Categories

(Core :: DOM: Editor, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

()

Details

(Keywords: technote, testcase)

Attachments

(5 files, 9 obsolete files)

10.94 KB, patch
Brade
: review+
bzbarsky
: superreview+
brendan
: approval1.8b3+
Details | Diff | Splinter Review
10.51 KB, patch
Details | Diff | Splinter Review
1.12 KB, text/html
Details
8.52 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.49 KB, patch
neil
: review+
Details | Diff | Splinter Review
- 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).
Nominating.  Things are better, but by no means ok (testing in an Apr 13 build
here).  jst, brade, any idea what's going on?
Flags: blocking1.8b2?
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.
Attachment #182702 - Flags: superreview?(bzbarsky)
Attachment #182702 - Flags: review?(brade)
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...
Attachment #182702 - Flags: superreview?(bzbarsky) → superreview+
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)
Attachment #182702 - Flags: superreview?(bzbarsky)
Attachment #182702 - Flags: superreview+
Attachment #182702 - Flags: review?(brade)
Attachment #182702 - Flags: review-
Attachment #182702 - Flags: superreview?(bzbarsky)
Brade, how about this then? This does that, and some other random cleanup (and
fixes a transaction object leak in some error cases too).
Attachment #183250 - Flags: superreview?(bzbarsky)
Attachment #183250 - Flags: review?(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?
Attachment #183250 - Flags: review?(brade) → review+
Comment on attachment 183250 [details] [diff] [review]
Safer change, includes brade's suggestions

Nit from comment 3 still applies; sr=bzbarsky with that fixed.
Attachment #183250 - Flags: superreview?(bzbarsky) → superreview+
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Attachment #183250 - Flags: approval1.8b3?
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
Attachment #183250 - Flags: approval1.8b3? → approval1.8b3+
(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
Attached patch Fix nits.Splinter Review
Attachment #182702 - Attachment is obsolete: true
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...).
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.
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
QA Contact: bugzilla → nobody
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.
Flags: blocking1.8b4? → blocking1.8b4-
Keywords: technote
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.
(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+
Blocks: 322740
Blocks: 333869
Attached patch fix selection behavior (obsolete) — Splinter Review
This fixes the different selection behavior for me (but that is just a minor issue, compared to the other ones).
Flags: blocking1.9a2?
(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? 

Attached patch fix selection and focus behavior (obsolete) — Splinter Review
(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.
Attachment #219352 - Attachment is obsolete: true
Attachment #222113 - Flags: review?(brade)
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.
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).
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 312930
Comment on attachment 222113 [details] [diff] [review]
fix selection and focus behavior

Neil, maybe you are able to review this?
Attachment #222113 - Flags: review?(brade) → review?(neil)
No longer blocks: 312930
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?
Attachment #222113 - Flags: review?(neil)
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.
Attachment #222113 - Flags: superreview?(peterv)
Attachment #222113 - Flags: review?(daniel)
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).
Attachment #222113 - Attachment is obsolete: true
Attachment #226510 - Flags: superreview?(peterv)
Attachment #226510 - Flags: review?(daniel)
Attachment #222113 - Flags: superreview?(peterv)
Attachment #222113 - Flags: review?(daniel)
Attached patch fix link opening problem (obsolete) — Splinter Review
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?
Attachment #226610 - Attachment is patch: true
Attachment #226610 - Attachment mime type: application/octet-stream → text/plain
-> martijn
Assignee: mozeditor → martijn.martijn
Flags: blocking1.8.1+
*** Bug 322740 has been marked as a duplicate of this bug. ***
(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.
Ok, thanks Neil. For some reason, I can't reproduce that at all :(
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.
Flags: blocking1.8.1+ → blocking1.8.1-
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.
Attachment #226510 - Flags: superreview?(peterv) → superreview+
Blocks: mobb-4
might bug 347257 be related to this bug?
Blocks: 347257
Flags: blocking1.9a2? → blocking1.9+
Attached file testcase
Comment on attachment 226510 [details] [diff] [review]
Fix selection and focus behavior v2 (backed out)

r=daniel@glazman.org
Attachment #226510 - Flags: review?(daniel) → review+
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.
Depends on: 351057
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 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.
Attachment #226510 - Attachment description: Fix selection and focus behavior v2 → Fix selection and focus behavior v2 (backed out)
Attachment #226510 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Ok, this fixes all the remaining issues, and it doesn't regress bug 351057 nor does it seem to regress bug 348981.
Attachment #226610 - Attachment is obsolete: true
Attachment #242489 - Flags: superreview?(peterv)
Attachment #242489 - Flags: review?(daniel)
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 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.
Attachment #242489 - Attachment is obsolete: true
Attachment #242489 - Flags: superreview?(peterv)
Attachment #242489 - Flags: review?(daniel)
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #242986 - Flags: superreview?(peterv)
Attachment #242986 - Flags: review?(daniel)
Attached patch fixes link opening (obsolete) — Splinter Review
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.
Attachment #243193 - Flags: review?(neil)
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.
Attached patch fixes link opening v2 (obsolete) — Splinter Review
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).
Attachment #243201 - Flags: review?(neil)
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.
Attachment #243193 - Attachment is obsolete: true
Attachment #243201 - Attachment is obsolete: true
Attachment #243321 - Flags: review?(neil)
Attachment #243193 - Flags: review?(neil)
Attachment #243201 - Flags: review?(neil)
Attachment #243321 - Flags: review?(neil) → review+
Attached patch toolkit versionSplinter Review
Toolkit part that also needs fixing.
Attachment #243408 - Flags: superreview?(jst)
Attachment #243408 - Flags: review?(neil)
Attachment #243321 - Flags: superreview?(jst)
Attachment #243408 - Flags: review?(neil) → review+
Comment on attachment 243321 [details] [diff] [review]
fixes link opening v3

sr=jst
Attachment #243321 - Flags: superreview?(jst) → superreview+
Comment on attachment 243408 [details] [diff] [review]
toolkit version

r+sr=jst
Attachment #243408 - Flags: superreview?(jst) → superreview+
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.
Flags: in-testsuite?
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?
Attachment #242986 - Flags: superreview?(peterv) → superreview+
QA Contact: nobody → editor
Are there still issues remaining here?
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).
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
No longer blocks: 333869
No longer blocks: mobb-4, 347257
Attachment #242986 - Attachment is obsolete: true
Attachment #242986 - Flags: review?(daniel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: