Closed Bug 106586 Opened 23 years ago Closed 23 years ago

Inappropriate source into the plaintext copy.

Categories

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

Product:
Core
Component:
DOM: Serializers
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
CLOSED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: moied, Assigned: peterv)

References

Details

(Keywords: topembed, Whiteboard: [fixed on trunkl])

Attachments

(3 files)

Testcase
731 bytes, text/html
Details
Furhter reduced
398 bytes, text/html
Details
Proposed patch
6.16 KB, patch
hjtoi-bugzilla
: review+
jst
: superreview+
Details | Diff | Splinter Review
BuildID: 2001-10-22-18-0.9.4 OS: ALL This bug report was spun off from bug 97687 Inappropriate source into the plaintext copy. Steps to reproduce: Goto this URL http://www.cnn.com/2001/WORLD/asiapcf/central/09/24/ret.binladen.message/ Using NS6.2, make a selection starting from the end of the word Kabul in the second paragraph: "The fax was sent to the Qatari-based news channel Al Jazeera at its offices in Kabul." And drag selection up and to the left so it includes the entire document up to that point...Then paste the plaintext content into windows notepad and, in the middle of my text output I get the following: Actual result: Inappropriate source inserted in the plaintext copy. EDUCATION CAREER IN-DEPTH <script></script> QUICK NEWS LOCAL COMMUNITY Expected result: it should eliminate any source in the plaintext copy. EDUCATION CAREER IN-DEPTH QUICK NEWS LOCAL COMMUNITY
Keywords: nsbranch+, topembed
Blocks: 107066
Harish, this looks like a problem in the parser. The parser passes a textnode to nsPlainTextSerializer::AddLeaf(const nsIParserNode& aNode) and the node has value "<script></script>".
Assignee: peterv → harishd
--> 0.9.6
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
moied: I'm not sure if the actual page has changed or not. It's a good idea to attach a copy of the problem page rather than relying on the dynamic page. Also could you please attach a reduced testcase. Thanx.
Attached file Testcase —
Attached file Furhter reduced —
peter: it looks like we may have to ignore IFRAME content in the plaintextserializer. Try this testcase: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> </head> <body bgcolor="#FFFFFF"> Before <iframe width=20 height=15><script></script></iframe> After </body> </html>
Also, we may have to implement GetPref() in nsPlainTextSerializer. Right now we always assume that FRAMES & SCRIPT are enabled.
Back to Peter. IFRAME content should be ignored in the plaintext serializer. Thanx peter.
Assignee: harishd → peterv
Status: ASSIGNED → NEW
This is a lot harder than I thought. The plaintext serializer kicks in after the HTML has been serialized, and it uses the output of the HTML serializer. The problem is that we probably want both frames and noframes/iframes content in the HTML copy, but the plaintext one should take one or the other depending on whether frames is enabled. By the time the text serializer is called, we can't determine whether frames are enabled or not.
Status: NEW → ASSIGNED
Attaching a patch, it's not perfect since we should do the frames check before calling the serializer and pass in the flag, but it works. I think we should check this in and leave the bug open to fix the copy code (that calls the serializer).
Component: Parser → DOM to Text Conversion
Attached patch Proposed patch — — Splinter Review
Comment on attachment 57472 [details] [diff] [review] Proposed patch What is the GetPref() function doing there, nobody is calling it? Also, you have a comment stating that we should have the caller pass in the OutputNoFramesContent flag; why not just do that instead of the hack in the patch?
Heikki: the parser calls GetPref. The copy code serializes to HTML, parses that and feeds the output from the parser to the PlainTextSerializer (which is a content sink). I haven't figured out exactly where in the copy code I need to put the check for frames enabled or not. I can find out now and pass in the flag from there, but it will take more time.
Heikki: GetPref would get called by CNavDTD.
Keywords: topembed
Comment on attachment 57472 [details] [diff] [review] Proposed patch Ok, in that case r=heikki.
Attachment #57472 - Flags: review+
Target Milestone: mozilla0.9.6 → mozilla0.9.7
The check for frames would need to happen in nsHTMLFormatConverter (mozilla/widget/src/xpwidgets/nsHTMLFormatConverter.cpp). We'll probably have to implement some flags system (on nsIFormatConverter?) and set the right flags from nsCopySupport which has access to the document.
Comment on attachment 57472 [details] [diff] [review] Proposed patch sr=jst
Attachment #57472 - Flags: superreview+
Checked in into the trunk. Do we want this on the 0.9.4 branch?
This bug used to have the topembed keyword, but Marek removed it for some reason. I don't see how this is different from all the other plaintext bugs which have all landed on the 0.9.4 branch (like script contents ending in copy or wrong newlines with PRE) so I am nominating this for 0.9.4.
Keywords: edt0.9.4
Whiteboard: [fixed on trunkl]
this bug doesn't appear to have the impact necessary to hit 0.9.4 at this stage.
Keywords: edt0.9.4 → edt0.9.4-
Fixed in the trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Verified fixed with build ID 20011221 on win2k
Status: RESOLVED → VERIFIED
Marking bug closed
Status: VERIFIED → CLOSED
Keywords: topembed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: