299793 - group focus doesn't work if not bound

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Trying to set focus to a group works properly if the group is bound.  Focus will
properly go to the first control that the group contains.  However, if the group
is NOT bound, focus will not be properly set.  This is because in
nsXFormsGroupElement::TryFocus, we test for GetRelevantState() returning
PR_TRUE.  This won't be the case if not bound.  GetRelevantState() looks
specifically for the existance of the "enabled" attribute.  

Two approaches:
1) Maybe a better test is for TryFocus to look for the existance of the
"disabled" attribute and if not there call that good enough.  
2) Make "enabled" a default attribute for unbound controls.

#1 will cause less headaches, but maybe we should make it so that a control that
isn't disabled really is enabled or otherwise an author's styling may not work
as the author anticipates.  Allan?

Comment 1

[aaronr]

Attachment: testcase

clicking on the trigger should set focus to the input field.

Comment 2

[Allan Beaufour]

(In reply to comment #0)
> Trying to set focus to a group works properly if the group is bound.  Focus will
> properly go to the first control that the group contains.  However, if the group
> is NOT bound, focus will not be properly set.

Hmmm ... what exactly is the states for a control that is not bound? Is that
defined somewhere?

Comment 3

[aaronr]

I couldn't find this spelled out in the spec, except for this which I found in
the appendix about pseudoclasses: ":enabled &  :disabled  	[CSS3]  	Selects any
form control bound to a node with the model item property relevant evaluating to
true or false (respectively)."

It looks like to me that if we add a styles for the different processors for
enable, i.e.

xf|*[enabled]{
  background-color: red;
}
xf|*:enabled {
  background-color: red;
}
.enabled {
  background-color: red;
}

that the group has a red background on load for every processor but ours
(Novell, fP and XSmiles).  So I would think that the default state would be
enabled.  Also kinda leads me to believe that there are only two states, on and
off.  Nothing for 'uninitialized'.

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> I couldn't find this spelled out in the spec, except for this which I found in
> the appendix about pseudoclasses: ":enabled &  :disabled  	[CSS3]  	Selects any
> form control bound to a node with the model item property relevant evaluating to
> true or false (respectively)."
> 
> It looks like to me that if we add a styles for the different processors for
> enable, i.e.
> 
> xf|*[enabled]{
>   background-color: red;
> }
> xf|*:enabled {
>   background-color: red;
> }
> .enabled {
>   background-color: red;
> }
> 
> that the group has a red background on load for every processor but ours
> (Novell, fP and XSmiles).  So I would think that the default state would be
> enabled.  Also kinda leads me to believe that there are only two states, on and
> off.  Nothing for 'uninitialized'.

I agree on the above. Let all controls be "enabled" by default.

Comment 5

[Allan Beaufour]

Attachment: Patch

Sets controls enabled by default in nsXFormsControlsStub::Create() and
::ResetProperties().

Comment 6

[aaronr]

Comment on attachment 189172 [details] [diff] [review]
Patch

kStateAttribute[6]?  Really?  I know that we shouldn't use NS_LITERAL_STRING
where we don't have to, but couldn't we have a #define ATTRIBUTE_ENABLED 6 or
something and reference it that way?  It would be easier to read.  If nothing
else, please put a comment in nsXFormsUtils.cpp to ask people not to reorder or
add attributes to the middle of the kStateAttributes array since we are
dependent on the order.  Thanks.

r=me.

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> (From update of attachment 189172 [details] [diff] [review] [edit])
> kStateAttribute[6]?  Really?  I know that we shouldn't use NS_LITERAL_STRING
> where we don't have to, but couldn't we have a #define ATTRIBUTE_ENABLED 6 or
> something and reference it that way?  It would be easier to read.  If nothing
> else, please put a comment in nsXFormsUtils.cpp to ask people not to reorder or
> add attributes to the middle of the kStateAttributes array since we are
> dependent on the order.  Thanks.

kStateAttributes is a hack, and waits for bug 271720. I used it here so that
when we kill kStateAttributes we remember to fix this patch too. I'll include a
comment above the calls like "XXX hack, until bug 271720 is landed", ok?

Comment 8

[aaronr]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 189172 [details] [diff] [review] [edit] [edit])
> > kStateAttribute[6]?  Really?  I know that we shouldn't use NS_LITERAL_STRING
> > where we don't have to, but couldn't we have a #define ATTRIBUTE_ENABLED 6 or
> > something and reference it that way?  It would be easier to read.  If nothing
> > else, please put a comment in nsXFormsUtils.cpp to ask people not to reorder or
> > add attributes to the middle of the kStateAttributes array since we are
> > dependent on the order.  Thanks.
> 
> kStateAttributes is a hack, and waits for bug 271720. I used it here so that
> when we kill kStateAttributes we remember to fix this patch too. I'll include a
> comment above the calls like "XXX hack, until bug 271720 is landed", ok?


Sure, if this is just temporary, then forget my comment.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (In reply to comment #7)
> > kStateAttributes is a hack, and waits for bug 271720. I used it here so that
> > when we kill kStateAttributes we remember to fix this patch too. I'll include a
> > comment above the calls like "XXX hack, until bug 271720 is landed", ok?
> 
> 
> Sure, if this is just temporary, then forget my comment.

No, it was fine. I should have included a comment about it in the first place
instead of just thinking it.

Comment 10

[Allan Beaufour]

Checked in.