Closed Bug 427456 Opened 12 years ago Closed 12 years ago

Frame recursion option is ignored when using Microformats API

Categories

(Toolkit Graveyard :: Microformats, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: mkaply)

References

Details

(Keywords: addon-compat, Whiteboard: [has patch][has review][has approval])

Attachments

(4 files, 2 obsolete files)

Attached file mochitest for this. (obsolete) —
While working with some automated tests for Microformats, I stumbled upon this bug.  

1. Query for microformats and set {recurseFrames: false}
2. Note that it still finds microformats in <frame> tags

After more investigation, I realized that it is counting items in <frame> tags no matter what this setting is, and it *never* counts items in <iframe> tags.

I'm not sure if the <iframe> part is by design or also part of this bug.  If it is by design, then this needs to be documented.

== Expected ==
if {recurseFrames: false} then we should not count items in frames
And if it is true, we should count them and we should also count items in iframes.

== Actual ==
As described above, always counts items in frames and never counts them in iframes.

Attaching a testcase
Blocks: 427457
Assignee: nobody → mozilla
Component: General → Microformats
Product: Firefox → Toolkit
QA Contact: general → microformats
You can't generate an iframe as referenced in this testcase. If you were to display this to the glass, you would see the iframe is empty.

If you put the geo in a separate file and use:

<iframe src="geo.html">

You'll see both microformats.

Also, one other thing - 

<abbr> class="foo">I am not a microformat</abbr>
<abbr> class="title">Foolish title, not a format</abbr>

should be

<abbr class="foo">I am not a microformat</abbr>
<abbr class="title">Foolish title, not a format</abbr>

This surprised me as well. I thought you could do this with an iframe.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Attached file The geo.html I'm using
(In reply to comment #1)
> You can't generate an iframe as referenced in this testcase. If you were to
> display this to the glass, you would see the iframe is empty.
> 
> If you put the geo in a separate file and use:
> 
> <iframe src="geo.html">
> 
> You'll see both microformats.
True, that would be the more common method of implementing an iframe'd microformat anyway.  However, I'm sorry to report that when I change the test to do this (the geo.html I'm using is attached).  I still get the same errors: I get one MF reported when attempting with recurseframes: False and one with recurseFrames: true.

If I comment out the geo in the <frame> tag, then I get 0 microformats returned from my count calls, which is good.  But that indicates to me that the geo in the <frame> tag is still being counted even with recurseFrames: False, and the geo in the iframe is never counted.  And as such the test still fails. 

I apologize about the typos, I'll attach a new test.

I'm going to reopen the bug for now, because it seems to me that the test still fails even with the changes you suggested.  I'll be happy to close it again if we can determine that it is indeed invalid.
Attached file the corrected mochitest (obsolete) —
Attachment #313999 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
OK, I think i understand what's going on. There are two separate problems here.

In the first case where the test fails, it's because the frame is defined in the document, it is not an "external frame". So when I run XPath, I get that microformat.

I'm wondering if the option should be "recurseExternalFrames". That was really the intent. I don't know how to prevent recursing into frames specifically.

In the second case, the problem is that to determine the frames, I'm looking for the defaultview on the passed in root element. Since the passed in root element isn't the document, I don't get any default views.

This brings up a very interesting problem. How do I know that the frames I'm recursing into are children of the passed in element. I can't just go to the document frames array.

In this case, I'm tempted to only have recurseFrames work if the passed in element has a frames array.

In both cases, though, I'm a little stumped.

You've done a great job on these testcases :)
OK, so what this fix does is check to see if the passed in rootElement doesn't have a defaultView and it it doesn't use the one for the ownerDocument.

Then when it gets frames, it checks to make sure they are children of the passed in rootElement.

This also fixes a bug where I was using content.document which doesn't work. rootElement is REQUIRED.
Attachment #315636 - Flags: review?(sayrer)
Attachment #315636 - Flags: review?(sayrer) → review+
Attachment #315636 - Flags: approval1.9?
(In reply to comment #5)
> Created an attachment (id=315636) [details]
> Fix for second problem
> 
I've patched my build, but I'm not seeing any difference when I run the test case as is.  From your comments, I'd have expected the second check to pass while the first should still fail, unless I misunderstood.

Should I change the root element in the testcase?

Comment on attachment 315636 [details] [diff] [review]
Fix for second problem

Please address comment 6 and re-nominate.
Attachment #315636 - Flags: approval1.9? → approval1.9-
The problem is that second iframe hasn't loaded yet before the test is run.

We probably need to use an onload handler on the second iframe before running the test.
(In reply to comment #8)
> The problem is that second iframe hasn't loaded yet before the test is run.
> 
> We probably need to use an onload handler on the second iframe before running
> the test.
> 
Ok.  I've put an onload handler into the iFrame.  The once the onload handler is called the test_MicroformatsAPI function is called (no other changes to the test).
Here are the results:
* Without the patch - both tests fail.
* With the patch, the second failure is indeed fixed, as mkaply noted in comment 5.

I'd recommend approving the patch for landing based on the fact that it does fix what will likely be a more common problem "in the wild".  Of course, we still need to decide what to do about the first failure (which still fails even with the patch and the onload handler, as expected).

This is the test with the onload handler.  I just kick off the entire test once the iFrame loads, this was the easiest way to do it, and it had the extra benefit of not modifying the actual tests that are run.
Attachment #314594 - Attachment is obsolete: true
Comment on attachment 315636 [details] [diff] [review]
Fix for second problem

The issue was in the testcase. We have updated the testcase to handle this case.
Attachment #315636 - Flags: approval1.9- → approval1.9?
Clint:

There are basically two choices here, and I wanted your opinion.

1. Rename recurseFrames to recurseExternalFrames and document it as only applying to frames that reference external files.

2. Remove recurseFrames completely and just always recurse frames.

What do you think?
Comment on attachment 315636 [details] [diff] [review]
Fix for second problem

a=beltzner, thanks for continuing to look into the tests for this :)
Attachment #315636 - Flags: approval1.9? → approval1.9+
(In reply to comment #12)
> Clint:
> 
> There are basically two choices here, and I wanted your opinion.
> 
> 1. Rename recurseFrames to recurseExternalFrames and document it as only
> applying to frames that reference external files.
> 
> 2. Remove recurseFrames completely and just always recurse frames.
> 
> What do you think?
> 
I like the idea of renaming it.  I can envision some use cases where you might use recurseFrames: False setting to ensure that microformat detection doesn't adversely impact page load performance. 
Per the discussion, we're going to rename recurseFrames to recurseExternalFrames and document it.
Attachment #317053 - Flags: review?(sayrer)
Need the rename of the API before ship (last attachment)
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Keywords: late-compat
Whiteboard: [needs toolkit peer review]
Attachment #317053 - Flags: review?(sayrer) → review+
Whiteboard: [needs toolkit peer review] → [needs landing]
Comment on attachment 317053 [details] [diff] [review]
Rename recurseFrames to recurseExternalFrames

a1.9=beltzner
Attachment #317053 - Flags: approval1.9+
Did this land?
Whiteboard: [needs landing] → [has patch][has review][has approval]
landed. sorry for the delay.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 527217
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.