Page Info's label-control association is broken (should be by id).

RESOLVED FIXED

Status

RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: nick, Assigned: nick)

Tracking

({html4, testcase})

Trunk
html4, testcase

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
According to the HTML spec, the "for" attribute in a label specifies the *id* of
a form control, not the name
(http://www.w3.org/TR/html401/interact/forms.html#h-17.9.1). Check how the
example in the spec uses both name and id attributes. But the code in
pageInfo.js associate it by the name attribute.
(Assignee)

Comment 1

17 years ago
Created attachment 92077 [details]
testcase
(Assignee)

Updated

17 years ago
Keywords: testcase
(Assignee)

Comment 2

17 years ago
Created attachment 92081 [details] [diff] [review]
patch
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Comment on attachment 92081 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #92081 - Flags: superreview+
(Assignee)

Comment 4

17 years ago
Created attachment 92082 [details] [diff] [review]
silly and harmless thing fixed
Attachment #92081 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #92082 - Flags: superreview+
(Assignee)

Comment 5

17 years ago
Created attachment 92083 [details] [diff] [review]
Ops, attached the old file instea of the new one =/
(Assignee)

Updated

17 years ago
Attachment #92082 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #92083 - Flags: superreview+
Comment on attachment 92083 [details] [diff] [review]
Ops, attached the old file instea of the new one =/

You are correct that the label really should match the id, and not the name.
However, because mozilla also associates the for attribute with the names, this
should also match with the names. The absolute easiest way to make this change
is to add "|| formfields[j].id == whatfor" to the test inside the inner loop.

I'd rather you didn't use .form anywhere, it has bugs, though I don't remeber
the bug numbers right off. A couple of the comments in the code mention it
though. I think .getAttribute("form") works correctly though, if you really
need to do that.
(Assignee)

Comment 7

17 years ago
>You are correct that the label really should match the id, and not the name.
>However, because mozilla also associates the for attribute with the names.

Where? If it does so, it's broken there.

>I'd rather you didn't use .form anywhere, it has bugs, though I don't remeber
>the bug numbers right off. A couple of the comments in the code mention it
>though. I think .getAttribute("form") works correctly though, if you really
>need to do that.

Uhm... control.form is very widely used in pages, it gets the form object from a
control. Being so idely used... why would be the bug that prevents its use here?
Anyway it's an optimization, and it coud be removed...
(Assignee)

Updated

17 years ago
Keywords: html4
(Assignee)

Comment 8

17 years ago
AFAICS if I open the testcase and click on "Good label", the appropiate control
get focused. If I click on "Bad label" it doesn't. It seems that Mozilla is
getting right the label-form association and doesn't use name at all.
which test case?
(Assignee)

Comment 10

17 years ago
er... the one attached to this bug, aptly named "testcase" =)
oh, that one. hrm, what does it do when you delete the doctype (putting it into 
quirks mode?) (I don't have Mozilla on this computer here at work)
(Assignee)

Comment 12

17 years ago
I've tried with the page in quirks mode, same thing. Mozilla links the label to
the form only by the id attribute, ignoring the link by "name".
(Assignee)

Updated

17 years ago
Keywords: review

Comment 13

17 years ago
Comment on attachment 92083 [details] [diff] [review]
Ops, attached the old file instea of the new one =/

r=ksosez
Attachment #92083 - Flags: review+
Comment on attachment 92083 [details] [diff] [review]
Ops, attached the old file instea of the new one =/

Prevailing style clearly puts a space after if, for, and other keywords. 
Please follow it.

/be
(Assignee)

Comment 15

17 years ago
Created attachment 92120 [details] [diff] [review]
Some spaces added
Attachment #92083 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #92120 - Flags: superreview+
Attachment #92120 - Flags: review+
(Assignee)

Updated

17 years ago
Keywords: review → approval, mozilla1.0.1
Comment on attachment 92120 [details] [diff] [review]
Some spaces added

Approval granted for trunk checkin.  Please request branch checkin once this is
verified on trunk with no regressions.
Attachment #92120 - Flags: approval+
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Approval for branch checkin; please change mozilla1.0.1+ to fixed1.0.1 when
checked in.
Keywords: mozilla1.0.1 → mozilla1.0.1+
fixed on branch
Keywords: approval, mozilla1.0.1+ → fixed1.0.1

Updated

16 years ago
Keywords: verified1.0.1

Comment 20

16 years ago
Verified on windows 2k, (netscape branch build: 2002-08-21-08-1.0)
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.