Last Comment Bug 302513 - Figure out id attributes inside repeats
: Figure out id attributes inside repeats
Status: RESOLVED FIXED
: fixed1.8, testcase
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/
: 302520 (view as bug list)
Depends on:
Blocks: 264329
  Show dependency treegraph
 
Reported: 2005-07-28 08:12 PDT by Allan Beaufour
Modified: 2005-09-28 13:58 PDT (History)
4 users (show)
allan: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase: Switch inside repeat (2.94 KB, application/xhtml+xml)
2005-07-28 08:32 PDT, Allan Beaufour
no flags Details
Testcase: Switch inside repeat v2 (3.29 KB, application/xhtml+xml)
2005-08-18 06:34 PDT, Allan Beaufour
no flags Details
Testcase: Focus inside repeat (2.43 KB, application/xhtml+xml)
2005-08-18 06:34 PDT, Allan Beaufour
no flags Details
Patch for toggle and setfocus (10.05 KB, patch)
2005-08-18 06:36 PDT, Allan Beaufour
no flags Details | Diff | Review
Testcase: Dispatch inside repeat (2.62 KB, patch)
2005-08-18 08:02 PDT, Allan Beaufour
no flags Details | Diff | Review
Testcase: Dispatch inside repeat v2 (3.30 KB, application/xhtml+xml)
2005-08-18 08:54 PDT, Allan Beaufour
no flags Details
Patch for toggle, setfocus, and dispatch (12.95 KB, patch)
2005-08-18 09:59 PDT, Allan Beaufour
bugs: review-
Details | Diff | Review
Patch using GetIDAttributeName() (13.28 KB, patch)
2005-08-18 23:57 PDT, Allan Beaufour
bugs: review+
Details | Diff | Review
w/smaug's comments (14.97 KB, patch)
2005-08-19 02:24 PDT, Allan Beaufour
doronr: review+
Details | Diff | Review

Description Allan Beaufour 2005-07-28 08:12:52 PDT
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 2005-07-28 08:32:51 PDT
Created attachment 190845 [details]
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 2005-07-28 23:37:16 PDT
*** Bug 302520 has been marked as a duplicate of this bug. ***
Comment 3 Allan Beaufour 2005-08-18 04:21:44 PDT
Patch coming up
Comment 4 Allan Beaufour 2005-08-18 06:34:01 PDT
Created attachment 193052 [details]
Testcase: Switch inside repeat v2
Comment 5 Allan Beaufour 2005-08-18 06:34:32 PDT
Created attachment 193053 [details]
Testcase: Focus inside repeat
Comment 6 Allan Beaufour 2005-08-18 06:36:24 PDT
Created attachment 193054 [details] [diff] [review]
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 2005-08-18 08:02:50 PDT
Created attachment 193059 [details] [diff] [review]
Testcase: Dispatch inside repeat
Comment 8 Allan Beaufour 2005-08-18 08:54:32 PDT
Created attachment 193064 [details]
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 2005-08-18 09:59:38 PDT
Created attachment 193071 [details] [diff] [review]
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] 2005-08-18 10:23:38 PDT
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 2005-08-18 10:31:30 PDT
(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] 2005-08-18 10:46:38 PDT
(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 2005-08-18 23:29:19 PDT
(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 2005-08-18 23:57:03 PDT
Created attachment 193131 [details] [diff] [review]
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] 2005-08-19 01:29:22 PDT
(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] 2005-08-19 01:37:35 PDT
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 2005-08-19 02:18:08 PDT
(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 2005-08-19 02:19:29 PDT
(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 2005-08-19 02:24:45 PDT
Created attachment 193141 [details] [diff] [review]
w/smaug's comments
Comment 20 Allan Beaufour 2005-08-23 04:01:14 PDT
Checked in to trunk
Comment 21 Olli Pettay [:smaug] 2005-09-28 13:56:59 PDT
Removing xf-to-branch.

Note You need to log in before you can comment on or make changes to this bug.