280368 - Support attribute-based repeats (first step)

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P5)

Description

[Allan Beaufour]

We need to support something like:
<html:table id="tab" xforms:repeat-nodeset="..." xforms:repeat-model="model2">
...
</html:table>

When implementing remember that <setindex/> and index() should still work, and
refers to the @id on the element with the attributes ("tab" in the above).

Comment 1

[aaronr]

Got a note from T.V. Raman.  We need this in to consider ourselves XForms Basic.
 Ugh!

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> Got a note from T.V. Raman.  We need this in to consider ourselves XForms Basic.
>  Ugh!

Ugh white man! :) Yes, let's not forget it totally, but it's not on high on my
priority list. Let's get some of the more basic functionality working first.

Comment 3

[alexander :surkov (:asurkov)]

If I think right then node with attributes like xforms:repeat-nodeset should have some xtf binding. But as I know xtf binding is added only by tagname. Have you any idea how it can be done?

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> If I think right then node with attributes like xforms:repeat-nodeset should
> have some xtf binding. But as I know xtf binding is added only by tagname. Have
> you any idea how it can be done?

AFAIK there is no way of doing this right now. We need "XTF for attributes". Things seems to be moving towards sXBL/XBL2 though so we might need to start advocating for that there.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #3)
> > If I think right then node with attributes like xforms:repeat-nodeset should
> > have some xtf binding. But as I know xtf binding is added only by tagname. Have
> > you any idea how it can be done?
> 
> AFAIK there is no way of doing this right now. We need "XTF for attributes".
> Things seems to be moving towards sXBL/XBL2 though so we might need to start
> advocating for that there.
> 

Is it too difficult to expand XTF for bindings supporting by attributes or does it contradict with XTF ideology? I guess it can takes a long time for sXBL/XBL2 awaiting. Also I think bug 306526 is in need of the such XTF possibility.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > If I think right then node with attributes like xforms:repeat-nodeset should
> > > have some xtf binding. But as I know xtf binding is added only by tagname. Have
> > > you any idea how it can be done?
> > 
> > AFAIK there is no way of doing this right now. We need "XTF for attributes".
> > Things seems to be moving towards sXBL/XBL2 though so we might need to start
> > advocating for that there.
> > 
> 
> Is it too difficult to expand XTF for bindings supporting by attributes or does
> it contradict with XTF ideology? I guess it can takes a long time for sXBL/XBL2
> awaiting. Also I think bug 306526 is in need of the such XTF possibility.

I'm not sure it matters what way we go. Gecko 1.8 is closed, so we are talking long term here no matter what.

Comment 7

[Doron Rosenberg (IBM)]

theoretically, in css, we could do html|foo[xforms:attr], but that would be costly.

Comment 8

[Allan Beaufour]

This could actually work:
*[xf|repeat-nodeset] {
  -moz-binding: url("xforms.xml#repeathandler");
}

<binding id="repeathandler">
  <content>
    <xf:repeat mozType:attrbased="true" inherits="nodeset=repeat-nodeset">
      <children/>
    </xf:repeat>
  </content>
</binding>

Then in the current (C++) repeat handling of refresh/expansion:
if (attrbased) {
  template = cloneNode(this.parent, false);
  template.removeAttribute("repeat-*");
  for (c in this.children) {
    template.appendChild(c);
  }
} else {
  template = (currentcode);
}

Hacky, and slightly expensive, but it could work, and reuse the existing code.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> 
> Hacky, and slightly expensive, but it could work, and reuse the existing code.
> 

Not, always.
1) if some element has binding then repeat binding overrides it. It is actual for xul.
2) probably some elements will be broken when they see xf:reapeat inside of them. F.x. I think html:select will.

Comment 10

[Allan Beaufour]

(In reply to comment #9)
> (In reply to comment #8)
> > 
> > Hacky, and slightly expensive, but it could work, and reuse the existing code.
> > 
> 
> Not, always.
> 1) if some element has binding then repeat binding overrides it. It is actual
> for xul.

No it shouldn't. Because it will be correctly bound to the cloned content inside the "unrolled" repeats.

> 2) probably some elements will be broken when they see xf:reapeat inside of
> them. F.x. I think html:select will

They would not have xf:repeat inside them. I'm not following you.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> > 1) if some element has binding then repeat binding overrides it. It is actual
> > for xul.
> 
> No it shouldn't. Because it will be correctly bound to the cloned content
> inside the "unrolled" repeats.

I didn't understand. If I understand right then one element can have one binding. If it is right then binding for repeat should override default one. Am I not right?

> > 2) probably some elements will be broken when they see xf:reapeat inside of
> > them. F.x. I think html:select will
> 
> They would not have xf:repeat inside them. I'm not following you.

Probalby html:select shouldn't. But we can cover some other elements. Though in general it seems to me it's not right way to support bounded elements set.

What about xtf binding for attributes? As I can see it is one right way.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #10)
> > > 1) if some element has binding then repeat binding overrides it. It is actual
> > > for xul.
> > 
> > No it shouldn't. Because it will be correctly bound to the cloned content
> > inside the "unrolled" repeats.
> 
> I didn't understand. If I understand right then one element can have one
> binding. If it is right then binding for repeat should override default one. Am
> I not right?

You are, but look at my pseudocode. I clone the node that the repeat is bound to, without the repeat-* attributes and then used in the template. That means that:
<tr repeat-nodeset="/foo/bar"></tr>
will be bound to my repeat-thing, overriding the tr element binding, correct. But, it will be unfolded into:
<repeat nodeset="/foo/bar">
  <tr>
  </tr>
</repeat>
And thus, the original binding will be in place for the tr element in the unrolled content.

> What about xtf binding for attributes? As I can see it is one right way.

XTF is a dead end. XBL2 is where the solution should come from.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> XTF is a dead end. XBL2 is where the solution should come from.
> 

Eh? XTF still has lots of features which XBL2 doesn't

Comment 14

[Doron Rosenberg (IBM)]

(In reply to comment #13)
> (In reply to comment #12)
> > XTF is a dead end. XBL2 is where the solution should come from.
> > 
> 
> Eh? XTF still has lots of features which XBL2 doesn't
> 

Last I heard, they planned on adding xtf-like functionality into xbl2 :)

Comment 15

[Allan Beaufour]

(In reply to comment #13)
> (In reply to comment #12)
> > XTF is a dead end. XBL2 is where the solution should come from.
> > 
> 
> Eh? XTF still has lots of features which XBL2 doesn't

Ok, I was maybe being a bit short/cocky there :)

I do not believe that anybody (besides us) are going to extend or maintain XTF. Afaik, the plan is for XBL2 to solve the things that XTF solve. Moreover, 1.8.x is most probably closed land, so any enhancements to XTF would have to target 1.9 -- that is distant future.

imho, we should make sure that we can use XBL2 instead of XTF at some point in the future. Unfortunately, I have not had time to investigate XBL2 :(

More specifically for this bug, the best solution I have is the hack I've just detailed.

Comment 16

[alexander :surkov (:asurkov)]

1) XBL bindings aren't applied to html:td/html:tr elements.
2) Is Allan's hack supposes a support of nested attribute based repeats? F.x.

<xf:model><xf:instance>
  <root xmlns="">
    <item>
      <subitem>1.1</subitem>
      <subitem>1.2</subitem>
    </item>
    <item>
      <subitem>2.1</subitem>
      <subitem>2.2</subitem>
    </item>
  </root>
</xf:instance></xf:model>

<grid>
 <columns>
   <column/>
   <column/>
 </columns>
 <rows xf:repeat-nodeset="/bla/bla">
   <row xf:repeat-nodeset="*">
     <xf:output ref="."/>
   </row>
 </rows>
</grid>

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> 1) XBL bindings aren't applied to html:td/html:tr elements.

Are you saying that XBL bindings does not work for td/tr elements?

> 2) Is Allan's hack supposes a support of nested attribute based repeats?

It is supposed to work, and in theory it should. That said, it _is_ a hack.

Comment 18

[Allan Beaufour]

Attachment: Testcase - div

Comment 19

[Allan Beaufour]

Attachment: Testcase - div, nested

Comment 20

[Allan Beaufour]

Attachment: Test - table (tr)

Comment 21

[Allan Beaufour]

Attachment: Work in progress

This is some of the way
* the approach is the one from comment 8
* I had to change element defering, because it was deferring on the XBL doc
* I had to do a lot of css changes, ie. make the xforms namespace explicit, or else the binding would not work. It might be bogus though...
* tr does not work. the binding is never triggered

Comment 22

[Allan Beaufour]

Attachment: Testcase - div (including insert and delete)

Comment 23

[Allan Beaufour]

Attachment: Patch

Here's a cleaned up version of attachment 224080 [details] [diff] [review]. The css changes were bogus, otherwise this is just as noted in comment 21.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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


>+nsXFormsModelElement::DeferElementBind(nsIXFormsControl *aControl)
> {
>-  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
>+  NS_ENSURE_ARG_POINTER(aControl);
>+  nsCOMPtr<nsIDOMElement> element;
>+  nsresult rv = aControl->GetElement(getter_AddRefs(element));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(element));
>+  NS_ASSERTION(content, "nsIDOMElement not implementing nsIContent?!");
> 
>-  if (!doc || !aControl) {
>-    return NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc();
>+  if (!doc) {
>+    // We do not carea about elements without a document. If they get added to
>+    // a document at some point in time, they'll try to bind again.
>+    return NS_OK;

typo "carea"


>+nsresult
>+nsXFormsRepeatElement::GetBindingParent(nsIDOMNode *aNode,
>+                                        nsIDOMNode **aParent)
>+{
>+  NS_ENSURE_ARG_POINTER(aNode);
>+  NS_ENSURE_ARG_POINTER(aParent);
>+
>+  nsCOMPtr<nsIDOMDocument> domDoc;
>+  aNode->GetOwnerDocument(getter_AddRefs(domDoc));
>+  NS_ENSURE_STATE(domDoc);
>+
>+  nsCOMPtr<nsIDOMDocumentXBL> docXBL(do_QueryInterface(domDoc));
>+  NS_ENSURE_STATE(docXBL);


No need the check NS_ENSURE_STATE(domDoc), that will be done 
after QI to docXML

Looks great :) This is a bit strange way to use XBL, but it works.
r=me

Comment 25

[Allan Beaufour]

(In reply to comment #24)
> This is a bit strange way to use XBL, but it works.

Is that not the purpose of extensions/xforms? Use XTF and XBL in strange and twisted ways ;-)

Comment 26

[Allan Beaufour]

Attachment: Patch with smaug's comments fixed

Comment 27

[Doron Rosenberg (IBM)]

Comment on attachment 224184 [details] [diff] [review]
Patch with smaug's comments fixed

Not the most efficient at places (those removeAttribute calls, can't they be done on nsIContent faster?), but this is repeat, it always will be slow :)

Comment 28

[Allan Beaufour]

Fixed on branch. It _does not_ work for tr elements.

Comment 29

[aaronr]

(In reply to comment #28)
> Fixed on branch. It _does not_ work for tr elements.
> 


Could you make sure that this is readme'd or mentioned on the status page?  Otherwise we'll lose track of it.

Comment 30

[aaronr]

(In reply to comment #28)
> Fixed on branch. It _does not_ work for tr elements.
> 

For those interested in why, read: https://bugzilla.mozilla.org/show_bug.cgi?id=83830

Comment 31

[Allan Beaufour]

Are we still using 'xf-to-branch' mechanism, so this still needs to be sync'ed to branches?

Comment 32

[aaronr]

(In reply to comment #31)
> Are we still using 'xf-to-branch' mechanism, so this still needs to be sync'ed
> to branches?
> 

yep, still needs to go to branches.

Comment 33

[aaronr]

checked into 1.8.0 branch on 2006/09/21

Comment 34

[aaronr]

checked into 1.8 branch on 2006/11/21