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

RESOLVED FIXED in mozilla1.8alpha5

Status

()

P3
normal
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: martin.honnen, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.8alpha5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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.]
(Reporter)

Comment 1

14 years ago
Created attachment 162299 [details]
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).
(Assignee)

Comment 2

14 years ago
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?
(Assignee)

Comment 4

14 years ago
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.
Attachment #162355 - Flags: superreview?(peterv)
Attachment #162355 - Flags: review?(bugmail)
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?
(Assignee)

Comment 6

14 years ago
importNode is just severely broken all around (it doesn't even change the
ownerDocument; it just clones).
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.
(Assignee)

Comment 8

14 years ago
So just convert the atom back to string and call SetAttribute?  Or
SetAttributeNS if we have a namespace?
(Assignee)

Updated

14 years ago
Attachment #162355 - Flags: superreview?(peterv)
Attachment #162355 - Flags: superreview-
Attachment #162355 - Flags: review?(bugmail)
Attachment #162355 - Flags: review-
(Assignee)

Comment 10

14 years ago
Created attachment 162550 [details] [diff] [review]
Patch per Jonas' comments
Attachment #162355 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #162550 - Flags: superreview?(peterv)
Attachment #162550 - Flags: review?(bugmail)
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.
Attachment #162550 - Flags: superreview?(peterv) → superreview+
(Assignee)

Updated

14 years ago
Assignee: general → bzbarsky
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Summary: event handler creation with createAttribute depends on case of attribute name but should not → [FIXr]event handler creation with createAttribute depends on case of attribute name but should not
Target Milestone: --- → mozilla1.8alpha5
(Assignee)

Comment 12

14 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.