Closed Bug 87894 Opened 24 years ago Closed 23 years ago

Leading and trailing whitespace should be trimmed from IMG src attribute value.

Categories

(Core :: DOM: HTML Parser, defect, P2)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: attinasi, Assigned: harishd)

References

(
URL
)

Details

(Keywords: compat, topembed+, Whiteboard: [bugscape 14779] [adt2 RTM] [TOPSITES][fixed on the trunk 06/11 and branch 06/18])

Attachments

(3 files, 2 obsolete files)

mozilla-banner
4.41 KB, image/gif
Details
testcase
109 bytes, text/html
Details
patch v1.2
5.97 KB, patch
john
: review+
jst
: superreview+
jud
: approval+
Details | Diff | Splinter Review
Spin-off from bug 87298 and bug 79929, both of which document specific cases of whitespace handling in URLs. There is a lot of relevant information and even patches in those two bugs, including RFC quotes, testcases, and test URLs.
CC'd pollmann and dougt since they both had patches for this in the other bugs
2 me
Assignee: neeti → dougt
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3
*** Bug 83603 has been marked as a duplicate of this bug. ***
Over to vishy@netscape.com. He has 87298 and 23485 "whitespace" bugs.
.
Assignee: dougt → vishy
Status: ASSIGNED → NEW
-> alecf
Assignee: vishy → alecf
Target Milestone: mozilla0.9.3 → mozilla1.0.1
*** Bug 92555 has been marked as a duplicate of this bug. ***
*** Bug 95794 has been marked as a duplicate of this bug. ***
This is properly a parser bug. Reassigning.
Assignee: alecf → harishd
Component: Networking → Parser
QA Contact: benc → moied
Summary: Whitespace (at least leading and trailing) should be trimmed from URLs in QuirksMode → Leading and trailing whitespace should be trimmed from CDATA attributes in quirks mode.
*** Bug 95754 has been marked as a duplicate of this bug. ***
I don't think the parser messes with attrubtes at all. More likely, the content elements themselves have to do this, as they understand the semantics of the attribute values. I'm not sure who would own this though...
Related bugs for whitespace stripping, Bug 101087 Marked dup of 95754 and bug 95754 is marked dup of this bug,
whitespace trimming must be done in HTML mode only. In XML mode whitespace must nut be trimmed. In HTML mode it must be trimmed.
*** Bug 130124 has been marked as a duplicate of this bug. ***
Keywords: compat
*** Bug 133536 has been marked as a duplicate of this bug. ***
http://bugscape/show_bug.cgi?id=14779 tracks top sites which are affected by this bug. What are the changes of getting this fixed for RTM and 1.0.1?
Keywords: nsbeta1
Whiteboard: [bugscape 14779]
Please note that as per the title, this should be a quirks-mode-only change. While the three sites listed in this bug are quirks, any of the bugscape sites using standards mode should be evangelized.
*** Bug 148474 has been marked as a duplicate of this bug. ***
Changed to P2 major.
Severity: normal → major
Priority: -- → P2
[adt2 RTM] as this issue is effecting top sites.
Blocks: 143047
Whiteboard: [bugscape 14779] → [bugscape 14779] [adt2 RTM] [TOPSITES]
After investigating the bug I belive that this bug should in fact be fixed in |layout|. Reasons are: 1) We can target the fix to a specific attribute. Whereas in parser it would be a more generic fix and therefore could have an impact on the performance. 2) Setting attribute values via DOM will benifit from layout fix. That is, if the bug is fixed in the parser then attribute values, with leading/trailing whitespace, set via DOM will exhibit the same bug. 3) With a layout fix we don't need additional code to preserve view-source.
Attached patch patch v1.0 (obsolete) — — Splinter Review
Strip leading/trailing space characters, in IMG src, in addition to stripping \n\r\t.
Attached file testcase —
Comment on attachment 86333 [details] [diff] [review] patch v1.0 Since speed is really critical in the sink where this new method is now used I'd suggest changing the aSet argument to a simple const char pointer in stead of a const nsAString&. Make that change and I'll have one more look.
Attachment #86333 - Flags: needs-work+
Attached patch patch v1.1 (obsolete) — — Splinter Review
Addresses jst's concern.
Attachment #86333 - Attachment is obsolete: true
Attached patch patch v1.2 — — Splinter Review
Minor optimization
Attachment #86754 - Attachment is obsolete: true
Comment on attachment 86846 [details] [diff] [review] patch v1.2 sr=jst
Attachment #86846 - Flags: superreview+
Comment on attachment 86846 [details] [diff] [review] patch v1.2 r=jkeiser
Attachment #86846 - Flags: review+
Fix landed ( 06/11 ) on the trunk.
Status: NEW → ASSIGNED
Whiteboard: [bugscape 14779] [adt2 RTM] [TOPSITES] → [bugscape 14779] [adt2 RTM] [TOPSITES][fixed on the trunk 06/11]
Keywords: adt1.0.1
moied, can you verify this on the trunk?
Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I can verify using 2002-06-13-08/win2k trunk that the test case and the three sites http://mimimaternity.com/collection.asp?category_Name=Dresses+%2D+Spring&category_Id=3045&PageIsOn=1 http://www.atlantic-records.com http://www.metlife.com/ now show the missing images. atlantic still has some differences with respect ot IE however they are due to nav4/ie4 coding and are evang fodder. vtrunk
Marking VERIFIED per bclary's comment ( #36 ).
Status: RESOLVED → VERIFIED
Changed summary to reflect the specific problem.
Summary: Leading and trailing whitespace should be trimmed from CDATA attributes in quirks mode. → Leading and trailing whitespace should be trimmed from IMG src attribute value.
adding adt1.0.1+. Please get drivers approval before checking in.
Keywords: adt1.0.1 → adt1.0.1+
Attachment #86846 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Fix landed ( 06/18 ) on the branch.
Keywords: fixed1.0.1
Whiteboard: [bugscape 14779] [adt2 RTM] [TOPSITES][fixed on the trunk 06/11] → [bugscape 14779] [adt2 RTM] [TOPSITES][fixed on the trunk 06/11 and branch 06/18]
Added topembed+ per Susie's suggestion - just so any Bugzilla queries looking for "topembed+" can pick this one up.
Keywords: topembed → topembed+
verified fixed with branch build 20020619 using winxp
Keywords: mozilla1.0.1+
*** Bug 126352 has been marked as a duplicate of this bug. ***
Keywords: verified1.0.1
*** Bug 122728 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: