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)
Core
DOM: Editor
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)
|
(deleted),
patch
|
ehsan.akhgari
:
review+
dbaron
:
approval-mozilla-aurora-
christian
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•13 years ago
|
Version: unspecified → SeaMonkey 2.1 Branch
|
|
Assignee | |
Comment 1•13 years ago
|
||
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"
|
|
Assignee | |
Comment 3•13 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•13 years ago
|
||
I'll CC Neil and Ehsan for an opinion.
|
|
||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
||
Comment 5•13 years ago
|
||
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•13 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).
|
|
||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Attachment #540989 -
Flags: review+ → review?(ehsan)
|
|
||
Comment 9•13 years ago
|
||
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•13 years ago
|
Attachment #540989 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 10•13 years ago
|
||
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
|
||
Comment 11•13 years ago
|
||
Moving to correct Product/Component - thanks for this work Wolfgang
Assignee: nobody → wgermund
Status: NEW → ASSIGNED
|
|
||
Comment 13•13 years ago
|
||
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+
|
|
||
Comment 14•13 years ago
|
||
Oh, and thanks a lot for diagnosing and fixing this issue! :-)
|
|
||
Comment 15•13 years ago
|
||
Landed on inbound.
|
||
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
|
||
Comment 17•13 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•13 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?
Keywords: regression
|
||
Comment 19•13 years ago
|
||
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•13 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?
|
||
Comment 24•13 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-
|
||
Comment 25•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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]
|
|
||
Comment 28•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Oh, actually this *is* in beta now, so no need to do anything.
|
||
Comment 31•13 years ago
|
||
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.
Description
•