264644 - [FIXr]event handler creation with createAttribute depends on case of attribute name but should not

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Martin Honnen]

In a HTML document (text/html) the case of attribute names doesn't matter,
whether you have <input onclick="JavaScript code"> or <input onClick="JavaScript
code"> or <input ONCLICK="JavaScript code">, in each case there is then an event
handler created that is fired when the click event occurs.
However when I try to use the DOM Level 2 Core method createAttribute to create
such event handler attributes dynamically with script I have found that Mozilla
(tested with Mozilla 1.8a5
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041015 and
with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804)
only creates an event handler when the letters are all lower case, that is I use
createAttribute('onclick'). With other casing there is an attribute node created
but when the element is clicked there is no event handler fired so it seems no
event handler is created.
I will attach a test case that demonstrates the problem, it contains three
statically created HTML checkbox controls with onclick event handlers that all
fire when the checkbox is clicked. Additionally three checkbox controls are
created dynamically with script where createAttribute and setAttributeNode is
used to attempt to create onclick event handlers dynamically but when you click
the dynamically created checkboxes you will find that only for the first where
the attribute is created with createAttribute('onclick') the event handler is
fired while for the other two the event handler is not fired.
In my view the creation of an event handler using
createAttribute/setAttributeNode in a text/html (HTML) document should not
depend on the case of the attribute name therefore I am filing this bug.
[Note that I am aware of other DOM ways to add event handlers (e.g.
addEventListener) but I think the DOM Core methods should work reliably too.]

Comment 1

[Martin Honnen]

Attachment: test case, see description in bug report

The test case works flawlessly with Opera 7.60 preview (meaning with that
browser both the event handlers of the statically created as well as the event
handlers of the dynamically created checkboxes fire when the checkbox is
clicked).

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem here is that createAttribute atomizes the atom name and the event
handler check does an atom compare.  Which fails if the case is wrong, of course.

Should createAttribute on a tag-soup document downcase the incoming attr name
like the HTML content sink does?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162355 [details] [diff] [review]
Like so

I went with doing this instead of doing a IsCaseSensitive() check in
nsDocument::CreateAttribute, but either way is OK, I guess.

In either case, I decided that making createAttributeNS case-insensitive was
uncalled for even for tag-soup.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 162355 [details] [diff] [review]
Like so

The other option is letting SetAttributeNode go through SetAttribute rather
then directly to SetAttr since that will fix the casing.

The only problem I can think of is if you create an attribute in an xml
document and move it to an html document. Of course, you shouldn't do that
without calling importNode.

Though maybe importNode on an attribute-node needs fixing too? Looking at
importNode though it doesn't fix casing on elements even, which maybe it
shouldn't since you should get the same type of element just with a new
ownerDocument, which maybe should apply to attributes too.

What do you guys think, how should importNode behave on an attribute?

Comment 6

[Boris Zbarsky [:bzbarsky]]

importNode is just severely broken all around (it doesn't even change the
ownerDocument; it just clones).

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed, but what do we want it to do?

The more I think about it the more I think that a better solution is to let
SetAttributeNode call SetAttribute. That way they'll always act the same even
for attributes created in different documents. That should follow the path of
least surprice to everyone.

Though I would not be opposed to checking in this patch as well...


Martin: While using document.createAttribute certainly should work I would
strongly recommend just using element.setAttribute instead. There are a few bugs
in the mozilla implementation (partly because noone uses these functions so they
get very little testing) and there's a few ambiguities in the spec. On top of
this they are inherintly slow in implementation and clumsy in syntax.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So just convert the atom back to string and call SetAttribute?  Or
SetAttributeNS if we have a namespace?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, sounds good to me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch per Jonas' comments

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 162550 [details] [diff] [review]
Patch per Jonas' comments

Fine by me, note that it'll be slightly slower since we'll end up re-checking
the name of the attribute.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.