302513 - Figure out id attributes inside repeats

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Right now we just kill the id attribute on cloned content:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/xforms/nsXFormsRepeatElement.cpp&rev=1.21&root=/cvsroot&mark=923#920

I think the idea is that the id should be valid for the currently selected index
inside a repeat. And stuff inside "repeat rows" should use have a "local scope"
for id's.

"Inspiration"-links :)
http://lists.w3.org/Archives/Member/w3c-forms/2005JanMar/att-0181/2005Feb28.html#topic6
http://lists.w3.org/Archives/Member/w3c-forms/2005JanMar/0182

Comment 1

[Allan Beaufour]

Attachment: Testcase: Switch inside repeat

Here's a switch inside a repeat. People would except all the switches to work I
guess...

Comment 2

[Allan Beaufour]

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

Comment 3

[Allan Beaufour]

Patch coming up

Comment 4

[Allan Beaufour]

Attachment: Testcase: Switch inside repeat v2

Comment 5

[Allan Beaufour]

Attachment: Testcase: Focus inside repeat

Comment 6

[Allan Beaufour]

Attachment: Patch for toggle and setfocus

Here's a patch handling "local scope" for setfocus and toggle, passing the two
attached testcases.

I need to do a better explanation of what I've done, and handle dispatch too.
It should behave in the same way.

Comment 7

[Allan Beaufour]

Attachment: Testcase: Dispatch inside repeat

Comment 8

[Allan Beaufour]

Attachment: Testcase: Dispatch inside repeat v2

Also tests one more thing: Dispatch element behaviour in non-focused repeat
row: Load the test-case and press the add button in the group at the bottom of
the form.

Comment 9

[Allan Beaufour]

Attachment: Patch for toggle, setfocus, and dispatch

Here's a patch for the elements that should have special handling for use with
repeats.

The idea is that xforms actions using ids but residing outside a repeat should
work on the currently selected repeat row. Actions inside repeats should work
on the element matching the id inside their own repeat row (ie. scoped).

I've implemented a nsXFormsUtils::GetElementById() that checks if an element
with a given id is a child of a repeat. If it is, it checks whether the action
element is inside a repeat and then uses the action element's repeat row, if
not it uses the currently focused repeat row.

It's not exactly cheap :( We could possibly use the mRepeatState mechanisms
used for nsXFormsDelegateStub, but I'd rather add that later if we agree on
that.

Comment 10

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

Comment on attachment 193071 [details] [diff] [review]
Patch for toggle, setfocus, and dispatch


>+  }
>+  NS_NAMED_LITERAL_STRING(idStr, "id");
>+  PRUint32 i;
>+  nsAutoString idVal;
>+  nsCOMPtr<nsIDOMNode> childNode;
>+  for (i = 0; i < elements; ++i) {
>+    descendants->Item(i, getter_AddRefs(childNode));
>+    element = do_QueryInterface(childNode);
>+    if (element) {
>+      element->GetAttribute(idStr, idVal);
>+      if (idVal.Equals(aId)) {
>+        break;

This is wrong. Attribute called "id" may not actually be the ID attribute.
You'll have to QI to nsIContent and use GetIDAttributeName().

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (From update of attachment 193071 [details] [diff] [review] [edit])
> This is wrong. Attribute called "id" may not actually be the ID attribute.
> You'll have to QI to nsIContent and use GetIDAttributeName().

I guess you are right. How about all the other places we use "id", shouldn't
they be fixed too?

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 193071 [details] [diff] [review] [edit] [edit])
> > This is wrong. Attribute called "id" may not actually be the ID attribute.
> > You'll have to QI to nsIContent and use GetIDAttributeName().
> 
> I guess you are right. How about all the other places we use "id", shouldn't
> they be fixed too?

No. I checked them and they are all related to XForms elements, and in those
cases the "id" attribute is actually ID.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 193071 [details] [diff] [review] [edit] [edit] [edit])
> > > This is wrong. Attribute called "id" may not actually be the ID attribute.
> > > You'll have to QI to nsIContent and use GetIDAttributeName().
> > 
> > I guess you are right. How about all the other places we use "id", shouldn't
> > they be fixed too?
> 
> No. I checked them and they are all related to XForms elements, and in those
> cases the "id" attribute is actually ID.

Why? The id attribute is not defined anywhere in the XForms spec. The id
attribute comes from the host language.
"A host language must permit an attribute of type xsd:ID on each XForms element."
[http://www.w3.org/TR/xforms/slice3.html#structure-attrs-common]

Comment 14

[Allan Beaufour]

Attachment: Patch using GetIDAttributeName()

Ok, now using GetIDAttributeName. I also use GetNodeType to check for whether
it is an element, must be cheaper than QI.

Comment 15

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

(In reply to comment #13)
> Why? The id attribute is not defined anywhere in the XForms spec. The id
> attribute comes from the host language.
> "A host language must permit an attribute of type xsd:ID on each XForms element."
> [http://www.w3.org/TR/xforms/slice3.html#structure-attrs-common]
> 

Ah, you're right. I wonder how to decide what is actually the host language, if 
the document contains elements from several "languages"
But in our case, the ID attribute is always "id", that is harcoded in XTF :(

Comment 16

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

Comment on attachment 193131 [details] [diff] [review]
Patch using GetIDAttributeName()


>+
>+  PRUint32 i;
>+  PRUint16 type;
>+  nsAutoString idVal;
>+  nsCOMPtr<nsIDOMNode> childNode;
>+  for (i = 0; i < elements; ++i) {
>+    descendants->Item(i, getter_AddRefs(childNode));
>+    childNode->GetNodeType(&type);
>+    if (type == nsIDOMNode::ELEMENT_NODE) {
>+      nsCOMPtr<nsIContent> content(do_QueryInterface(childNode));
>+      NS_ASSERTION(content, "An ELEMENT_NODE not implementing nsIContent?!");
>+      content->GetAttr(kNameSpaceID_None,
>+                       content->GetIDAttributeName(),
>+                       idVal);
>+      if (idVal.Equals(aId)) {

Could you do
rv = content->GetAttr(kNameSpaceID_None,
		      content->GetIDAttributeName(),
		      idVal);
if (rv == NS_CONTENT_ATTR_HAS_VALUE && idVal.Equals(aId)) {

I think that might be a bit faster.
With that r=me

Comment 17

[Allan Beaufour]

(In reply to comment #15)
> (In reply to comment #13)
> > Why? The id attribute is not defined anywhere in the XForms spec. The id
> > attribute comes from the host language.
> > "A host language must permit an attribute of type xsd:ID on each XForms
element."
> > [http://www.w3.org/TR/xforms/slice3.html#structure-attrs-common]
> 
> Ah, you're right. I wonder how to decide what is actually the host language, if 
> the document contains elements from several "languages"

Frankly, I do not know. It's been discussed in the WG a couple of times just to
include the id attribute in XForms instead. The above leads to problems again
and again.

> But in our case, the ID attribute is always "id", that is harcoded in XTF :(

:(

Comment 18

[Allan Beaufour]

(In reply to comment #16)
> Could you do
> rv = content->GetAttr(kNameSpaceID_None,
> 		      content->GetIDAttributeName(),
> 		      idVal);
> if (rv == NS_CONTENT_ATTR_HAS_VALUE && idVal.Equals(aId)) {
> 
> I think that might be a bit faster.

Right on! I was thinking of HasAttr() but thought it was too slow, forgot that
the  return value actually has the info.

Comment 19

[Allan Beaufour]

Attachment: w/smaug's comments

Comment 20

[Allan Beaufour]

Checked in to trunk

Comment 21

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

Removing xf-to-branch.