Closed Bug 331911 Opened 15 years ago Closed 15 years ago

SetIntrinsicState initialized too early

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: aaronr)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(2 files, 2 obsolete files)

SetIntrinsicState is currently being initialized during nsXFormsControlStub::OnCreated.  This is too early.  SetIntrinsicState won't do anything until the element is in a document.  It works most of the time because we'll set the intrinsic state during refresh of controls under the model's list.  But this won't work for elements (like group) that are not be bound to anything.
Attached file testcase
In this testcase, focus isn't passed from a group to its first child because the GetRelevantState() call in nsXFormsGroupElement::TryFocus is returning that the group isn't relevant.  This is because we only call SetIntrinsicState on a xf:group one time, during ::OnCreated.
Attached patch proposed fix (obsolete) — Splinter Review
I fixed this by moving the initialization of the intrinsic states from ::OnCreated to ::DocumentChanged.
Attachment #216472 - Flags: review?(allan)
Attachment #216472 - Flags: review?(smaug)
fails 9.1.1.c in the testsuite
Blocks: 322255
Comment on attachment 216472 [details] [diff] [review]
proposed fix

removing review requests.  Doron mentioned that I should change TryFocusChildControl while I'm in a focus bug to test for child nodes being element nodes before QI'ing to be more efficient.
Attachment #216472 - Flags: review?(smaug)
Attachment #216472 - Flags: review?(allan)
Status: NEW → ASSIGNED
(In reply to comment #4)
> (From update of attachment 216472 [details] [diff] [review] [edit])
> removing review requests.  Doron mentioned that I should change
> TryFocusChildControl while I'm in a focus bug to test for child nodes being
> element nodes before QI'ing to be more efficient.
> 

I believe QIs should be avoided if possible :)
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 216472 [details] [diff] [review] [edit] [edit])
> > removing review requests.  Doron mentioned that I should change
> > TryFocusChildControl while I'm in a focus bug to test for child nodes being
> > element nodes before QI'ing to be more efficient.
> > 
> 
> I believe QIs should be avoided if possible :)
> 

Found a couple of other places that do the same thing (run a child list QI'ing w/o checking type first).  I'll fix them all in a seperate bug.  Re-asking for review for this patch.

Attachment #216472 - Flags: review?(allan)
Attachment #216472 - Flags: review?(smaug)
Comment on attachment 216472 [details] [diff] [review]
proposed fix

> nsXFormsControlStubBase::DocumentChanged(nsIDOMDocument *aNewDocument)
> {
>   // We need to re-evaluate our instance data binding when our document
>   // changes, since our context can change
>   if (aNewDocument) {
>+    // The intrinsic state needs to be initialized here since
>+    // SetIntrinsicState will do nothing until the element lives in a document.
>+    ResetProperties();
>+  
>     Bind();
>     Refresh();
>   }
> 
>   return NS_OK;
> }

I agree, something's not right, but doesn't the handling then belong inside Bind()?

(Hmmm, actually it belongs inside Refresh(), as the changes to MIP states should not be changes before refresh, but that's a bigger issues)
Attachment #216472 - Flags: review?(allan) → review-
(In reply to comment #7)
> (From update of attachment 216472 [details] [diff] [review] [edit])
> > nsXFormsControlStubBase::DocumentChanged(nsIDOMDocument *aNewDocument)
> > {
> >   // We need to re-evaluate our instance data binding when our document
> >   // changes, since our context can change
> >   if (aNewDocument) {
> >+    // The intrinsic state needs to be initialized here since
> >+    // SetIntrinsicState will do nothing until the element lives in a document.
> >+    ResetProperties();
> >+  
> >     Bind();
> >     Refresh();
> >   }
> > 
> >   return NS_OK;
> > }
> 
> I agree, something's not right, but doesn't the handling then belong inside
> Bind()?
> 
> (Hmmm, actually it belongs inside Refresh(), as the changes to MIP states
> should not be changes before refresh, but that's a bigger issues)
> 


I was just looking for a place that we can initialize a control to 'enabled' just once.  Bind and Refresh both get called twice even if the control doesn't have a SNB attribute.
Comment on attachment 216472 [details] [diff] [review]
proposed fix

(In reply to comment #8)
> (In reply to comment #7)
> > I agree, something's not right, but doesn't the handling then belong inside
> > Bind()?
> > 
> > (Hmmm, actually it belongs inside Refresh(), as the changes to MIP states
> > should not be changes before refresh, but that's a bigger issues)
> 
> I was just looking for a place that we can initialize a control to 'enabled'
> just once.  Bind and Refresh both get called twice even if the control doesn't
> have a SNB attribute.

I still do not think it is the correct place, but the whole SetIntrinsicState() business is somewhat defect in the first place, so ok, if this fixes your use case. r=me
Attachment #216472 - Flags: review- → review+
Hardware: PC → All
Attachment #216472 - Flags: review?(smaug) → review+
Attached patch patch checked in (obsolete) — Splinter Review
This is the patch that I checked in.  Updated to trunk.
Attachment #216472 - Attachment is obsolete: true
doh!
Attachment #217200 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Blocks: 313118
Blocks: 332853
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.