Closed Bug 664095 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox6 --- fixed

People

(Reporter: wolfgang, Assigned: wolfgang)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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>
Version: unspecified → SeaMonkey 2.1 Branch
I have found a third wrong behavior: After open test.html i can not set the cursor with the mouse.
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"
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() ?
I'll CC Neil and Ehsan for an opinion.
Blocks: 663649
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"?
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. :-)
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+
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-
Attachment #540989 - Attachment is obsolete: true
The Patch is created on mozilla-central hg identify 48e72227c2fa+ tip
Attachment #541305 - Flags: review?(ehsan)
Component: Composer → Editor
Product: SeaMonkey → Core
QA Contact: composer → editor
Version: SeaMonkey 2.1 Branch → Trunk
Moving to correct Product/Component - thanks for this work Wolfgang
Assignee: nobody → wgermund
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(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 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?
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 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?
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
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.

Attachment

General

Created:
Updated:
Size: