142898 - labels are not outlined when focus is received

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect, P4)

Description

[Brant Gurganus]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0.0+)
Gecko/20020507
BuildID:    2002050708

According to the LABEL test of the NGLayout Form Controls test cases, there
should be a dotted border around the label when given focus.  This is not the
case, however.

Reproducible: Always
Steps to Reproduce:
1. Go to http://www.mozilla.org/newlayout/testcases/htmlforms/label.html.
2. Give focus to the label.

Actual Results:  There is no dotted border around the label.

Expected Results:  There should be a dotted border around the label.

The page is rendered in quirks mode.  The pertinent source code is:
<input type="checkbox" id="chk">
<label for="chk">Match Case</label>

Comment 1

[Brant Gurganus]

I don't know completely how styling, etc. is done in Mozilla, but isn't this
basically just an onclick event that causes a CSS rule to become active for HTML
documents?

Comment 2

[Brant Gurganus]

This is a section 508 issues under paragraph c.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also bug 158043 comment 3.  This may require :active to be "fixed" first. 
Adding dependency to bug 65917.

Comment 4

[rods (gone)]

-->

Comment 5

[Brant Gurganus]

This seems to be partially fixed now.  The label sometimes becomes highlighted
although it is not outlined.

Comment 6

[Brant Gurganus]

The highlight is from clicking twice quickly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 224224 has been marked as a duplicate of this bug. ***

Comment 8

[Aaron Leventhal]

*** Bug 249813 has been marked as a duplicate of this bug. ***

Comment 9

[Nian Liu(n/a in a long time)]

i haven't see any spec on w3c about pseudo class active is either hierarchical
nor picky

Comment 10

[Aaron Leventhal]

Neo, I don't really understand your comment. Could you rephrase?

Comment 11

[Nian Liu(n/a in a long time)]

now this bug is set to depend on 65917 which describe pseudo class active
behavior. however, since there is no spec about that, i wonder whether this bug
still need to be depent on 65917. David may give some input to explain that.

Comment 12

[Nian Liu(n/a in a long time)]

"The :active pseudo-class applies while an element is being activated by the
user. For example, between the times the user presses the mouse button and
releases it."
-- from w3c css spec

i wonder :active has no relation here with focus line. test result in forms.css
to add 
    label:active{
    -moz-outline: 1px dotted invert;
    }
shows that it's consistent with what "for example part" describes.

Comment 13

[Aaron Leventhal]

Neo, I agree, :active does not relate to this bug. This bug is about :focus

I think you should move forward with a fix.

Comment 14

[Nian Liu(n/a in a long time)]

Aaron, do you know a way to get the label element from input element which is
connected with label element with label's attribute "for"?

Comment 15

[Aaron Leventhal]

What I would do is add something to nsIFormControl -- perhaps
attribute nsIDOMNode labelFor

Then in the label code get the form control node that corresponds to it, QI to
nsIFormControl and set the labelFor to this label.

Then it will be easy to get later when you need it.

(In reply to comment #14)
> Aaron, do you know a way to get the label element from input element which is
> connected with label element with label's attribute "for"?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nothing prevents a control from having more than one label.

Comment 17

[Aaron Leventhal]

David, is it worth the code bloat to keep track of all the labels? I've only
ever seen 1 label with a form control in real web pages, and I can't think of
much useful reason to have more than one. As far as I'm concerned, outlining the
first label is enough. In that case we can call the nsIFormControl attribute
firstLabel or something.

Or, Neo can keep track of a growable array of labels, but I doubt that's worth
it. More effort to code, more code bloat, very little payoff.

Comment 18

[Nian Liu(n/a in a long time)]

Aaron, i'm afraid it's hard to assign labelfor in label code since label part
and checkbox/radio part may appear in different sequence. i guess a higher level
when parsing html tree?

Comment 19

[Nian Liu(n/a in a long time)]

Aaron, i decide to connect checkbox/radio with label when first reflow. however, i 
haven't got which kind of frame owns the label element.

i had thought that it should be nsFormControlFrame. however, when i set breakpoint 
in nsFormControlFrame::Reflow and query interface of nsIDOMHTMLLabelElement from
GetContent(), i can't get it. anything i missed?

Comment 20

[Aaron Leventhal]

Neo, you could put in a temporary debugging line that says
presShell->GetPrimaryFrameFor(labelNode, &frame) to see what kind of frame it
has, and then look in the debugger. It's probably an inline frame.

Comment 21

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 22

[Nian Liu(n/a in a long time)]

Attachment: patchv2

connect should be done when frame init not need to be done in reflow

Comment 23

[Aaron Leventhal]

Comment on attachment 154744 [details] [diff] [review]
patchv2

You can't modify nsIDOMHTML interfaces. Instead, you can definitely modify
nsIFormControl or perhaps an nsIDOMNSHTMLInputElement and
nsIDOMNSHTMLLabelElement interfaces (Netscape DOM extension interfaces). 

Jst, will you let us modify those interfaces? Being able to get from a label to
its form control and vice-versa is useful for accessibility apps.

Comment 24

[Johnny Stenback (:jst)]

From looking at this patch, it seems like the new code in
nsHTMLLabelElement::GetForElement() could simply be moved to the one place where
the code is called. But having a GetForElement() is helpful for accessibility as
well, I'd have no problems with adding such a method (deDOMtaminated, returning
an already_AddRefed<nsIContent> or somesuch) to nsIFormControl.

Comment 25

[Nian Liu(n/a in a long time)]

unfornetelly, nsHTMLLabelElement is directly inherited from
nsGenericHTMLFormElement and nsIDOMHTMLLabelElement. it seems for me that
nsIFormControl( which is nsGenericHTMLFormElement directly inherited from) is
the only interface we can add GetForElement(). am i right? if it is, i'll add it
there. however, i don't feel comfortable here since we spread special element
attribute to other element. or can i add a nsIDOMNSHTMLLabelElement.idl to make
it much clearer? i prefer that.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's not an API we want to expose to script, so it shouldn't be nsIDOM*

Comment 27

[Nian Liu(n/a in a long time)]

so David, your suggestion is that move all idl change into nsIFormControl?

Aaron and Jst, if both of you agree, i'll do that.

Comment 28

[Aaron Leventhal]

(In reply to comment #27)
> so David, your suggestion is that move all idl change into nsIFormControl?
> 
> Aaron and Jst, if both of you agree, i'll do that.

Neo, I agree. Jst already agreed as well.

Comment 29

[Nian Liu(n/a in a long time)]

Attachment: patch v3(move nsIDOM* idl change to nsIFormControl)

Comment 30

[Aaron Leventhal]

Are we overloading the meaning of :active?

From http://www.w3.org/TR/REC-CSS2/selector.html#dynamic-pseudo-classes
"The :active pseudo-class applies while an element is being activated by the
user. For example, between the times the user presses the mouse button and
releases it."

Comment 31

[Nian Liu(n/a in a long time)]

it's just a trick to modify state of label when "for" element get/lose focus. if
we modify label element state to "focus", we can't keep "for" element state and
label element state as "focus" at same time. so it's really a mess.

and also, to give label a press down ui hint is not a big prob. some users may
even like it. at least, i think so.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 155058 [details] [diff] [review]
patch v3(move nsIDOM* idl change to nsIFormControl)

:active is definitely the wrong state to use here.  It's for the state when the
mouse button is depressed over an element.  It seems like what you want is
:focus -- i.e., when a form control is focused, you want a label pointing to
that control to match :focus as well.  It's worth double-checking that that's
conformant to CSS2.1 / css3-selectors, but I'd think (without looking at the
specs) that it would be.  Doing something like this probably requires adding a
concept of an additional :focus-ed element to the event state manager.

The getters and setters on nsIFormControl should probably use nsIContent rather
than nsIDOM* interfaces.

There's no reason to put code that's purely content-related in nsInlineFrame. 
It should be in nsHTMLLabelElement.

nsHTMLInputElement::GetLabelElement doesn't need to QueryInterface since it
already has that interface.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And your added function in nsHTMLLabelElement is wrong and duplicates (part of)
the correct algorithm that's already in GetForContent.  You should just expose
that, possibly with a wrapper.

Comment 34

[Nian Liu(n/a in a long time)]

pls. see my comments below

:active is definitely the wrong state to use here.  It's for the state when the
[nian]
yes, i know what active means. as i described below, it's just a trick to update
label element style. use focus and current api will bring too much trouble and
even can't do that. however, i'll try to add some functions in
nsEventStateManger.cpp to update label element state and keep consitent with
current focus implementation at the same time.
[/nian]
The getters and setters on nsIFormControl should probably use nsIContent rather
than nsIDOM* interfaces.
[nian]
is there any rules that i can refer to define api in mozilla?
[/nian]
There's no reason to put code that's purely content-related in nsInlineFrame. 
It should be in nsHTMLLabelElement.
[nian]
current implementation makes it possible that label element can appears before
"for" element in html and vice versa, so we may not set label element for "for"
element with SetAttr once we get label element has a "for" element connected
with it when navigate the html source(if "for" element appears after label
element, it's a null value when we parse "for" attribute). that's the reason why
i do that in frame init. at that time, the node tree is ready and couldn't be
null for "for" element.
[/nian]
nsHTMLInputElement::GetLabelElement doesn't need to QueryInterface since it
already has that interface.
[nian]
yes, you're right
[/nian]

And your added function in nsHTMLLabelElement is wrong and duplicates (part of)
the correct algorithm that's already in GetForContent.  You should just expose
that, possibly with a wrapper.
[nian]
since i added a new api, i'd more like GetForContent reuse the logic in the api.
i'll wrapper it.
[/nian]

Comment 35

[Aaron Leventhal]

Nian, what's the status of this? If you want David to re-review you should
probably reset the review flag to say |dbaron: review?|

Comment 36

[Nian Liu(n/a in a long time)]

it's better that David can have a look first on comment#34 and both of us get an
agreement before i make the next patch.

David, would you like to comment comment#34?

Comment 37

[Aaron Leventhal]

Okay say we make :focus apply to the label of a focused control, what about
:active or :hover ?

Comment 38

[Aaron Leventhal]

I'm about halfway through a patch for this, but it's too large of a change for
Firefox 1.5

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #32)
> specs) that it would be.  Doing something like this probably requires adding a
> concept of an additional :focus-ed element to the event state manager.

Note that this isn't quite right, since a control can have multiple labels.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And I think :active and :hover probably shouldn't have this behavior.)

Comment 41

[Aaron Leventhal]

(In reply to comment #39)
> (In reply to comment #32)
> > specs) that it would be.  Doing something like this probably requires adding a
> > concept of an additional :focus-ed element to the event state manager.
> 
> Note that this isn't quite right, since a control can have multiple labels.

I'msure that technically a control can have multiple labels, but don't you think
it's okay if just use the first one?

Comment 42

[Aaron Leventhal]

Removing sec508 keyword because you can see focus on the control itself.

Comment 43

[Aaron Leventhal]

*** Bug 297111 has been marked as a duplicate of this bug. ***

Comment 44

[Aaron Leventhal]

*** Bug 78839 has been marked as a duplicate of this bug. ***

Comment 46

[u294409]

Change OS to all.

Comment 47

[u294409]

Hmmm... When label should get focus, checkbutton/radiobutton takes it and has two focus indicators (bug #412069).

Comment 48

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S4. However, the bug has 4 duplicates.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.