42429 - Source formatting inside tables is stripped off

Status: VERIFIED FIXED

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

Description

[Akkana Peck]

Make an html file (I'll attach one) with a table which has some formatting to
make the html source readable.

Read that file into composer, then do a Debug->Output HTML and observe that all
the formatting has been lost.  Do a Debug- > Dump Content Tree, and observe that
the formatting isn't making it in to the output because it was never put into
the content model.

Comment 1

[Akkana Peck]

This is a fairly recent regression: adding keyword nsbeta2 in the hope that it
will encourage searching for whatever caused the regression.  If not beta2, we
should certainly fix it for beta3.

Comment 2

[Akkana Peck]

Attachment: HTML file with formatting inside a table

Comment 3

[Peter Trudelle]

nsbeta2-

Comment 4

[rickg]

Likely a XIFDTD problem. The content model (in the browser) is fine.

Comment 5

[Akkana Peck]

The content model, if you print it from the editor's Debug->DumpContentTree,
shows no whitespace inside the table.  The regression is happening at parse
time, not at output time.

Comment 6

[rickg]

If by "formatting" you mean the layout of the input source file, then this bug 
should be marked "wontfix". From day one we've been clear that formatting of the 
input would NOT be preserved (unless you're in viewsource). 

Comment 7

[rickg]

Marking wont fix.

Comment 8

[Akkana Peck]

From day one, we've been telling people we'd preserve their html formatting, and
you've been telling us that we could.  And we could (at least in the body),
until this very recent regression.  Why the change?  And what does view source
have to do with this?  (Unless you're talking about browser-only view source
which re-fetches the document.)

I can point to lots of other bugs, and many person-weeks of effort, which have
been spent already on preserving the user's html formatting.  If we're going to
go back on that now, it sure would have saved a lot of time to have made that
decision up front (and posted it to the net so people would have a chance to
yell about it, since that was one of our biggest complaints about old composer).

Comment 9

[rickg]

Not true -- we've never EVER claimed to support source formatting. You and I 
have personally discussed that the gecko position is that we can (and may) do 
whatever is necessary to correct the markup we're given in compatibility mode 
documents. I couldn't have said this more times -- to you and other folks on the 
editor team. The most we ever agreed to was to not go out of our way to strip 
character data that serves this purpose. 

To that issue: I don't know what may have occurred to affect this (few changes 
have been made to the compatibility DTD recently). But I do know that this is 
not the highest priority issue given our current buglist.

Comment 10

[Dawn Endico]

Source formatting is extremely important for collaborative projects. If
someone edits my file I don't want their editor reformatting everything
and removing stuff it thinks is invalid. If the editor reformats the 
document it makes 'diff' completely useless. On www.mozilla.org we keep
track of changes in our documents with bonsai and cvs diff. For me, this
make the mozilla editor completely useless. Its a shame to ask people not
to use mozilla to edit mozilla documentation.

Comment 11

[Akkana Peck]

Reopening based on the flood of comments we've gotten on this issue.  Sounds
like we need to escalate this and get an official decision.

Comment 12

[harishd]

Without formatting it looks very strange.

Comment 13

[harishd]

Building up content-model for whitespace ( other than PRE ) would pose a huge 
performance problem. 

Comment 14

[Hixie (not reading bugmail)]

harish: ??? Surely we have to do this anyway. We cannot know if the author 
is going to do something like this:

   TABLE { display: block; white-space: pre; }

...and thus force us to take the whitespace into account.

I'm confused about this bug. If it is a feature of the Transitional or Strict
DTDs that we strip whitespace where the DTD says it cannot exist, then according
to the CSS specs that is wrong, because it means we are no longer honouring the
content of the original document with a "white-space:pre" case like above.

(However, the HTML spec would tend to support the behaviour, since whitespace is
not meaningful in those parts.)

In XML, we should *always* be keeping white space, so the issue in XML doesn't
come up. Right?

Comment 15

[Akkana Peck]

We always retained whitespace until about a week ago; was there a performance
problem with the old code before this regression was introduced?

Comment 16

[harishd]

I see the same behavior in PR1! 

Comment 17

[harishd]

CCing Chris Karanaze, Chris Waterson, and David Baron.

Comment 18

[rickg]

There are two issues here: one is how the HTML source file is laid out. The 
other is how the gecko engine interprets the content model. The parser does it's 
best to build the content model with the hightest fidelity. The parser knows 
it's can't interpret newlines, whitespace, etc., given that CSS is free to 
interpret this later.  For example, one of the few things that parsing engine 
will do is correct quotes around attributes. 

Akkana is concerned that when html markup is viewed in the editor, the results 
don't perfectly match the input source from the original html source. There can 
be several reasons for the difference:

1) the parser changed something
2) the DTD's changed something
3) the content model changed something 
4) the routines in the editor that display the source changed something

One test that can be applied is how the source looks in our viewsource window 
(as opposed to the editor's source view). If the main viewsource window looks 
similar to the markup, then the parser is off the hook. If viewsource looks ok, 
but the editor source window doesn't, I'd suspect either the content model or 
the source display routines in the editor.

Akkana -- have you done this test?

Also -- I'm removing the term "regression" because: 1) as harish points out, 
this hasn't changed since PR1, and 2) as I understand it, this is a NEW feature
in the editor.

Comment 19

[harishd]

Rickg, currently NavDTD omits ( kOmitWS ) whitespace inside TABLE.  This was 
done so because at one point the TABLE code was not ready for WS handling..and 
also there were issues about wasteful frame constructions ( leading to 
performance degradation ). It would be a trivial task for the DTD to pass on 
WS..however the main problem should be addressed in the layout.

BTW, we ( Karnaze,Waterson,David,Akkana, and I ) should probabaly have a meeting 
sometime next week to resolve the issue.

Comment 20

[harishd]

Nominating for beta3.  

Comment 21

[Charles Manske]

Just in case it wasn't noticed, the news group thread on this topic 
yielded universal agreement that this should be fixed.
The consensus is "Composer is useless" if it doesn't preserve user's formating,
including inside of tables.

Comment 22

[rickg]

Charles is overselling his case, but I did notice strong support from the folks 
who bothered to reply. Harish: there's no reason to strip ws from tables anymore 
(AFAIK); I like your suggestion to open it up.

Comment 23

[harishd]

Increasing priority to P1.

Comment 24

[harishd]

Attachment: A trivial patch..

Comment 25

[harishd]

We should be using IsOnlyWhitespace() method, found in nsITextContent, to 
determine if the node is whitespace or not.  I don't think any other additional 
bit is necessary.

Comment 26

[Nisheeth Ranjan]

Marking nsbeta3+...

Comment 27

[harishd]

I've checked in the patch. Reassigning bug to karnaze to make sure that we don't 
,unnecessarily, create frames for whitespace and therefore degrade the 
performance.

Comment 28

[leger]

Low visibility functionality/polish. Moving to P3.  Adding [PDTP3]

Comment 29

[Akkana Peck]

The whitespace still isn't making it into the document.  Harish, is the code you
checked in disabled until Chris checks it, or something?

Comment 30

[karnaze (gone)]

Reassigning to Peterl.

Comment 31

[Peter Lubczynski]

The patch doesn't seem to save all the whitespace in tables. The following test 
case shows that the newlines between TR and TD tags are not making it into the 
content model. 

Whitepsace in the BODY and TD is making it in the content tree and frames are 
being created for it. 

back to Harish

Comment 32

[Peter Lubczynski]

Attachment: testcase with whitespace between TR and TD

Comment 33

[Akkana Peck]

I've been seeing missing newlines, too.  Thanks for pinning down where!

Comment 34

[harishd]

Hmmm...wonder how my patch did not work! Darn. Will look into this. Thanx Peter 
for the testcase.

Comment 35

[rickg]

harish: the simple fix is to add comment, newline and whitespace as special 
children of table, tr, tbody, th. Do you want me to fix this?

Comment 36

[harishd]

Attachment: Revised patch

Comment 37

[harishd]

Okay, this time, the fix is in, for real!!!!!

Back to Peter.

Comment 38

[Peter Lubczynski]

Oh, this looks pretty good. Whitespace in tables is now being saved in the 
content tree. And it doesn't look like frames are being created inside the table 
for whitespace either. 

Load the testcase below into the editor. I already ran it once through the 
editor so that it would do all the fix-ups. Do a Debug->Output HTML and compare 
it to the orriginal. Notice the formatting in the TABLE is preserved. In theory, 
if no changes are made in the editor, the output should be the same as the 
input.

There is one quirk I noticed, however. The TITLE tag source seems to have been 
reformatted. However, the SCRIPT tag inside the HEAD seems to be fine. I don't 
know if this is being caused by the Editor fixing the markup or the parser 
because the content tree only contains the body. 

Also, my Edit Source window is blank in today's build but all of my chrome is 
having problems today. Can anyone else verify?

Back over to the editor folks to make sure this is what they want.

Comment 39

[Peter Lubczynski]

Attachment: document with whitespace formatting

Comment 40

[Peter Lubczynski]

reassign to editor group for a look

Comment 41

[rubydoo123]

assigning to akkana for review

Comment 42

[Akkana Peck]

Title and Edit Source are unrelated -- you should file separate bugs on those,
if they aren't already filed (I think they might be -- cmanske and vidur have
been talking about some problem involving title and other stuff in the head).

Is there some remaining problem with table formatting?  (I haven't tested the
fix yet myself, but will believe you if you say Harish's fix did the trick.) 
I'm not sure why I have this bug now.

Comment 43

[rubydoo123]

it seems to me then, that this needs to be marked fixed so it can be tested. 
Peter?

Comment 44

[Peter Lubczynski]

marking FIXED

Comment 45

[harishd]

This is not FIXED. TABLE code still needs to handle whitespace/newlines. 
Reopening the bug. This is a must fix.

Comment 46

[harishd]

Reopening...

Comment 47

[harishd]

Peterl, I have backed out the patch that I checked in because of bug 52443. I 
will reenable my changes once the table code is ready to deal with 
newlines/whitespace inside TABLE,TR,THEAD,TFOOT,TBODY. To reproduce the problem 
refer to  bug 52443. I'll talk to Bob Lord/David Drinana, PSM folks, in 
attaching a reduced test case.

CCing Bob, David, and myself.

Comment 48

[karnaze (gone)]

The 2nd attachment in bug 52598 is a test case for this bug. The fix I have for 
52598 deals with unexpected Text frames inside the row group frame. Those text 
frames should probably not be there.

Comment 49

[karnaze (gone)]

Reassigning to myself. I have a fix which does not create any whitespace text 
frames inside table related frames (td being an exception). It also fixes bug 
52443 (when Harish's patch is applied).

Comment 50

[karnaze (gone)]

Harish, I've checked in my fix and now it is up to you to check yours back in.

Comment 51

[harishd]

Enabled my change. Marking FIXED.

Comment 52

[sujay]

verified in 9/20 build.