81935 - harbortides.com - JS doesn't ignore ---> in HTML (requires //--> or -->)

Status: RESOLVED WORKSFORME

(Tech Evangelism Graveyard :: English US, enhancement)

Description

[Chris Waterson]

To reproduce the problem:

1. Go to above URL
2. Enter a zip code in the pink area, and click go.

Expected: form submits
Actual: nothing happens

Note that the form works if you use the topmost entryfield, but either of the
entryfields in the pink ``Start Here'' area don't work.

Comment 1

[rods (gone)]

reassigning

Comment 2

[Christopher Blizzard (:blizzard)]

You get this in the JS console:

Error: search is not defined

and there's a function that looks OK:

<script language="javascript">
<!---
function search()
{
	document.form1.submit()
}

[...]

but the way that it's handled in the element is really strange.  Is this
expected to work?  That big JAVASCRIPT: just screams at me as the source for all
of our pain.

<td valign="top"><input type="text" name="searchfor" size="15">&nbsp;<input
type="button" value="Go" name="go" onclick="JAVASCRIPT:search()"></td>

Comment 3

[Christopher Blizzard (:blizzard)]

restore summary since it was somehow deleted in my last note in this bug

Comment 4

[hirata masakazu]

I think I am seeing this at 
http://www.ncbi.nlm.nih.gov/entrez/query.fcgi?CMD=Index&DB=PubMed .
If I type a word in the text area at the bottom and hit Preview, the word in the
text area above is replaced by "PubMed" which is an item in the drop down list
of the top box. Mac trunk 2001052804.
correcting manually and hitting Preview again returns a strange result, showing
some form header.
There are lots of weird things going on with forms recently. 

Comment 5

[Travis Freeland]

This appears to be fixed in the last couple of days build.

Comment 6

[basic]

Note that when the page loads this appears in the javascript console

Error: syntax error
Source File: http://www.harbortides.com/
Line: 34, Column: 2
Source Code:
---->

Comment 7

[basic]

The ----> at the end of the script block was causing problems. Mozilla's js
allows //--> and --> at the end of the script but not --->, ----> or more than 2
'-' before a '>' on the last line.

Comment 8

[basic]

Attachment: simplified testcase

Comment 9

[basic]

attachment 37338 [details]

'TEST --->' button calls a js function defined in a script block ending with --->
'TEST //-->' button calls a js function defined in a script block ending with //-->

Both functions calls alert("Success!!");

Comment 10

[basic]

Forgot to mentioned that I tested this bug using build 2001060504 win32 sea
installer talkback trunk

Comment 11

[Johnny Stenback (:jst)]

'--->' is an invalid comment termainator in HTML, so IMO we have two options
here, reassign this bug to evangelism and have the site fixed and leave mozilla
as is, or fix the JS scanner (jsscan.c, line ~1200) to cope with this broken
HTML. Does IE, NS4.x handle this broken HTML "correctly"?

Comment 12

[Eric Pollmann]

Yes, IE handles ignores the -->.  Handing this to JS folks, CC'ing Harish.

Comment 13

[Phil Schwartau]

cc'ing beard, rginda - 

Comment 14

[Phil Schwartau]

The valid HTML comment-closer is of course '//-->'

Compare bug 31255, where we modified jsscan.c to accept '-->' as well. 
That doesn't seem to cover '--->', as [e-mail]'s testcase shows.

For completeness I will attach a version of the testcase employing 
all three of these. As above, click on each button and watch the 
JavaScript console for errors. The buttons using '//-->' and '-->'
succeed; the button using '--->' fails.

Comment 15

[Phil Schwartau]

Attachment: simplified testcase using all three : //-->, -->, --->

Comment 16

[Phil Schwartau]

Updating summary -

Comment 17

[Phil Schwartau]

Wait - doesn't the HTML parser see the JS before the JS parser does?

Then perhaps this bug is a duplicate of bug 79139, which states that 
strict HTML mode DOES NOT ALLOW extraneous '-' characters in HTML comments ...

If that is the case, this bug may be invalid. However, there is always
the issue of how IE treats '--->', and whether we should follow suit.
I will look into that now.

Comment 18

[Phil Schwartau]

Attachment: simplified testcase, corrected to work properly in IE

Comment 19

[Phil Schwartau]

The above testase (id=37690) works without problem in IE4.7
In Mozilla, again only the buttons using '//-->' and '-->' succeed.

I'm guessing this is an issue for the HTML parser to decide, 
and is not a JS Engine issue. 

Opinions?

Comment 20

[Johnny Stenback (:jst)]

This is a JS engine issue (since it already deals with HTML comment
termainators), if we're gonna fix this we should either make the JS scanner deal
with these broken comment terminators, or we should re-think the whole HTML
comment handling in <script> tags all together, something I'm sure we don't
wanto do at this point.

Comment 21

[rogerl (gone)]

I'm guessing we need to go to complete IE compatibility or else get evangelism 
to be proactive about fixing the offending sites? In any case, it doesn't feel 
critical for the short term.

Comment 22

[Kenton Hanson (gone)]

Does this really need to be fixed by 1.0 or can I move it to future.

Comment 23

[Johnny Stenback (:jst)]

If this isn't too hard to fix I think it's worth pushing for mozilla1.0 (if not,
we might as well not fix it at all), it'll make more sites work as expected in
mozilla for a fairly low cost (assuming it's easy to fix), and that will take
some of the load of our evangelist's already way overloaded sholders.

Seems like it shouldn't be too hard, so I'd say fix this for mozilla1.0.

Comment 24

[Kenton Hanson (gone)]

I'll stay with the 1.0 schedule.

Comment 25

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 26

[.Ulli]

A nearly unreadable test page:
http://www.the-nations.com/index1.htm
Within the center News-frame the font color should be white and not black.

Would you please try these steps:
1. on the above url open the print preview
2. the page seams garbled but just above the News you see a part of the html
denoting css
3. scroll to page 2 - there is html coding too

You may inspect the ignored css:
1. close the print preview
2. save the whole page
3. within the folder index1_files open the file content.html with a normal editor
4. on line 5 there is ... //-->sheet" href="../Styles/styles.css">

My Version:
  Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+) Gecko/20020405

Comment 27

[Phil Schwartau]

I just tried http://www.the-nations.com/index1.htm with Mozilla 
trunk builds 20020406xx WinNT, and 20020407xx Linux. 

The font color in the central frame was white - no problems at all.
The site looks the same in Mozilla as it does in NN4.7 and IE6.

The Print Preview function in Mozilla does not show any garbling.
The only issue there is the <body> tag which is visible after the
content, when it shouldn't be. But that is another matter.

This site does not seem to bear on the current bug, which is about
the invalid comment terminator '--->' (note the three dashes).

Comment 28

[Bob Clary [:bc] (inactive)]

Since I was cc'd here is my opinion. I have seen this in the wild but not that
often. The primary pain from my point of view was the mangling of the source
when saving pages as described in comment 26. I would like that to be fixed but
that is not the particular issue described here. If you are considering changing
our behavior to mimic IE, then you should do it completely and not for single
test cases as this. What does IE do in general for invalid tokens in JS?
Otherwise, this kind of error can be caught by an HTML validator which everyone
should use as a part of their QA process.

Comment 29

[Kenton Hanson (gone)]

We need to decide to either declare this an invalid bug or provide a patch to
mimic IE's behavior (see comment #11)

Comment 30

[Johnny Stenback (:jst)]

Has someone reverse engineered how IE deals with stuff like this? I.e. what are
the HTML comment terminators that IE recognizes in scripts? Bob, do you have any
ideas about this?

Comment 31

[Bob Clary [:bc] (inactive)]

We appear to collect the content between the opening <script> and ending
</script> and place it all into a #text node as a child of the script element.

IE appears to collect the same content however does not expose it as a child of
the script element but as a property (.text) of the script element itself.

Both Mozilla and IE6 agree on the content of an HTML comment such as:

<!--  var foo = 'bar'; ------>

outside of a script, e.g.  var foo = 'bar'; ----. However, it appears that IE
judges the end of the script contained in such a comment to not include the
trailing ----. Changing to <!--  var foo = 'bar'; ++++--> or even <!--  var foo
= 'bar'; { --> still does not generate an error in IE. My guess is that it
ignores any parse errors in the last statement of a script. Whether you want to
emulate that totally psychopathic behavior of some demented IE programmer is
your call. ;-)

Comment 32

[Boris Zbarsky [:bzbarsky]]

*** Bug 175171 has been marked as a duplicate of this bug. ***

Comment 33

[Boris Zbarsky [:bzbarsky]]

*** Bug 178711 has been marked as a duplicate of this bug. ***

Comment 34

[Brendan Eich [:brendan]]

Why shouldn't this bug be WONTFIXed?  Or morphed into an evang bug against
harbortides.com and whatever other sites rely on the creepy IE behavior?

/be

Comment 35

[Phil Schwartau]

Note this bug is a special case of bug 175171:
JS handling of non-whitespace chars + "-->" on same line

which was reported from a site using the following line:
/* ]]> */ --> 


If there is a taste for fixing this bug, we might hold it open
despite bug 175171, as a special case that is more easily fixed.

On the other hand, if we WONTFIX this bug, we should probably 
WONTFIX bug 175171, also -

Comment 36

[Jon Kahlo]

Can Evangelism take this? This was found by a co-worker of mine and he thinks
the problem is with Mozilla. Maybe there should be an "Almost Standards" mode
for JavaScript. :-)

Comment 37

[Bob Clary [:bc] (inactive)]

evangelism can take it however it is up to the engineers to decide if Mozilla
should be changed or not and on what branches. If Mozilla is to be fixed then it
should be on the long running 1.0.x branch as well as the trunk.

Comment 38

[Brendan Eich [:brendan]]

Ignoring any syntax error in the last statement of a script is not "almost
compatible" with ECMA-262 -- it's completely bogus, unnecessary, and a
disservice to web content developers.  I hope we never have to imitate such a hack.

Over to evangelism for now, bounce back if too many major sites with
intransigent webmasters/content-authors depend on this.

/be

Comment 39

[Bob Clary [:bc] (inactive)]

Ok, this is evang fodder now. I suggest that unless you want "stoopid evang
spam" you remove yourselves from the cc list NOW.

To be consistent, please move bug 175171 to tech evang as well.

Comment 40

[Phil Schwartau]

Setting default QA -

Comment 41

[basic]

I tried emailing their hotmail account but got a failure notice.

Comment 42

[basic]

futuring and lowering severity to enhancement. The forms on the site basically
work except for the two buttons on the front page (if one press return after
entering in the text fields it submits).

Comment 43

[Bob Clary [:bc] (inactive)]

tech evang june 2003 reorg

Comment 44

[Pascal Chevrel (relman team) -> :pascalc]

"No web site is configured at this address."
-> WFM