416134 - Parent microformats should not get properties from nested microformats

Status: RESOLVED FIXED

(Toolkit Graveyard :: Microformats, defect)

Description

[Mike Kaply [:mkaply]]

There has been an ongoing debate in the community about this subject, and we've finally decided that parent microformat should not see properties from nested microformats.

The example makes this clear why.

I have a patch and testcase, but things are getting mixed in with all the other stuff, so it will be here soon.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Fix + test

Enumerate through propnodes and if their parent is a known microformat, don't use the property as part of a parent property.

We have to ignore adr and geo for hCard because when adr and geo are in an hCard, they aren't really their own microformat (that is, the information can bubble up to the hCard)

Comment 2

[Mike Kaply [:mkaply]]

This patch contains previous patch. Sorry about that.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Fix + test

Fix that doesn't include other patch, plus I've changed it to traverse backwards similar to the previous patch so it doesn't use i-- in the for loop.

Comment 4

[Robert Sayre]

Comment on attachment 303042 [details] [diff] [review]
Fix + test


>+      for (let i=propnodes.length-1; i >= 0; i--) {
>+        /* Can't use getParent because I don't want to use attribute */
>+        /* microformats plus I need to special case adr and geo in hCard */

No first person in comments.

>+          if (Microformats[Microformats.list[j]].className) {
>+            if (!first) {
>+              xpathExpression += " or ";
>+            } else {
>+              first = false;
>+            }

I don't understand this conditional. Is 'first' null or undefined and then set to false intentionally? If so, needs a comment with explanation. If not, delete the else branch.

>+        /* If the propnode is not a child of the microformat, */
>+        /* Remove it */

Strange comment style.

Comment 5

[Mike Kaply [:mkaply]]

Yeah, that for loop is ugly.

Basically the problem is that I need to skip something when I process the first microformat, but I have no guarantee that the first microformat I process will be [0] in the array.

So I'm basically trying to have way to know when the thing I'm processing is not the first one.

There's probably a better way to do this, but I'm not able to think of it.

Open to any suggestions.

I'll fix the other issues.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Better patch per review comments

OK, what I've done is just not initialize my xpathExpression string until inside my for loop.

That way I can use the fact that the string is empty or not empty to decide to put the " or " in. This seems more elegant to me.

Comments fixed as well.

Comment 7

[Mike Kaply [:mkaply]]

Fix checked in.