The subject should say 'asFunction' instead.

Here's the deal:
 - upon calling onClick, a WADefaultScriptGenerator is created (inherited from WAScriptGenerator).
 - the parameter to onClick is "anObject". (Weird, shouldn't this be a String? move on..)
 - the event function (WAScriptGenerator>>#event:do:on:) simply appends the "object" (onClick event handler) to the onClick attribute.

The problem is in WAHtmlAttributes - it just appends the object instead of 
making sure that there are no double quotes.

Here's a proposed fix, simply replacing double quotes with \" 

Name: Seaside-Core-as.694
Author: as
Time: 6 February 2011, 9:08:58 pm
UUID: 5450d506-8db5-414b-996a-b434ec532cea
Ancestors: Seaside-Core-pmm.693

Escaping double quotes inside WAHtmlAttributes.

My bad, escaping double quotes is non-standard.
Replaced double quotes with single quotes, and updated relevant test.


Name: Seaside-Core-as.695
Author: as
Time: 6 February 2011, 9:39:13 pm
UUID: f017aa59-fdf2-604e-84c2-776bfdab18c2
Ancestors: Seaside-Core-as.694

Replaced double quotes with single quotes replacement in WAHtmlAttributes

Name: Javascript-Tests-Core-as.59
Author: as
Time: 6 February 2011, 9:44:38 pm
UUID: 0531acb9-997a-a049-90ca-f2515d7a3ace
Ancestors: Javascript-Tests-Core-lr.58

Replaced " to single quotes for JSRenderingTest>>#testLogger

This however does have implications on nested single/double quotes, whether 
those inserted in event handlers, or ones inserted by the Javascript frameworks 
(jQuery, Prototype).

I have looked into the problem to some extent as shown in 
http://news.gmane.org/gmane.comp.lang.smalltalk.squeak.seaside/23694
I seems that the JQueryInstance constructor incorrectly changes the single 
quote to a double quote.

First, re: anObject/aString, unless I'm misunderstanding where you're looking, 
it's rarely a String object that gets passed into onClick: methods.

As for the change you are proposing, is changing double quotes to single quotes 
a good idea? What if I add the value 'alert("This isn't going to be pretty.")'?

Also, wouldn't it be better to store the original value and do any 
modifications inside #encodeOn:?

(I've cc:ed Lukas on this as he's definitely the expert on this stuff)

re anObject/aString - yes, it is supposed to be a String passed to 
WATagBrush>>#onClick:

The change I did is not complete since, as I mention, it does not handle 
internally-scoped quotes. There are actually 2 problems at hand:
1) Handling double quotes within attribute values. This is divided into:
  a) For general attributes, which are not Javascript events. Attributes may contain the double quotes entity (i.e. " )
  b) For Javascript events, where double quotes cannot be used and cannot be converted to " . Preceding double quotes with a backward-slash (\") is not possible since it is not a valid Javascript expression (unless contained within another double-quotes string).

2) Escaping internal single and double quotes within Javascript events. Here 
are a few test cases (included with a tag and attribute for completeness):
  (i)   <button onClick="alert("This is not good")">Go</button> (double quotes within alert must be single quotes )
  (ii)  <button onClick="window.status = "How 'bout internal quotes?"">Go</button> (double quotes cannot be used here... also need to escape internal single quote in case we're converting the double quotes into single quotes).
  (iii) <button onClick="prompt("Here's another \"awkward\" example")">Go</button> (double quotes cannot be used here again..)
  (iv) <button onClick="alert('Here\'s an internal single quote. Here\'s "double" quotes inside single quotes.')">Go</button> (this one is a valid example of the use of quotations)
Note: the last test case shows that double quotes can be inside Javascript 
event attribute, given that they are contained within another string and are 
escaped (i.e. " ).

Test cases (i) and (ii) suggest that we may need to go for a 
convention-over-code setting, stating that event handlers are not allowed to 
contain any double quotes. If they do, #encodeOn: will raise an exception, 
indicating that this should be fixed (code smells).

Since it involves standardizing the jQuery+Prototype libraries, this should be 
a design choice regarding quotation in general.
My suggestions:
 - General HTML tag brushes should convert double quotes to the " entity. The change I did should be modified accordingly. Julian - you are right, this should go on WAHtmlAttributes>>#encodeOn: , I didn't notice it.
 - Javascript event attributes should override this behavior, and handle the internally scoped double-quotes string values. This looks like it's happening on WAScriptGenerator>>#event:do:on:
 - Javascript libraries should be examined to see if they produce any strings that need escaping.

Lukas?


p.s.: personally, I don't like the use onXXX event handlers because I think 
events should be attached dynamically (using the Javascript libraries). However 
this calls for a serious wrapping of Javascript functionality, one which offers 
all-round Javascript capabilities straight from Seaside.

One post on the mailing list suggested we are already converting " to " inside 
attributes. Why is that not good enough here?

And, no, my point was that it is usually *not* a String being passed to 
#onClick:.

" is not valid javascript. How does converting double quotes to the " entity 
help?

re #onClick - yes you're right. Well I guess it should get an object, 
especially considering the fact that WAHtmlAttributes inserts the given object 
to the string stream using greaseString.

Well, surely the browser decodes entities in tag attributes before evaluating 
the JS content? The " isn't part of the JS, it's part of the encoding of an 
HTML attribute...

This certainly works fine in Firefox with no errors or warnings logged to the 
console:

<html>
<head>
<title>foo</title>
</head>
<body onload="javascript:alert("foo")">
</body>
</html>

I'm against the fix currently commited for two resons:
- It moves encoding from WAXmlEncoder to WAHtmlAttributes where IMHO it doesn't 
belong
- It makes WAHtmlAttributes deal with JavaScript which it shouldn't
- It breaks thins like
html heading
   title: 'He said "good morning"'

Yes, I know this is not very constructive without proposing a better solution.

The current "fix" is no fix at all, but it surely kicked the discussion on this.
I have another fix, will submit shortly.

Please ignore the previous fix.

The double quotes are converted to " , however they do not handle the case when 
there are nested quotes. Therefore the following doesn't work:

<html>
<head>
<title>foo</title>
</head>
<body onload="javascript:alert("These "quotes" are not compliant")">
</body>
</html>


This doesn't apply to general html attributes, where language syntax is not an 
issue.
This does apply to Javascript, where nested double quotes are not valid.
We can ignore this... or we can either: (i) raise an exception; (ii) handle 
nested quotes by adding a backslash.

Here's the working version of the above example using a backslash with an odd 
number of nested double quotes:
<body onload="alert("These \"q\"uot\"e\"s\" are compliant")">

Surely that is also addressed by the encoding we already do for attributes (as 
mentioned in Comment 12)? Has anybody been able to confirm that? It seems to 
more or less validate XHTML strict so the entities must be *valid* in the 
attribute and Firefox at very least decodes them. Do we even have a problem 
here?

I'm a bit confused by Comment 15. What are you suggesting the code was that 
created the output? If the programmer passed that whole thing in as a string, 
then that's their problem. We don't validate the HTML that they generate and we 
don't need to validate their JS either.

I can confirm that the encoding of double quotes is done right.
I don't think we have a problem, rather:
 - a peculiar case of double-quotes escaping.
 - a Halos source "issue" which shows regular double-quotes symbols ("), even though the actual source is " It's a feature of browsers, converting entities with actual symbols.

I have mistakenly thought this for an issue.
We can choose simply ignore this, revert and close the issue.

Ok, I moded Seaside-Core-as.695 and Seaside-Core-as.694 to 
http://www.squeaksource.com/Seaside29old.html which is our repository for 
"deleted" versions and reverted testLogger in Javascript-Tests-Core-as.60

Do we need to open a new issue for a bug in the halo source view, then? It 
seems like we should be encoding & into & to that it shows up properly...

There's no need.
The Halo is implemented as plain HTML (with span and CSS classes for each 
tag/attribute). Therefore the browser will always convert HTML entities to 
their representations. The only case where it doesn't is when the code is 
inside <pre>, which means changing the entire Halos implementation... 
completely unnecessary.

It would be sufficient to include a comment somewhere in the book about the 
fact that the Halos doesn't show the original HTML entities, since they are 
rendered by the browser.

Hmm.. perhaps I wasn't being clear.

If the actual source is:

<body onload="javascript:alert("foo")">

Then I think the halo source view should be outputting:

<body onload="javascript:alert(&quot;foo&quot;)">

(well, it also adds styling, but I'm ignoring that for the instant) I think 
that *should* cause the browser to display things correctly.

I imagine we must be encoding some entities there, since we output entities 
whenever we output HTML content, but maybe we're missing the & or something?

I think a solution needs to be cognizant of the quote nesting levels (double 
quote and single quotes) - handle internally-scoped quotes. In a project that 
described validation in the model and generates javascript for client and for 
script for server (server jscript or rhino). I parsed the JavaSript ( with 
JScript no a proper subset of EcmaScript). Appropriately escaped quotes can be 
generated even for cases like "('I said "hello" to fritz' aunt')". The XHTML 
was generated for the client validation handling any complex nesting.
The validation expressions were converted into valid javascript.

The challenge is to "say it in Seaside" without having to understand javascript.

I think the halo implementation is wrong

If we do

    html heading
        title: '"&'''; "first is a double quote, last is a single quote"
        with: '"&''' "first is a double quote, last is a single quote"

then we output

  <h1 title=""&'">"&'</h1>

However the halo will display

  <h1 title=""&'">"&'</h1>

Which has better readability but is inconsistent.

another example is

    html heading
        title: '&foo;';
        with: '&foo;'

will render as 

  <h1 title="&foo;">&foo;</h1>

but show up as

  <h1 title="&foo;">&foo;</h1>

The entities are replaced because that's what browsers do - replace entities.
Replacing the & character with & inside attribute values should do the trick.

Assuming we go with the halos all the way to a full source-code viewer:
 1. the halos would loose its ability to run event handlers code simply by clicking it (which is currently supported). I like this ability.
 - will we need to do that for all character entities? http://en.wikipedia.org/wiki/List_of_XML_and_HTML_character_entity_references

intrader.intrader, no, we don't parse the contents of attributes to make sure 
they are "valid" by some definition that only you know.

We encode entities to make sure that what the user supplies is what is provided 
to the browser (for example replacing " with "). And as we have now discovered, 
that is already working fine; it was a display bug in the halo source viewer 
that led you to believe it was not.

If you are providing a JavaScript code string as an attribute, it is *your* 
responsibility to make sure that your quotes within the string are properly 
nested. Seaside can't even know whether the attribute value is supposed to be 
JavaScript or something else. We will ensure that the value is output in a way 
such that the browser sees exactly what you provided.

Please use Issue 641 for further discussion of the halo source view... we're 
clearly not doing a second level of entity encoding in the source view, which 
is pretty surprising...

Once again, as I don't see my message 

Sorry, but this is a bug not in the 'Halo' handling but in the translation of 
single quote strings within the expression: method of an ajax request for on 
onClick message that results in embedded double quotes being generated.

Embedded double quotes are escaped using " Look at the actual source code. What 
version of Seaside/Smalltalk are you using?
The Halo was discussed because there was an issue there too.

Thanks for help, issue should be closed. It works.