Open Bug 97278 Opened 23 years ago Updated 2 years ago

Retain original source formatting: Composer adds newlines when switching view modes (extra empty space added)

Categories

(Core :: DOM: Serializers, defect, P3)

Product:
Core
Component:
DOM: Serializers
Type:
defect

Tracking

()

Milestone:
mozilla1.9alpha1

People

(Reporter: Peter, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: dupeme)

Attachments

(2 files, 1 obsolete file)

Demonstration of extra blank lines added
1.34 KB, text/html
Details
fix !!! At last !
7.88 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review 
Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.3+) Gecko/20010827

In Composer (html view) there seems little control over linebreaks and indented
text (edit mode, not web page formatting stuff).

I cannot seem control where line breaks and indentation go (html edit text, not
web content formatting).

Switching between the html tab and the "normal" tab has horrible consequences
(pref: retain source formatting is selected). Here is an example:

Before:
*******************************************************************
<html><head>
  <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  <meta name="author" content="Peter Lairo">
</head><body><br>


After switching to "Normal" tab, and then back to html tab:
*******************************************************************
<html><head>
  
  <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">

  
  <meta name="author" content="Peter Lairo">
</head><body><br>
I should add that this makes editing html a *very* frustrating task.

The example I gave is just one of many linebreak & spaces obliterating problems.
Keywords: mozilla0.9.4, nsCatFood
Whiteboard: dupeme
I bet that this is just a dup of bug 95129
we have a component for composer you know, called "editor".  Might be 95129
Assignee: asa → brade
Severity: major → normal
Component: Browser-General → Editor
QA Contact: doronr → sujay
Not a dupe of bug 95129 because that bug is about how formatting is *saved* as
controlled by a pref setting; this bug occurs *without* ever saving the file, it
is an editing problem. 

*While* editing and switching between the "source" and "normal" tabs, the source
formatting should stay as the user entered it.
Try toggling the pref described in bug #95120.
This may indeed be a dup of that bug.

The pref isn't quite clear but it is what is used when switching between source 
mode and the other modes as well as what is used for saving (and eventually 
publishing).  It has to do with how the DOM is serialized into HTML (regardless 
of destination being memory, disk, clipboard, or server).

I found no reference to a pref setting in bug 95120.

I tried both seemingly applicable settings in pref ("retain original source
formatting" and "Pretty Print"). Neither had the desired effect (maitain the
formatting made by the user).
-->akkana
Akkana--is this a serializer issue or something we can fix?  Do we have another 
bug on this?
Assignee: brade → akkana
I don't think it's a dupe of bug 87102. The other bug refers to the formatting
after saving the file. Also, the other bug only mentions the formatting at the
beginning and end of the file. This bug is about linebreaks and indentation
throughout the file and *while* editing (not after saving).

I suggest to keep them separate because this bug deals with the broader problems
with editing html files in composer.
Confirmed on 9-10 build.  The tab button seems to have no affect in the HTML
window as well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is the same issue as in the other two bugs already referenced.  The
serializer doesn't do a good job of retaining source formatting.  Over to the
lucky serializer owner.
Assignee: akkana → peterv
Component: Editor: Core → DOM to Text Conversion
Priority: -- → P2
Target Milestone: --- → mozilla1.1
-> tanu
Assignee: peterv → tmutreja
Looks like parser related bug.
-> Harishd
Assignee: tmutreja → harishd
[was 1.1alpha]
Target Milestone: mozilla1.1alpha → Future
When becomes this bug fixed?
I can recognize no progress for a bug fix:
Now I can't wait much longer and have to use an
alternative HTML-editor for mozilla's composer.
Can somebody tell me when this bug becomes fixed?
The workaround is to set the "Reformat HTML source" preference (in the Composer
prefs panel).
Setting the "Reformat HTML source" preference (in the Composer prefs panel);
is no workaround for me becuase I write my documents in the HTML-source view and
reformat destroys all my formats (esppeciall  the line breaks are not as I like
it). 
So again my question: Are there plans to fix this bug in the near furture?
I don't think I'm too aggressive if I ask for the progress in this
bug fix. My last question was written 2002-10-11 14:09.
I now believe that this bug will become never fixed, so I suggest to 
set it to WONTFIX and remove the option
   "Retain original source formating"
in the preferences dialog. 
I believe that nobody uses the Composer for something else then e-mail 
writting, because a handmade html-page is not editable with composer
until the handmade html-source format becomes retained.
PS: My last try was Mozilla nightly build 17 Jan 2003.
Noting potential duplicates: this appears to be related to
bug 25141
bug 174361
bug 176866
Blocks: 174361
This bug's report focuses on unwanted newlines in the <head> of the document;
as such, it is a duplicate of Bug 25141. (Peter Lairo, do you agree?  Harish?)

It is closely related, but not identical, to Bug 176866, which refers to line
break issues and other partial pretty-printing within the <body> of the document.
Bug #25141 is about newlines being removed, this one is about them being added.
As such they are seem to be different. Unless the cause of both problems is the
same, in which case they are dups.
Ah, I see.  OK, not a dupe of 25141.

I had been under the impression that the extra whitespace occurred, in 1.2 and
earlier, simply by opening the file.  Checking back on 1.2, that is not the
case.  I apologize for the confusion.

The behavior described in Bug 176866 occurs under identical circumstances.
The behavior currently being complained about in Bug 159615 also occurs under
identical circumstances.

I posit that:

  176866 is a duplicate of this bug.
  159615 should be re-closed, and discussion of that problem directed here.
*** Bug 176866 has been marked as a duplicate of this bug. ***
*** Bug 194579 has been marked as a duplicate of this bug. ***
Attached a test case in which the HTML has no blank lines.  Open it in Composer
(CTRL-E from browser works), switch to HTML Source view, make any edit, switch
to Normal View and back to HTML Source view.  Each time you edit and switch out
of HTML Source view, MORE blank lines are added

Can we change the summary of this bug to something more descriptive, such as:

"Composer adds newlines switching to HTML Source view in Retain Orignal Source
Formatting mode"
Summary: In Composer (html view) there seems little control over linebreaks and indented text → Composer adds newlines when switching view modes.
*** Bug 196411 has been marked as a duplicate of this bug. ***
carryover dependency from duplicate bug
Depends on: 190955
That dependancy looks more like a dup of an existing bug, however I don't have
enough time to search the list on the tracking bug (Bug #174361), so I'm just
going to add it to the tracking bug for now.
*** Bug 210670 has been marked as a duplicate of this bug. ***
Taking...
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.5beta
Hmm, I thought "Accept" was supposed to assign the bug to me.
Assignee: harishd → burpmaster
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
I'm wondering if this bug will be fixed.  It is quite difficult to use composer
as an HTML editor. Perhaps the target milestone should be changed since 1.5 has
already been released.
I'll start working on a patch for this soon.  Setting target milestone to
1.7alpha.  If I get this done early enough, we can try for blocking1.6b.

What makes this bug tricky is that Mozilla *should* apply some formatting for
this mode, but only to new content.  This ensures, for example, that a new
document created in Composer won't all be a single line in the source.

The current system seems to keep track of what is new, marking nodes as "dirty"
in the DOM.  Then, when serializing, only dirty nodes are formatting.  Of
course, this isn't working correctly right now.  I'll look into making this
work, but it may be better to make the editor do this rather than the
serializer.  For example, the enter key would add a <br/> node and then a
newline right into the DOM, and the serializer would just output everything as-is.
Summary: Composer adds newlines when switching view modes. → Retain original source formatting: Composer adds newlines when switching view modes
Target Milestone: mozilla1.5beta → mozilla1.7alpha
If I'm remembering correctly, I think the reason we did it this way -- have the
serializer rather than the editor add the formatting whitespace -- is that
adding the formatting nodes in composer would vastly complicate the edit rules.
 Formatting whitespace often depends on context (what node comes before or
after, or what node contains the current node) so every time any node was added
or removed, composer's edit rules would have to look around the insertion point
and possibly modify whitespace for nearby nodes as well as the node being
inserted or deleted.  That's not impossible (perhaps it could even be fast
enough) but no one wanted to sign up to do that work and all the associated
testing that would be required.  Doing the formatting at the serializer level,
where nothing is changing, seemed much simpler (as well as easy to test in an
automated way).

Even if you rewrote all the edit rules to handle the whitespace insertion there,
you'd still have the problem of whitespace munging by the parser and resulting
serializer confusion (the document has to go through the serializer to go to
source view and the parser to get back to normal view -- there's no alternative
there) so I recommend concentrating on those issues first. Then if you want to
try moving whitespace handling into the editor, do that as a separate step.
I still think the serializer should do the pretty printing, but when pretty
printing is turned off, I want to ensure that newlines are inserted after <br>
tags and in some other appropriate locations, but only when those tags are newly
created in Composer.  No whitespace should be added around tags that were in an
opened file to begin with (in retain original mode).  That is why adding the
whitespace during the edit makes sense to me.

I'll go ahead and investigate why every node is always marked dirty.  If that is
easy to fix, I'll get the current scheme working.
Attached patch Fix (obsolete) — — Splinter Review 
It's simpler to fix than I originally thought.

By testing the three places where MarkNodeDirty was called from, I found that
CreateElementTxn and SplitElementTxn are used during editing, and
InsertElementTxn is used while loading the document (or switching from source
view to normal view).

So, my fix is to remove MarkNodeDirty from InsertElementTxn::DoTransaction.
Comment on attachment 137840 [details] [diff] [review]
Fix

Requesting review
Attachment #137840 - Flags: review?(mozeditor)
Burpmaster, can you ask Akkana for review?  She's CC:ed already and is active
again on Mozilla...
Comment on attachment 137840 [details] [diff] [review]
Fix

Okay, changing the review request to Akkana.
Attachment #137840 - Flags: review?(mozeditor) → review?(akkzilla)
*** Bug 237773 has been marked as a duplicate of this bug. ***
*** Bug 209157 has been marked as a duplicate of this bug. ***
*** Bug 220186 has been marked as a duplicate of this bug. ***
Summary: Retain original source formatting: Composer adds newlines when switching view modes → Retain original source formatting: Composer adds newlines when switching view modes (extra empty space added)
Wow, that's a nice simple patch.  But are you sure that InsertElementTxn is
never used while editing?  Grepping for it in .cpp and .js files gets lots of
hits (from insertElementAtSelection, mostly).  Is there a simple way to show
that it's not used in editing?

Cc'ing a couple other editor folk who might know more about how this transaction
is used.
/me falls into gdb mode to check
As I suspected, this transaction is used all the time... When you click on B and
type some text, it's called; when you launch composer and type some text, it's
called; whatever element you create, it's called...
Comment on attachment 137840 [details] [diff] [review]
Fix

r-, but I'm happy to reconsider if you can convince me that it's safe.
Attachment #137840 - Flags: review?(akkzilla) → review-
It should be safe, because the dirty flag in question is only used for one
purpose (just do a text search for "_moz_dirty"), it tells the serializer to
apply formatting regardless of the setting.  The worst that could happen if it
wasn't set when it should have been is that the source output won't be formatted
nicely.

The important thing is to catch all the key events where we want to mark
something for one-time formatting.  CreateElementTxn is called while inserting
linebreaks, tables, and other things in the editor, and the MarkNodeDirty is
taken care of there.  This produces the desired behavoir of formatting things
only after creation, and then letting users make whatever changes they want
without Composer reformatting again when "retain original source formatting" is on.
(sorry, if I'm reporting to wrong place, but I could not find any Nvu bugs
database.)

Well... I was using Nvu 0.2 on Windows when I noticed extra lines got added and
it seems to me there is something wrong with those linefeed characters besides
of them being added to the places they are not wanted (which is annoying, no doubt).

It seems the linefeed characters are of "wrong" type.  When I opened the
document with gvim after saving it in Nvu, I noticed, that each linefeed had an
extra ^M character at the end of the line. And not only the extra blank lines
but all the other lines also had the ^M symbol at the end.

If it is a separate bug - please be kind to me and don't beat me in my face...
*** Bug 244005 has been marked as a duplicate of this bug. ***
Blocks: 247221
Blocks: 242511
Since one year there is no news about this.
As an average user, I did not recompile the source with the given patch. But the
status of the bug does not tell me if the problem is (to be considered as)
fixed, i.e. if the currently distributed version still has this annoying bug
(which makes it quite useless, imho, for editing HTML pages intended to be
published) or not.
In the first case (bug fixed), this should be mentioned, so the community can
upgrade their version and relax.
In the second case, I would like to make another suggestion, which would be to
insert a blank (if this is considered necessary) only if there is not yet a
blank line present. This should be quite straightforward to implement.
Thanks and apologies if my post was not adequate.

Have you tried NVu? www.nvu.com - it's based on Composer, and *much* improved. 

I'm not sure Composer is much developed anymore  (if someone knows better, I'd
love to know!) except in the context of HTML mail...

/Mikael
What I consider the major advantage of NS/Mozilla over all other collection of
tools (be it Gnome or KDE or Microsoft), is to have "all in one", nicely
interacting client for E-mail, Web browsing, editing a viewed document and
uploading it back again, etc...

I do not even consider as an alternative to install yet another huge package
with all the mess that comes around such an installation, to do just one of the
jobs (which are to 99% [of my needs, at least] accomplished by the all-in-one
tool Mozilla).

And if Mozilla could be such a nice thing by just adding a tiny
 "...if( output_buffer[position-1] != '\n') ..."
somewhere, I think it would be a pity not to do it, and I can't see why it takes
years to do it!
(Of course I know I exaggerate the easiness a little bit, but not so much...)

I've been busy with college, but I'm taking a lighter schedule this summer, so
I'll have time to work on this.
Target Milestone: mozilla1.7alpha → mozilla1.9alpha
Forgive my bluntness, but fixing Composer is a waste of your time. As previously
mentioned, not only is Composer now being maintained in the form of Nvu, but the
Mozilla Suite is officially being abandoned in favor of smaller apps like
Firefox and Thunderbird, so your contributions would most likely go to waste.
Please read http://www.mozilla.org/seamonkey-transition.html.

I think Mad Max's preference for an unwieldy monolithic application is driven
more by nostalgia than logic. There's little practical difference between
installing a single huge app verses installing 2 or 3 smaller apps which
collectively perform the same purpose. If anything, using the smaller apps is
more efficient, because you download what you need and use what you need.
Personally, I like how if Firefox crashes (granted, a rare occurrence, even with
Deer Park), my email, html editor, and chat clients don't also crash.

Again, if you truly wish to aid in Composer's development, you should lend your
talents to the Nvu project.
(In reply to comment #54)
> Forgive my bluntness, but fixing Composer is a waste of your time.

I'll try not to go into a long-winded reply, but there are a few things I don't
understand. First I don't understand why you take the time to come here and post
that.
Well, Nvu is worth taking a look if it actually works in something other than
Linux desktop.

> Mozilla Suite is officially being abandoned in favor of smaller apps like
> Firefox and Thunderbird,

Well that's not exactly what that says. And I don't understand that anyway, it
does not seem logical to me at all.

> If anything, using the smaller apps is more efficient, because you download
> what you need and use what you need.

Well it should be that way anyway, you should not have to install the Composer
if you don't want it, you should not have to download and install Calendar if
you don't use it. I'm sure this has been debated to death, but it seems more
logical to have everything based on Gecko, one suite for testing extensions, and
separate components of that suite. I don't see the logic in splitting everything
up so that they eventually become incompatible. For example FireFTP only works
in Firefox. This doesn't make sense, if it was developed in the Mozilla Suite it
should work in both, and we'd know it was compatible with, say, Thunderbird.
Another example on the Nvu site: "If you can't launch Mozilla after Nvu, it's
because your version of Mozilla or Mozilla Firefox is too old".  Now if they
were truly separate applications this would never be a problem, so this just
goes to show that it's a potentially huge problem.
Take Perl for example; some Perl libraries may require you compile Perl with
certain options, but all libraries are compatible. You don't have to download
some libraries you don't need. I prefer to design my applications (web sites and
scripts) using the "component" method, I build the entire unit, break it into
pieces, then reuse the pieces. If I update the core or a piece, this updates
everything, or if I make a bad design choice I'll have to go through and
recreate the links. Designing in this way means that you need to test before you
can implement, which is why you need the Suite, in my case I fake it and use a
test component (using a test link), testing all functions before it's put into
production (just like testing in a "suite").
An analogy would be making a major change to a style sheet or script file, you
link to a test script to verify it works on a test page, then you can update all
your links. It makes no sense to duplicate the same code on every page, not only
is it more work, you eventually end up with variations of the same code.

I personally like Composer and Mozilla, I am a long time Netscape user and still
use the Communicator theme. I don't use Composer much only because of this bug,
but I use practically every other Mozilla Suite component, the Java Console is
the only part I really don't use, so downloading the entire suite is no big deal.

Another issue I don't understand is the OS compatibility, compare:
http://www.mozilla.org/products/mozilla1.x/sysreq.html
http://www.mozilla.org/products/firefox/system-requirements
http://www.mozilla.org/products/thunderbird/sysreq.html
(Nvu doesn't say, so we're left to experiment I suppose)

Now I really don't like Firefox at all, can I make it look and function like
Mozilla? I don't think so. I can't install and test FF or TB in, say, Win95. The
difference tells me there are things in FF and TB I don't need, since Mozilla
works fine. And what about features in Mozilla that aren't in Thunderbird; can I
middle-click a link in TB and have it open in a new tab in FF? FF doesn't even
offer FTP upload, so I can't convince clients to use Firefox instead of IE
because they'd also have to install FireFTP, or go back to using IE for that, or
they should install a real FTP program which is what FireFTP is supposed to be,
but what's wrong with having basic FTP functions extended by FireFTP?

But if we like Composer, there's no reason not to pursue it. Maybe we like Nvu
or Amaya, we can develop those as well. I don't want "FrontPage like features",
screw that, I resent the comparison.
Mozilla may not be the most popular choice, it may not be "officially approved",
but it doesn't need to be. It's a development tool. If bugs like this get fixed
and they stop at 1.7.x that's fine by me, if it ain't broke there's no need to
fix it. The problem I see with the 1.8.x road map is that they will likely drop
certain components that aren't "popular". "We have FireFTP so lets drop all FTP
functions in 1.8.x" or "we'll drop the composer component because Nvu is more
popular". Seems more logical to encourage Composer in order to steer people way
from **** apps like Frontpage and Dreamweaver that are the source of so many
evangelism problems today.

If you want to work on this bug, please do, even if it means a patch. I will do
as much as I can as well.
I think maybe I need to report a "popularity bug".
One more note: it sure would be nice to be able to edit the html source of eMail
messages. Communicator allowed you to edit the HTML source of a message, this
could be a use for Mozilla Composer, but isn't.
Nothing to do with this bug, but another reason to continue working on Composer.

Hey, calm down :-)

No need to make this an nvu/composer fight. Both products have, or might have, a
place. If someone wants to keep using composer, or keep developing composer,
that is perfectly in order.

I was merely suggesting to Mad Max that there is another product that might fit
*his* needs. Seems that is not the case, so let's leave it at that.
I don't want to start a fight, no way!
I think I am implying that Nvu could be an extension of Composer, not a
replacement. If one can debug Composer, one could also debug Nvu, since they
both use the same core. I know we don't want to see "this Nvu bug depends on
this Composer bug", so Composer should be kept simple and Nvu (or some
extension) can extend it.
It shouldn't be a fight, but a co-op.

This is a bugzilla bug, not a newsgroup. Please refrain from posting
non-technical unrelated to the bug ITSELF stuff here, and move to
netscape.public.mozilla.editor . Thanks.
I'm unclear what suite-vs-separate has to do with this bug anyway.

nVu goes through the same gecko mechanism to get the source that composer does,
doesn't it? (libeditor, the gecko parser, the serializer, etc.)

If Daniel has solved this problem in nVu (has he? I don't see any reports here
indicating that nVu does anything different from composer on this problem) then
that fix should be able to be backported to the gecko used in the suite; or that
may even happen automatically when the nVu branch lands.

If not, then both nVu and Composer could benefit from a solution.

Of course, the fact that the suite is orphaned means that wherever a fix comes
from, there won't be any further suite builds which would incorporate the fix.
People who want an updated Composer will have to build the suite themselves.
That's a bummer and I wish it wasn't true, but this isn't the right place to
mourn it.
I've continued this discussion in netscape.public.mozilla.general, in the thread
labeled "Mozilla Composer (Bug 97278 Cont.)".
I've been following this bug for a couple of years now, and hoping it will be fixed. I originally began using the Mozilla Suite because it included a nice editor that would handle simple html pages, and let me fiddle with the code in html mode. But having the code view add extra lines is such a huge drawback that I have had to move to programs which are available for windows only, which I hate to do because I am such a Linux fanatic.
Are there any plans to ever fix this problem?
I checked this with SeaMonkey 1.1.3 in win32 and found the problem still persists, and possibly a couple related bugs; multiple break tags are added to empty table cells; it was an xhtml page and unclosed tags, including the newly inserted break tags, are not terminated correctly. I opened a directory listing in SeaMonkey, saved it as index.htm, opened it in Composer, made changes to the head in source view then saved it. Breaks are added to the empty filesize column.
I'll have to correct the xhtml and try it again, though I don't think that should make any difference, but perhaps it is a new, related bug.
Valid HTML does the same thing.
Seems to me that just disabling line wrapping would fix this bug, but unfortunately I'm no expert programmer, wish I was.

There is a workaround for the adding-white-lines problem: Edit | Preferences | Composer | detick 'Preserve original source formatting'.

And there certainly is a place for Composer! It (up to v. 1.8) is by far the most user-friendly WYSIWYG editor. Perfect for people with little or no knowledge of HTML but who cannot afford to have their website maintained by a pro. 

- Frank 
There is, indeed a place for Composer. But it is useless for anyone who ever wants to view the html source, even in a plain text editor. I gave a copy of Composer to a friend who I wanted to do web editing for me. He never used the code view to do any editing, but when I get any page back that he has edited several times, it is practically unreadable in my Kate editor. To get from one line of code to the next, I sometimes have to scroll through several pages of blank lines. this is indeed a severe bug, as many simple web pages on my site have tripled, and even quadrupled in size from the added bulk of blank lines.
When I first picked up a copy of NVU, (which is Composer without the rest of the Mozilla Suite) I thought it was a fabulous WYSIWYG web page editor, and I still feel this way. If this bug were fixed, NVU would be, in my mind, far superior to FrontPage, Pagemaker, or any other web authoring tool at any price.
QA Contact: sujay → dom-to-text
I'm still hoping that there will be a fix for this bug. I am still using NVU and Kompozer, but now, each time I edit a web page, after I close it in NVU, I load it up in Kate, and then run "HTMLTidy" with a few switches set that remove extra blank lines. It's not perfect, but it does keep the bulk out of the web pages.
Dick
After long work and painful investigations on our serializers, I have
now a much much cleaner solution in hands. It lives inside BlueGriffon
and will be shipped with forthcoming v1.3.

  - honours MUCH better the OutputFormatted and OutputWrap flags
    in HTML, XHTML and XML; we had really severe holes in that
    implementation.

  - solves issues in pre-like elements

  - finally solves our 10+ years old bug of blank lines inserted in the
    source. The guilty code was in

      nsXMLContentSerializer::AppendWrapped_NonWhitespaceSequence

    that could insert a wrapping CR even if a CR was already present
    at the wrapping spot. Of course, since the rest of the wrapping code
    had big holes (see above), it was hard to fix...

  - adds a new useful nsIDocument flag for entities' output that
    Bluegriffon needs; other apps could find it useful too.

Since it's a Gecko-only patch, here it is. And since I don't have the
time to work on BMO bugs, you can do whetever you want with it. Just
give the credits please? Let me know.
Attachment #137840 - Attachment is obsolete: true
(In reply to Daniel Glazman from comment #68)
> Created attachment 574896 [details] [diff] [review] [diff] [details] [review]
> fix !!! At last !

Daniel, perhaps you'd like to request for review from anyone on this list:

https://wiki.mozilla.org/Modules/Core#Document_Object_Model

It will be really useful and will help to get the patch on its road to land on Gecko. :)
Comment on attachment 574896 [details] [diff] [review]
fix !!! At last !

I think bz is the right person to review this.
Attachment #574896 - Flags: review?(bzbarsky)
The patch is now live inside BlueGriffon 1.3 and works fine :-)
Comment on attachment 574896 [details] [diff] [review]
fix !!! At last !

I'm sorry for the lag here....  It took me a long time to page this code into my head.  :(  The lack of context in the patch or any sort of explanation for the changes didn't help anything...

> +  if (((mDoFormat || forceFormat) && !mPreLevel) || mDoRaw) {

Why?  If mDoRaw, we don't want to AppendNewLineToString when lineBreakBeforeOpen, do we?  Nor do we want to MaybeAddNewlineForRootNode...  This applies to both places where this change was made.

> +          entityText = ToNewCString(entityValue);

This leaks the string. You want to use entityReplacement, not a new entityValue, here and set entityText to entityReplacement.get() just like the case that calls HTMLConvertUnicodeToEntity.  Or better yet fix this code (in a patch below this one) to be sane and not use raw pointers that make it easy to leak like that.

Why the mDisableEntityEncoding changes for XHTML?  Those don't make sense to me, since XHTML doesn't have weird (P)CDATA stuff going on for those tags, unlike HTML.  At the very least, they deserve comments.

> +  if (mColPos + 1 >= mMaxColumn && !mDoRaw) {

Check mDoRaw first, since it's the common case.

The body of this block is a copy/paste that we seem to have repeated ten bajillion times.  More bonus points for factoring it out into a function (that takes an argument for whether to append indentation); this is probably best as a followup.

>+  //AppendToStringConvertLF(attrString, aStr);

Why add that?

The loop looking for newlines and spaces in nsXMLContentSerializer::AppendToString should just use PRUnichar, and say "Newline" instead of "CR", because treating \n as "CR" is just confusing, imo.

> +            CRDone = (*(aSequenceStart + wrapPosition - 1) == '\n');

How do you know wrapPosition != 0? Is that guaranteed by something?  This code could use a comment explaining what it's trying to do, because I don't follow.  Maybe that's because it says "CRDone" but looks for \n?  Is it trying to look for \r\n or just for newlines period?

r- pending answers to the questions and the memory leak fix...
Attachment #574896 - Flags: review?(bzbarsky) → review-
I've just installed the latest Seamonkey 

(User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:15.0) Gecko/20120909 Firefox/15.0.1 SeaMonkey/2.12.1    Build identifier: 20120909051431 )

and this bug IS STILL THERE. 

Not as bad as it was but it still add blank lines each side of <title></title>, after <body> and before </body> as well as random <br> being inserted.

I have (as always) Preserve original source formatting selected
@Everlevel: Even if you don't have "preserver original source" (i.e. allow the editor to strip away all the garbage in the source) it's the same thing: blank lines inserted all over the place at SeaMonkey's own will.

This is LUDICROUS!!!!!! A bug that makes the editor such a pain to use not to be fixed after MORE THAN 10 YEARS!!!

BTW, i installed Kompozer (http://www.kompozer.net/) and that one DOES CLEAN UP the html source if you select "Reformat HTML source"
I worked on some Mozilla bugs as a hobbyist/volunteer a long time ago, and I still have this bug assigned to me. Since I never got around to fixing this, I'll reset the assignee now so it doesn't look like I'm working on it, and maybe someone else will take it.
Assignee: brian → nobody
Assigned without Assignee

What's on with the patch?
Status: ASSIGNED → NEW
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: