SetIntrinsicState initialized too early

RESOLVED FIXED

Status

RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: aaronr, Assigned: aaronr)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
fixed1.8.0.4, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

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

Comment 2

13 years ago
Created attachment 216472 [details] [diff] [review]
proposed fix

I fixed this by moving the initialization of the intrinsic states from ::OnCreated to ::DocumentChanged.
Attachment #216472 - Flags: review?(allan)
(Assignee)

Updated

13 years ago
Attachment #216472 - Flags: review?(smaug)
(Assignee)

Comment 3

13 years ago
fails 9.1.1.c in the testsuite
(Assignee)

Updated

13 years ago
Blocks: 322255
(Assignee)

Comment 4

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

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 5

13 years ago
(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 :)
(Assignee)

Comment 6

13 years ago
(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.

(Assignee)

Updated

13 years ago
Attachment #216472 - Flags: review?(allan)
(Assignee)

Updated

13 years ago
Attachment #216472 - Flags: review?(smaug)

Comment 7

13 years ago
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)

Updated

13 years ago
Attachment #216472 - Flags: review?(allan) → review-
(Assignee)

Comment 8

13 years ago
(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 9

13 years ago
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+

Updated

13 years ago
Hardware: PC → All

Updated

13 years ago
Attachment #216472 - Flags: review?(smaug) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 217200 [details] [diff] [review]
patch checked in

This is the patch that I checked in.  Updated to trunk.
Attachment #216472 - Attachment is obsolete: true
(Assignee)

Comment 11

13 years ago
Created attachment 217201 [details] [diff] [review]
patch w/ fixed line endings

doh!
Attachment #217200 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

13 years ago
Blocks: 313118

Updated

13 years ago
Blocks: 332853

Updated

13 years ago
Keywords: fixed1.8.0.3, fixed1.8.1

Updated

13 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.