Closed Bug 380668 Opened 18 years ago Closed 17 years ago

Extra newlines added after a meta element that already has them before and after (line breaks, LF)

Categories

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

Product:
Core
Component:
DOM: Serializers
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: Aleksej, Assigned: sicking)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Example.
8.58 KB, image/png
Details
Above patch that compiles.
3.79 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
Build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070514 Minefield/3.0a5pre 1. Load attachment 264780 [details] 2. Save as Web Page, complete. 3. Compare the source code. Actual results: The saved file contains: - two empty lines after the <meta> tag. - an empty line before </body> Expected results: No empty lines where there are none in the original.
This looks like a regression from bug 331991 -- we skip the existing <meta>, but not the newline after it, and add our own newline after the <meta> we introduce. When we skip a <meta> we should probably skip the following newline if any. Requesting blocking, since this breaks editor roundtripping...
Blocks: 331991
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
Not blocking on this since the only whitespace added is in the head so it's not that big of a deal. Also, it's only the first roundtrip that fails, not subsequent ones if you roundtrip the same document multiple times.
Flags: blocking1.9? → blocking1.9-
Keywords: regression
Whiteboard: [wanted-1.9]
Actually, every single roundtrip adds a newline, given the way the patch in bug 331991 was written. If it were just the one time I wouldn't have requested blocking... re-requesting given that misunderstanding.
Flags: blocking1.9- → blocking1.9?
Ryan, this is a regression from bug 331991, can you take a look at it? Would be great if you also fixed the newlines-after-hidden-meta problem
Assignee: dom-to-text → sciguyryan
Flags: blocking1.9? → blocking1.9+
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Attached patch Patch v1 (obsolete) — — Splinter Review
Patch v1 This was a great idea suggested by Peterv, it avoids the meta tag skipping and so basically voids the problem to some degree.
Attachment #266056 - Flags: superreview?(bzbarsky)
Attachment #266056 - Flags: review?(bzbarsky)
This also includes the fix for bug 380659. (the meta tag charset stuff)
Flags: in-testsuite?
Comment on attachment 266056 [details] [diff] [review] Patch v1 The problem is that this puts the <meta> at the end of the <head>. After all the scripts and stylesheets. And quite possibly too far into the document for the parser to pick it up and make use of it. The whole reason we did the skipping in the first place was to ensure that the <meta> is the first thing in the head (which is needs to be). I suppose you could see whether the node passed in when serializing the head start has a meta child of the right sort and if so hold off on putting in your own meta and then modify the child in-place when it comes through... but any meta you add _must_ come at the beginning of <head>.
Attachment #266056 - Flags: superreview?(bzbarsky)
Attachment #266056 - Flags: superreview-
Attachment #266056 - Flags: review?(bzbarsky)
Attachment #266056 - Flags: review-
Attached patch Patch v2 (obsolete) — — Splinter Review
Path v2 This is the more simple approach and should correctly fix the problem here. If anyone could test this for me I'd appreciate it because I currently do not have a working build tree (not my computer as my internet connection isn't working).
Attachment #266056 - Attachment is obsolete: true
Comment on attachment 266930 [details] [diff] [review] Patch v2 This should fix it so I'm going to request a review from bz.
Attachment #266930 - Flags: superreview?(bzbarsky)
Attachment #266930 - Flags: review?(bzbarsky)
Comment on attachment 266930 [details] [diff] [review] Patch v2 s? And that you will check in regression tests to this effect. Please do both. If you need help with one or the other, ping people on IRC or whatnot as needed. r+sr=bzbarsky with the above issues addressed.
Attachment #266930 - Flags: superreview?(bzbarsky)
Attachment #266930 - Flags: superreview+
Attachment #266930 - Flags: review?(bzbarsky)
Attachment #266930 - Flags: review+
I meant: I assume that you have checked that this in fact does fix the problems? And that you will check ...
(In reply to comment #11) > I meant: > > I assume that you have checked that this in fact does fix the problems? And > that you will check ... > This patch seems to fix the problem yes but its still not quite consistent with the line break results generated in Firefox 2 but I'm not sure if that matters.
Whiteboard: [wanted-1.9] → [wanted-1.9] [checkin needed]
"Patch v2" checked in. Clearing checkin-needed status.
Whiteboard: [wanted-1.9] [checkin needed] → [wanted-1.9]
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070618 Minefield/3.0a6pre - now only one empty line is added after <meta> with attachment in comment #0, not two.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Any reason why this was re-opened? As far as I can tell this is doing what it should be now. ------ <html><head> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> </head><body> ® </body></html> ------ Is what I get and that seems OK (plus it doesn't break editor round tripping).
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a6pre) Gecko/20070620 Minefield/3.0a6pre Saved and viewed with its own View Source: ------ <html><head> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> </head><body> ® </body></html> ------
Attached image Example. —
This is saves with save as HTML. I don't know why there is a difference here and so I have no clue as to a fix.
(In reply to comment #17) > Created an attachment (id=269075) [details] > Example. > > This is saves with save as HTML. Hmm, also WFM with save as HTML. Comment #16 is Web Page, complete.
If anyone has any clue what the difference is here so I can attempt a fix please post. Otherwise I'm not sure what to do from here on in.
IIRC Save as HTML simply saves what we got from the network. Save as Web Page Complete is what actually serializes the current DOM. Is that not the case?
So the problem here is that we're appending the <head>, then a newline for the <meta> we plan to add, then the <meta>.... then the existing two newlines in the source (the one after <head> and the one after <meta>). Ideally, we would modify the existing <meta> tag if the document has one and add one if it doesn't, only adding newlines around it in the latter case. What would it take to make that possible?
IMHO this still isn't right. The easy way to check this is to open SeaMonkey Composer and view source. Before: <head> <meta ... After: <head> <meta ...
Ryan, what you need to do here is to reuse any existing <meta http-equiv="content-type" ...> elements if there are any, rather than always inserting a new one and attempting to remove the old one. So i'd simply search through all the children of the <head> and if there is such an element, not insert the new <meta>, but instead change its serialization when it's serialized.
Ryan, are you going to be able to get to this? Otherwise we might want to back out the original patch in bug 331991
Priority: -- → P4
Ryan, any updates? It'd suck if we had to back out the original patch :(
Let us know if there is any way we can help btw. I'll be on irc until tuesday.
Attached patch Uncompiled patch (obsolete) — — Splinter Review
This *should* do it. However it's completely untested, not even compiled.
Assignee: sciguyryan → jonas
Attachment #266930 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #293822 - Flags: superreview?(jst)
Attachment #293822 - Flags: review?(jst)
Attachment #269075 - Attachment is patch: false
Attachment #269075 - Attachment mime type: text/plain → image/png
This is Jonas' patch with a few tweaks to make it compile (and get the attribute off of the child rather than the content node in the new code in AppendElementStart). With this patch the test case mentioned in comment 0 works as expected.
Attachment #293822 - Attachment is obsolete: true
Attachment #294793 - Flags: superreview+
Attachment #294793 - Flags: review+
Attachment #293822 - Flags: superreview?(jst)
Attachment #293822 - Flags: review?(jst)
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED
Seems to WFM using the testcase from comment #0 with Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b3pre) Gecko/2008010504 Minefield/3.0b3pre Verifying (uncertainly).
Status: RESOLVED → VERIFIED
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Depends on: 431233
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: