44372 - & becomes & in attributes (when saving from editor)

Status: VERIFIED FIXED

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

Description

[TAKAHASHI Makoto]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; m16) Gecko/20000621
BuildID:    2000062108

I wrote html

<IMG SRC="http://s003.ultraranking.com/cgi-shl/nph-ultra.asp?515&amp;*&amp;none"
class=nobdr ISMAP alt="Ultra Ranking">

as in HTML 4.01 B2.2(http://www.w3.org/TR/REC-html40/appendix/notes.html#h-B.2)


Mozilla changed &amp; to &

Reproducible: Always
Steps to Reproduce:
1. Edit page of URI
2. Save it to local file
3. &amp; changed to & only.


&amp; should not be changed.

Comment 1

[Akkana Peck]

Looks like the output system needs to convert ampersands to & on output.

Naoki: shouldn't the entity converter be handling that?

Or should we get eHTMLTag_entity for the ampersand, in which case we'd handle it
directly in nsHTMLContentSinkStream::AddLeaf?  Cc'ing Harish, who might know.

Comment 2

[Akkana Peck]

Oops, forgot to cc some people who wanted to be cc'ed when I made that last comment.

Comment 3

[nhottanscp]

>Naoki: shouldn't the entity converter be handling that?
No, the entity converter's input can be HTML data which may already contain '&'. 
So it doen not generate &quot, &amp, &lt, &gt.

Comment 4

[Akkana Peck]

Okay, I thought the job of the entity converter was to convert unicode-encoded
entities into the & entity form.  I thought that was the whole reason we were
using it.

If it's not the entity encoder that does this, there must be a service somewhere
that does.  Clearly the output sink is the wrong place to maintain the list of
all possible entities in their unicode and ampersand representations; I'm sure
this code has already been written somewhere else.  Any idea what service does
handle this?

Comment 5

[nhottanscp]

It is possible to add a function to the entity converter which first converts 
&quot, &amp, &lt, &gt then do the conversion for everything else.

Comment 6

[Peter "jag" Annema]

Well, this is about CDATA encoding within %uri context...

<a href="foo.cgi?a=<hmmm>">hmmm</a>        (1)
<a href="foo.cgi?a=%3chmmm%3e">hmmm</a>    (2)
<a href="foo.cgi?a=&lt;hmmm&gt;">hmmm</a>  (3)

(1) is not valid URI syntax. (2) is. In html however one is allowed to encode 
it as (3) which will be written out by the editor as (1), but should be written 
out as (2) or (3). Same (more so) for &, %26 and &amp;. (2) appeals to me since 
that's what the browser will need anyway.

Comment 7

[Peter "jag" Annema]

Marking this All All (guessing it's XP ;-) )

Comment 8

[Akkana Peck]

Okay, trying for the third time to make this comment (the first time, mozilla
crashed, the second time, bugzilla lost my comment in a midair collision and
"submit anyway" failed but caused the form cache to lose the original info). 
Guess I'd better do it in small chunks this time.

Comment 9

[Akkana Peck]

Harish and I talked about this for a while.  This only happens in attributes. 
It turns out that the code that currently decides whether to generate inflated
entities (in text nodes, but not inside attributes) lives in nsXIFConverter, so
it makes sense for the attribute-expansion code to live there, too.  I'll do
this part.

Comment 10

[Akkana Peck]

However, there are problems with that.  What happens if we expand the entity
recognition code to include quotes (currently it's only &, <, >) and someone has
an attribute bordered by quotes but including a &quot;?  Not clear that there's
anything the nsXIFConverter can do in this case -- it doesn't have enough
information on the difference between the two quotes.

Harish said he'd look into trying to find a way for the parser to retain
information about which characters were originally entities, which would solve
this problem the right way.  

However, he's not sure that this is possible, so for now I'm taking the bug to
do the simpler fix.

Comment 11

[Akkana Peck]

Attachment: Better test case (doesn't hang if external server is down)

Comment 12

[rubydoo123]

setting to nsbeta3+

Comment 13

[rubydoo123]

setting priority in status whiteboard

Comment 14

[Akkana Peck]

Moving up to P2 so I'll have a fix early enough that Harish can decide whether
he has time to do the other part.

Comment 15

[Hixie (not reading bugmail)]

cCanging summary so that I don't panic each time I see this bug (because on the
browser side of things '&' *should* become '&' in attributes!).

Comment 16

[Akkana Peck]

Fixed.

Comment 17

[Akkana Peck]

Scott showed me a more efficient way to do this; attaching patch (which I'm
about to check in).

Comment 18

[Akkana Peck]

Attachment: Patch from Scott

Comment 19

[sujay]

verified in 8/30 build.