Last Comment Bug 664095 - Wrong behavior after load html file in composer (red Table border missing)
: Wrong behavior after load html file in composer (red Table border missing)
Status: VERIFIED FIXED
[status-firefox-6:fixed for SeaMonkey...
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Wolfgang Germund
:
Mentors:
: 601483 663649 666253 NikBull (view as bug list)
Depends on:
Blocks: 663649
  Show dependency treegraph
 
Reported: 2011-06-14 02:08 PDT by Wolfgang Germund
Modified: 2011-08-23 05:51 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() (687 bytes, patch)
2011-06-22 01:05 PDT, Wolfgang Germund
ehsan: review-
Details | Diff | Splinter Review
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2 (1.15 KB, patch)
2011-06-23 00:56 PDT, Wolfgang Germund
ehsan: review+
dbaron: approval‑mozilla‑aurora-
christian: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Wolfgang Germund 2011-06-14 02:08:22 PDT
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20110608 Firefox/4.0.1 SeaMonkey/2.1

Hallo

After load my test.html file (see below) in the SeaMonkey Composer Window there are two wrong behavior. (different to 2.0.14)



Reproducible: Always

Steps to Reproduce:
1. create HTML File test.html with <table ... border="0" ...
2. open Composer Window
3. File / Open File...

Actual Results:  
1. The Table with border="0" has no red dotted border.
   The Normal Mode looks like the Preview Mode.
   In DOM inspector i can see that EditorContent.css is not in CSS Roles.

2. The cursor is in the top right corner.


Expected Results:  
same as 2.0.14

1. The Table with border="0" has a red dotted border.
   In DOM inspector i can see that EditorContent.css is in CSS Roles.

2. The cursor is left from the first character.


Test Case
SeaMonkey Release 2.1 
Linux and Windows7
en-US and de

test.html

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  <title>1</title>
</head>
<body>
<h1 id="_nm_title">1</h1>
<p>abc<br>
</p>
<table style="text-align: left; width: 100%;" border="0" cellpadding="2"
 cellspacing="2">
  <tbody>
    <tr>
      <td style="vertical-align: top;">1<br>
      </td>
      <td style="vertical-align: top;">2<br>
      </td>
    </tr>
    <tr>
      <td style="vertical-align: top;">3<br>
      </td>
      <td style="vertical-align: top;">4<br>
      </td>
    </tr>
  </tbody>
</table>
<p>xyz<br>
</p>
</body>
</html>
Comment 1 Wolfgang Germund 2011-06-14 02:35:24 PDT
I have found a third wrong behavior:

After open test.html i can not set the cursor with the mouse.
Comment 2 newshs 2011-06-17 23:58:43 PDT
4th wrong behaviour : no "handling buttons" to insert or remove cells or lines or colums : impossibility to edit the table structure using the usual "border-buttons"
Comment 3 Wolfgang Germund 2011-06-20 01:07:06 PDT
I have a bug fix for this and need someone for the review and checkin.

The behavior come from composer/src/nsEditingSession.cpp
nsEditingSession::SetupEditorOnWindow()

  // Try to reuse an existing editor
  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mExistingEditor);
  if (editor) {
    editor->PreDestroy(PR_FALSE);
  } else {
    ...
  }

The clean up in nsHTMLEditor::PreDestroy() is insufficient!
I have copied 4 lines from DTOR to PreDestroy and the composer works fine.

diff -r f1acd88f828e editor/libeditor/html/nsHTMLEditor.cpp
--- a/editor/libeditor/html/nsHTMLEditor.cpp    Tue Jun 14 16:07:29 2011 -0700
+++ b/editor/libeditor/html/nsHTMLEditor.cpp    Mon Jun 20 10:01:10 2011 +0200
@@ -369,6 +369,11 @@ nsHTMLEditor::PreDestroy(PRBool aDestroy
     document->RemoveMutationObserver(this);
   }
 
+  while (mStyleSheetURLs.Length())
+  {
+    RemoveOverrideStyleSheet(mStyleSheetURLs[0]);
+  }
+
   return nsPlaintextEditor::PreDestroy(aDestroyingFrames);
 }


I have two questions to this:

1. Is it ok to copy or is it better to move the 4 lines code?
2. Are there other things in nsHTMLEditor::~nsHTMLEditor() that need to
   copy or move to nsHTMLEditor::PreDestroy() ?
Comment 4 Philip Chee 2011-06-20 08:50:59 PDT
I'll CC Neil and Ehsan for an opinion.
Comment 5 :Ehsan Akhgari 2011-06-20 11:39:25 PDT
I'm not sure what the bug is about.  Do you expect the handles to not appear if the table element has border="0"?
Comment 6 Wolfgang Germund 2011-06-21 00:26:58 PDT
After load of the document, SeaMonkey Composer uses frequently CSS Style manipulations.

In editor.js function SetDisplayMode(mode)

        case kDisplayModeNormal:
          editor.addOverrideStyleSheet(kNormalStyleSheet);
          // Disable ShowAllTags mode
          editor.enableStyleSheet(kAllTagsStyleSheet, false);
          editor.objectResizingEnabled = true;
          break;

        case kDisplayModeAllTags:
          editor.addOverrideStyleSheet(kNormalStyleSheet);
          editor.addOverrideStyleSheet(kAllTagsStyleSheet);

The editor is class nsHTMLEditor.cpp

The function addOverrideStyleSheet() don't work correctly after load of a document. If i edit a document from about:blank CSS Styles work fine. 

This behavior is since hg log entry:

Änderung:        57958:64fdcad8cb11
Nutzer:          Ehsan Akhgari <ehsan@mozilla.com>
Datum:           Thu Nov 18 16:01:12 2010 -0500
Zusammenfassung: Bug 612447 - Don't Recreate an editor object attached to a document in a frame if that frame is restyled; r=bzbarsky a=blocking-beta8+

I have a build system for SeaMonkey 2.1 and 2.2
SeaMonkey 2.1 is http://hg.mozilla.org/releases/mozilla-2.0
SeaMonkey 2.2 is http://hg.mozilla.org/releases/mozilla-beta

If i disable the reuse of the editor in nsEditingSession.cpp the composer works fine.

line 454
  // Try to reuse an existing editor
+ mExistingEditor = NULL;
  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mExistingEditor);
  if (editor) {
    editor->PreDestroy(PR_FALSE);
  } else {
    editor = do_CreateInstance(classString, &rv);
    NS_ENSURE_SUCCESS(rv, rv);
    mExistingEditor = do_GetWeakReference(editor);
  }

But i think the reuse of the editor has a sense which i don't know.

With the patch in comment 3 which i have tested on SeaMonkey 2.1 and 2.2
the function addOverrideStyleSheet() works fine in fresh document and after load (reuse an existing editor).
Comment 7 :Ehsan Akhgari 2011-06-21 08:02:56 PDT
OK, I see what you mean now.

I think the patch should be safe.  I would be happy to review and test it if you can attach it.  See https://developer.mozilla.org/en/creating_a_patch and let me know if you need help.  :-)
Comment 8 Wolfgang Germund 2011-06-22 01:05:29 PDT
Created attachment 540989 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy()

I hope I've understood everything correctly.

On Development Repository http://hg.mozilla.org/mozilla-central
hg pull
hg update
hg identify
b7a93f1279b7+ tip
hg diff ...

I think mozilla-central is assigned to SeaMonkey 2.4.
It is possible that the patch is checked-in also in mozilla-beta for SeaMonkey 2.2 ?
Comment 9 :Ehsan Akhgari 2011-06-22 11:58:48 PDT
Comment on attachment 540989 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy()

I think you should remove this code from the destructor too (basically we need to move it from the destructor to nsHTMLEditor::PreDestroy).

This patch won't get to mozilla-beta unfortunately, but it can land on mozilla-central once it's reviewed.
Comment 10 Wolfgang Germund 2011-06-23 00:56:07 PDT
Created attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

The Patch is created on mozilla-central
hg identify
48e72227c2fa+ tip
Comment 11 Ian Neal 2011-06-23 10:57:23 PDT
Moving to correct Product/Component - thanks for this work Wolfgang
Comment 12 Ian Neal 2011-06-23 10:58:34 PDT
*** Bug 666253 has been marked as a duplicate of this bug. ***
Comment 13 :Ehsan Akhgari 2011-06-23 13:07:33 PDT
Comment on attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

r=me.  I will land it when the try server build finishes.
Comment 14 :Ehsan Akhgari 2011-06-23 13:08:02 PDT
Oh, and thanks a lot for diagnosing and fixing this issue!  :-)
Comment 15 :Ehsan Akhgari 2011-06-24 10:56:20 PDT
Landed on inbound.
Comment 16 Marco Bonardo [::mak] 2011-06-25 03:26:42 PDT
http://hg.mozilla.org/mozilla-central/rev/de0a584b0c54
Comment 17 newshs 2011-06-25 04:10:51 PDT
(In reply to comment #5)
> I'm not sure what the bug is about.  Do you expect the handles to not appear
> if the table element has border="0"?

No, they don't appear, though they should, which makes it impossible to use them to edit the table :)
Comment 18 Ian Neal 2011-06-25 04:32:23 PDT
Comment on attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

It would be good to get this regression fix into aurora (or beta if the merge happens before this gets approved).
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-28 15:04:41 PDT
Comment on attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

Minusing approval requests per triage meeting.

We've shipped this bug in 4 and 5, and it's not clear to us what the impact of the bug is (for lack of a clear description of either the bug, the benefits of the patch, or its risks).
Comment 20 Ian Neal 2011-08-01 10:45:06 PDT
Comment on attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

This is a low risk fix (I'm sure ehsan could confirm) and far as I can see the only product it does impact on is Composer (as product and component of SeaMonkey) where, without the fix, it makes inserting and editing of images, tables, etc very hard work.
If it doesn't make mozilla-beta then it will be about 2 months before a usable version will be released, which is not good for the users.
Comment 21 Ian Neal 2011-08-01 11:09:03 PDT
*** Bug 601483 has been marked as a duplicate of this bug. ***
Comment 22 Ian Neal 2011-08-01 11:11:27 PDT
*** Bug 663649 has been marked as a duplicate of this bug. ***
Comment 23 Ian Neal 2011-08-01 11:15:21 PDT
*** Bug 666944 has been marked as a duplicate of this bug. ***
Comment 24 christian 2011-08-01 14:40:20 PDT
Comment on attachment 541305 [details] [diff] [review]
Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2

Denied for mozilla-beta. If the SM team wants this they can land it on their own relbranch or if they really need it on default please feel free to lobby us harder and renominate.
Comment 25 Justin Wood (:Callek) (Away until Aug 29) 2011-08-01 21:57:52 PDT
(In reply to comment #24)
> Comment on attachment 541305 [details] [diff] [review] [diff] [details] [review]
> Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2
> 
> Denied for mozilla-beta. If the SM team wants this they can land it on their
> own relbranch or if they really need it on default please feel free to lobby
> us harder and renominate.

For the record, looking over this bugs history, I'd like assurances for ehsan (or even kaze) that this would be a safe m-beta patch for our end to want on a relbranch this close to SeaMonkey's final beta. Without that assurance I won't expend the energy to relbranch for this alone on our 2.3 release.

That said, this is already in the Firefox 7 train, which will be in beta in about 2 weeks, so will be in SeaMonkey 2.4
Comment 26 :Ehsan Akhgari 2011-08-02 08:56:16 PDT
This patch should be very safe, and has already lived on trunk and Aurora for a while without causing issues.  We do not want it for Firefox, as it won't provide any obvious benefits, but I think this is something that SeaMonkey can take on their own relbranch for 2.3.
Comment 27 Justin Wood (:Callek) (Away until Aug 29) 2011-08-04 23:19:04 PDT
(In reply to comment #26) [ehsan said]
> This patch should be very safe, and has already lived on trunk and Aurora
> for a while without causing issues.  We do not want it for Firefox, as it
> won't provide any obvious benefits, but I think this is something that
> SeaMonkey can take on their own relbranch for 2.3.

I'm pushing this to a SeaMonkey relbranch now...

Marking status-firefox-6:fixed along with a whiteboard entry for lack of another clean way to track.

http://hg.mozilla.org/releases/mozilla-beta/rev/fd46ee429912
Comment 28 :Ehsan Akhgari 2011-08-16 10:42:27 PDT
We're not taking this bug for the beta migration, but if you guys wanna take this for your own relbranch again on beta, please do so!
Comment 29 Justin Wood (:Callek) (Away until Aug 29) 2011-08-16 10:57:39 PDT
(In reply to Justin Wood (:Callek) from comment #25)
> (In reply to comment #24)
> > Comment on attachment 541305 [details] [diff] [review]
> > Patch to clear CCS Styles in nsHTMLEditor::PreDestroy() Version 2
> > 
> > Denied for mozilla-beta. If the SM team wants this they can land it on their
> > own relbranch or if they really need it on default please feel free to lobby
> > us harder and renominate.
>
> That said, this is already in the Firefox 7 train, which will be in beta in
> about 2 weeks, so will be in SeaMonkey 2.4

(In reply to Ehsan Akhgari [:ehsan] from comment #28)
> We're not taking this bug for the beta migration, but if you guys wanna take
> this for your own relbranch again on beta, please do so!

Ehsan, huh are you saying that its being backed out of aurora/beta on Gecko 7 for a reason?

If so can I please be informed of the reasoning?
Comment 30 :Ehsan Akhgari 2011-08-16 11:02:23 PDT
Oh, actually this *is* in beta now, so no need to do anything.
Comment 31 Trif Andrei-Alin[:AlinT] 2011-08-23 05:51:09 PDT
Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20110813 Firefox/6.0 SeaMonkey/2.3

I've tested the html code posted in the description and there are no borders what so ever.(border="0")
Setting resolution to VERIFIED FIXED, but if i am mistaking feel free to rechange the status.
Thanks.

Note You need to log in before you can comment on or make changes to this bug.