The default bug view has changed. See this FAQ.

Wrong behavior after load html file in composer (red Table border missing)

VERIFIED FIXED in Firefox 6

Status

()

Core
Editor
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Wolfgang Germund, Assigned: Wolfgang Germund)

Tracking

({regression})

Trunk
mozilla7
regression
Points:
---

Firefox Tracking Flags

(firefox6 fixed)

Details

(Whiteboard: [status-firefox-6:fixed for SeaMonkey 2.3 only])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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>
(Assignee)

Updated

6 years ago
Version: unspecified → SeaMonkey 2.1 Branch
(Assignee)

Comment 1

6 years ago
I have found a third wrong behavior:

After open test.html i can not set the cursor with the mouse.

Comment 2

6 years ago
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"
(Assignee)

Comment 3

6 years ago
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

6 years ago
I'll CC Neil and Ehsan for an opinion.

Updated

6 years ago
Blocks: 663649

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'm not sure what the bug is about.  Do you expect the handles to not appear if the table element has border="0"?
(Assignee)

Comment 6

6 years ago
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).
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.  :-)
(Assignee)

Comment 8

6 years ago
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 ?
Attachment #540989 - Flags: review+

Updated

6 years ago
Attachment #540989 - Flags: review+ → review?(ehsan)
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.
Attachment #540989 - Flags: review?(ehsan) → review-
(Assignee)

Updated

6 years ago
Attachment #540989 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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
Attachment #541305 - Flags: review?(ehsan)

Updated

6 years ago
Component: Composer → Editor
Product: SeaMonkey → Core
QA Contact: composer → editor
Version: SeaMonkey 2.1 Branch → Trunk

Comment 11

6 years ago
Moving to correct Product/Component - thanks for this work Wolfgang
Assignee: nobody → wgermund
Status: NEW → ASSIGNED

Updated

6 years ago
Duplicate of this bug: 666253
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.
Attachment #541305 - Flags: review?(ehsan) → review+
Oh, and thanks a lot for diagnosing and fixing this issue!  :-)
Landed on inbound.
http://hg.mozilla.org/mozilla-central/rev/de0a584b0c54
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Comment 17

6 years ago
(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

6 years ago
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).
Attachment #541305 - Flags: approval-mozilla-beta?
Attachment #541305 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Keywords: regression
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).
Attachment #541305 - Flags: approval-mozilla-beta?
Attachment #541305 - Flags: approval-mozilla-beta-
Attachment #541305 - Flags: approval-mozilla-aurora?
Attachment #541305 - Flags: approval-mozilla-aurora-

Comment 20

6 years ago
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.
Attachment #541305 - Flags: approval-mozilla-beta- → approval-mozilla-beta?

Updated

6 years ago
Duplicate of this bug: 601483

Updated

6 years ago
Duplicate of this bug: 663649

Updated

6 years ago
Duplicate of this bug: 666944

Comment 24

6 years ago
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.
Attachment #541305 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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
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.
(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
status-firefox6: --- → fixed
Whiteboard: [status-firefox-6:fixed for SeaMonkey 2.3 only]
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!
(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?
Oh, actually this *is* in beta now, so no need to do anything.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.