Closed Bug 870787 Opened 7 years ago Closed 6 years ago

Improve named getter for form

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: baku)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 13 obsolete files)

24.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
57.34 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
See discussion from yesterday: <http://logbot.glob.com.au/?c=mozilla%23content&s=9+May+2013&e=9+May+2013>. I think the first step here is writing some serious tests to see what other browsers do and if the spec is somewhat sane*. Bonus points for testharness.js tests we can submit to the spec test suite.

* It doesn't look like it is; I already filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=21997>.
Keywords: dev-doc-needed
Attached patch mochitest (obsolete) — Splinter Review
Attachment #753215 - Flags: feedback?(Ms2ger)
Attached patch patch (obsolete) — Splinter Review
What about this?
Attachment #753215 - Attachment is obsolete: true
Attachment #753215 - Flags: feedback?(Ms2ger)
Attachment #753284 - Flags: review?(bzbarsky)
Comment on attachment 753284 [details] [diff] [review]
patch

Won't this fail testcases like:

  <table><form><tr><td><input name="foo">

and then getting .foo on the form?
Attached patch patch (obsolete) — Splinter Review
No, it works. But I added it in the mochitest to be sure.
Attachment #753284 - Attachment is obsolete: true
Attachment #753284 - Flags: review?(bzbarsky)
Attachment #753740 - Flags: review?(bzbarsky)
Comment on attachment 753740 [details] [diff] [review]
patch

Ah!  And it works because we check the controls explicitly before deferring to the document.  So the only thing we depend on for the document here is <img>, I guess.

So we need another test, like so:

 <table>
   <form id="form3">
   <tr><td><img name="img4">
 </table>

and it should presumably check that form3.img4 does not exist.  But it's worth checking what other UAs do in that case.
And also, that patch is a first step, but the end goal of this bug is to get us to actually matching the spec and to wean form off the document completely for this stuff, right?
Attached patch WIP (obsolete) — Splinter Review
Attachment #753740 - Attachment is obsolete: true
Attachment #753740 - Flags: review?(bzbarsky)
Attachment #753784 - Flags: review?(bzbarsky)
Comment on attachment 753784 [details] [diff] [review]
WIP

>+ok(form1.img2, "Form1.img2 exists");

Worth actually checking that it's the right element too.

>+ok(!form3.img4, "Form3.img4 doesn't exists");

As we just discussed, this should actually exist, since it does so interoperably across all browsers.

As long as we're adding tests for this stuff, can we exercise the past names map too?  And what happens when multiple things with the same name are present?  Multiple tests to exercise the various things is fine.

r=me on the mochitest with the above caveats.
Attachment #753784 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 753740 [details] [diff] [review]
> patch
> 
> Ah!  And it works because we check the controls explicitly before deferring
> to the document.  So the only thing we depend on for the document here is
> <img>, I guess.
> 
> So we need another test, like so:
> 
>  <table>
>    <form id="form3">
>    <tr><td><img name="img4">
>  </table>
> 
> and it should presumably check that form3.img4 does not exist.  But it's
> worth checking what other UAs do in that case.

Apparently in IE, form3.img4 is undefined; it's the img in Gecko/Blink. If we can get away with matching IE, maybe we should just do that.
Attached patch patch (obsolete) — Splinter Review
This works but let me know if you like this patch.
Attachment #753784 - Attachment is obsolete: true
Attachment #754503 - Flags: feedback?(bzbarsky)
(In reply to :Ms2ger from comment #10)
> Apparently in IE, form3.img4 is undefined; it's the img in Gecko/Blink. If
> we can get away with matching IE, maybe we should just do that.

If we can get away with matching IE, we should do so instead of adding more craziness. However, in the light of https://www.w3.org/Bugs/Public/show_bug.cgi?id=21957 please be sure to check IE older than IE10, too.
> Apparently in IE, form3.img4 is undefined

In which exact IE, and in which mode?
Some pre-release IE10 that I downloaded at some point; it seems like released IE10 does return the image. See some discussion starting at <http://krijnhoetmer.nl/irc-logs/whatwg/20130528#l-323>.
We can try to not make <img> form-associated in the table case, but I worry about site compat...
Comment on attachment 754503 [details] [diff] [review]
patch

I assume this is aiming for the "images are form-associated" behavior, right?

My first question is why we're still calling into the document in any way here and why we're storing the images in a separate hashtable from the other stuff.
Attachment #754503 - Flags: feedback?(bzbarsky) → feedback+
> I assume this is aiming for the "images are form-associated" behavior, right?

yes.

> My first question is why we're still calling into the document in any way
> here and why we're storing the images in a separate hashtable from the other
> stuff.

The separate hashtable is because images are used as fallback in the indexing.
If you have an input and an image with the same name, input should return and not the image.

About the document, probably I can remove that part. I have to check.
> The separate hashtable is because images are used as fallback in the indexing.

Ah, I see.  That's worth documenting!
In which case, let me read through this more carefully...
From specs:

    Let candidates be a live RadioNodeList object containing all the listed elements that are descendants of the form element and that have either an id attribute or a name attribute equal to name, in tree order.

    If candidates is empty, let candidates be a live RadioNodeList object containing all the img elements that are descendants of the form element and that have either an id attribute or a name attribute equal to name, in tree order.
Yes, I saw that in the spec.  I'm saying it's worth documenting in our code why we have the separate hashtable.
Attached patch patch (obsolete) — Splinter Review
Attachment #754503 - Attachment is obsolete: true
Attachment #755628 - Flags: review?(bzbarsky)
Comment on attachment 755628 [details] [diff] [review]
patch

> @@ -2397,35 +2397,16 @@ nsContentUtils::BelongsInForm(nsIContent

I believe that with this patch this function is only used from nsFormContentList.  Which with this patch is itself unused, so can go away, as can this function.  Please just kill them both, and thank you!

>+++ b/content/base/src/nsNodeUtils.cpp
>+    nsCOMPtr<nsIDOMHTMLImageElement> domImageElem(do_QueryInterface(aNode));
>+    if (domImageElem) {

How about:

  if (aNode->IsElement() && aNode->AsElement()->IsHTML(nsGkAtoms::img)) {
    
>+++ b/content/html/content/src/HTMLImageElement.cpp
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mForm)

Why are we not unlinking this?  Alternately, why are we storing a strong ref here, not a weak one like nsGenericHTMLFormElement?

>+HTMLImageElement::GetParentObject() const

No, I don't think we want this.  The fact that images are kinda form-associated isn't exposed to JS directly and shouldn't affect the event handler scope chain.  Just remove this GetParentObject method, please.

>@@ -407,16 +419,18 @@ HTMLImageElement::BindToTree(nsIDocument
>+  UpdateFormOwner();

Might be worth conditioning that on aParent, perhaps.

>+      UnsetFlags(MAYBE_ORPHAN_FORM_ELEMENT);

We should document on the ADDED_TO_FORM and MAYBE_ORPHAN_FORM_ELEMENT bits that they're used for form elements _and_ images.

>+HTMLImageElement::UpdateFormOwner()
>+    // Notify only if we just found this mForm.

Drop that comment, please.  We don't do any notifying here.

I see nothing here that updates the table in the form when the name or id of the HTMLImageElement changes.  Please add tests for that and then add the relevant code.  See nsGenericHTMLFormElement::BeforeSetAttr and nsGenericHTMLFormElement::AfterSetAttr for what probably needs to happen.

>+++ b/content/html/content/src/HTMLImageElement.h
>+#ifdef DEBUG
>+  nsIDOMHTMLFormElement* GetForm() const;
>+#endif

The definition is not conditional on DEBUG, so this will fail to build in an opt build.  Please either make this unconditional or make the definition conditional.

>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+  virtual nsINode* GetParentObject() const;

No, this is not supposed to be virtual.

>+++ b/content/html/content/src/nsHTMLFormElement.cpp

>+  nsISupports* ImageNamedItemInternal(const nsAString& aName);

How about calling this GetImageNamedItem() or something?  The "internal" bit seems irrelevant here.

That said, is there a reason this is on the form control list and not just on the form?

>+  nsTArray<HTMLImageElement*> mImageElements;  // Holds WEAK references

And this too.  I guess it more closely parallels existing code, but there is no real reason for it otherwise.  At least document that we're doing this to parallel the other thing, though the form control list never uses these members itself?  But I'd vaguely prefer a followup bug on moving these to the form and strongly prefer just doing it unless there's some reason not to that I'm missing.  AddElementToTableInternal and RemoveElementFromTableInternal would need to become a non-class static methods, but that seems ok (and perhaps they should do that anyway, since they no longer use any nsFormControlList members).

>+  nsInterfaceHashtable<nsStringHashKey,nsISupports> mImageNameLookupTable;

And here.

>+MarkOrphans(const nsTArray<HTMLImageElement*>& aArray)

Why duplicate this?  Just change the existing one to be:

  template<typename T>
  static void
  MarkOrphans(const nsTarray<T*>& aArray)

and then you can use it for both arrays.

> CollectOrphans(nsINode* aRemovalRoot,

This one is more of a pain to reuse the code for, because we actually want it to be slightly different.  We could do it with enough template games, but the result would be no shorter. :(

>+  // Put a script blocker around all the notifications we're about to do.
>+  nsAutoScriptBlocker scriptBlocker;

Not needed; there is no change in an image's state when its form-association changes.

>        See
>+    // also the code in nsGenericHTMLFormElement::FindForm.

This should be pointing to image element code somewhere, right?

>+        // When a form control loses its form owner, its state can change.
>+        node->UpdateState(true);

Not for an image, though.  Get rid of that bit, please.

>@@ -1371,34 +1449,43 @@ nsHTMLFormElement::FindNamedItem(const n
>+  result = DoResolveImageName(aName);

There's no reason to have a separate DoResolveImageName, imo.  DoResolveName exists because it has multiple callsites.  But this is the only place that cares about image names, right?  So you can just mControls->GetNamedImage() directly.

>+nsFormControlList::AddElementToTableInternal(
>+    aTable.Put(aName, static_cast<nsISupports*>(aChild));

No need for the cast.  The cast used to be needed because of ambiguous inheritance from nsISupports, but now the type is nsIContent, which singly-inherits nsISupports.

>-      list->AppendElement(newFirst ? aChild : content);
>-      list->AppendElement(newFirst ? content : aChild);
>+      list->AppendElement(newFirst ? aChild : content.get());
>+      list->AppendElement(newFirst ? content.get() : aChild);

Why these changes?

> nsFormControlList::GetSupportedNames(nsTArray<nsString>& aNames)
>   mNameLookupTable.EnumerateRead(CollectNames, &aNames);
>+  mImageNameLookupTable.EnumerateRead(CollectNames, &aNames);

That's definitely wrong.  This will cause something like this:

  <form id="f"><img id="foo"></form>
  <script>
    alert("foo" in document.getElementById("f").elements)
  </script>

to alert true, when it should alert false.  Yet another reason to put the table on the form itself.  ;)

Please add a test for this?

Please take that line out.

>+nsHTMLFormElement::AddImageElement(HTMLImageElement* aChild)

This duplicates a lot of code from AddElement.  How about we add a:

  template<typename ElementType>
  static void
  AddElementToList(nsTArray<ElementType*>& aList, ElementType* aElement)

which does all the binary search bits and whatnot and then call that from both here and from AddElement?

>+++ b/content/html/content/src/nsHTMLFormElement.h
>+   * removed because the id attribute has changed, or bacause the

s/bacause/because/

And also, we can't fold it in because we only call RemoveImageElement when the image stops being form-associated, but should be doing RemoveImageElementFromTable on attribute changes!  

>+   * Add an image element to end of this form's list of image elements

"to the end"

>+   * We can't fold this method into AddImageElement() because when
>+   * AddImageElement() is called, the image has no attributes.

Again, the important reason we can't is that attributes can change.

>+   * Find images in this form with the correct value in the name
>+   * attribute.

Name or id attribute.

>+++ b/parser/html/nsHtml5TreeBuilder.cpp

I'd like Henri to review this bit.  I have no idea whether this is catching all the cases that matter.  :(

>+++ b/parser/html/nsHtml5TreeOperation.cpp
>+      nsCOMPtr<nsIDOMHTMLImageElement> domImageElement = do_QueryInterface(node);
>+      if (domImageElement) {
>+        nsRefPtr<dom::HTMLImageElement> imageElement =
>+          static_cast<dom::HTMLImageElement*>(domImageElement.get());

See above for the fast way to check for an <img> using IsHTML().  And no need for the strong nsRefPtr.

r=me with the above comments fixed.
Attachment #755628 - Flags: review?(bzbarsky) → review+
Oh, and please make sure there's an interdiff?  I'd really love to not have to reread all of that!
Comment on attachment 755628 [details] [diff] [review]
patch

Henri, can you please review the parser changes at the end?
Attachment #755628 - Flags: review?(hsivonen)
(In reply to Boris Zbarsky (:bz) from comment #23)
> Comment on attachment 755628 [details] [diff] [review]
> patch
> 
> > nsFormControlList::GetSupportedNames(nsTArray<nsString>& aNames)
> >   mNameLookupTable.EnumerateRead(CollectNames, &aNames);
> >+  mImageNameLookupTable.EnumerateRead(CollectNames, &aNames);
> 
> That's definitely wrong.  This will cause something like this:
> 
>   <form id="f"><img id="foo"></form>
>   <script>
>     alert("foo" in document.getElementById("f").elements)
>   </script>
> 
> to alert true, when it should alert false.

I'm not sure that it would. It would make this return true, though:

for (var p in document.getElementById("f").elements) {
  if (p === "foo") {
    return true;
  }
}
return false;
> It would make this return true, though:

Er, yes.  Or even more simply:

  alert(Object.getOwnPropertyNames(document.getElementById("f").elements).indexOf("foo"));

should alert -1 but won't.
> >-      list->AppendElement(newFirst ? aChild : content);
> >-      list->AppendElement(newFirst ? content : aChild);
> >+      list->AppendElement(newFirst ? aChild : content.get());
> >+      list->AppendElement(newFirst ? content.get() : aChild);
> 
> Why these changes?

error: operands to ?: have different types ‘nsIContent*’ and ‘nsCOMPtr<nsIContent>’
Attached patch patch (obsolete) — Splinter Review
Attachment #755628 - Attachment is obsolete: true
Attachment #755628 - Flags: review?(hsivonen)
Attachment #757967 - Flags: review?(hsivonen)
Blocks: 879319
Attached patch patch (obsolete) — Splinter Review
Sorry for spamming.
Attachment #757967 - Attachment is obsolete: true
Attachment #757967 - Flags: review?(hsivonen)
Attachment #758015 - Flags: review?(hsivonen)
> error: operands to ?: have different types ‘nsIContent*’ and ‘nsCOMPtr<nsIContent>’

Hmm.  How did this use to work?

Is that last patch addressing some set of review comments?  If so, interdiff?
Comment on attachment 758015 [details] [diff] [review]
patch

>diff --git a/parser/html/nsHtml5TreeBuilder.cpp b/parser/html/nsHtml5TreeBuilder.cpp
>--- a/parser/html/nsHtml5TreeBuilder.cpp
>+++ b/parser/html/nsHtml5TreeBuilder.cpp
>@@ -3941,17 +3941,18 @@ nsHtml5TreeBuilder::appendVoidElementToC
>   elementPushed(kNameSpaceID_XHTML, name, elt);
>   elementPopped(kNameSpaceID_XHTML, name, elt);
> }
> 
> void 
> nsHtml5TreeBuilder::appendVoidElementToCurrentMayFoster(nsHtml5ElementName* elementName, nsHtml5HtmlAttributes* attributes)
> {
>   nsIAtom* popName = elementName->name;
>-  nsIContent** elt = createElement(kNameSpaceID_XHTML, popName, attributes);
>+  nsIContent** elt = createElement(kNameSpaceID_XHTML, popName, attributes,
>+    elementName == nsHtml5ElementName::ELT_IMG && !fragment ? formPointer : nullptr);

This looks like you are editing a generated .cpp instead of editing the .java it is generated from. r- on landing manual edits to generated files. However, if you don't already happen to have the corresponding .java edits, I can make them, since it would be annoying for you to have to get the generator up and running just for this. Shall I make the .java edits?

Also, I think there should be a spec bug on file for this parser change before we land the change. Is there a spec bug on file?

This will change to r+ once the .java edits and the spec bug have been taken care of.

>       // TODO: uncomment the above line when <keygen> (bug 101019) is supported by Gecko
>       nsCOMPtr<nsIDOMHTMLFormElement> formElement(do_QueryInterface(parent));
>       NS_ASSERTION(formElement, "The form element doesn't implement nsIDOMHTMLFormElement.");
>       // avoid crashing on <keygen>
>       if (formControl &&
>           !node->HasAttr(kNameSpaceID_None, nsGkAtoms::form)) {
>         formControl->SetForm(formElement);
>       }
>+

putting an "else" here would make sense and avoid useless checks.

>+      if (node->IsElement() && node->AsElement()->IsHTML(nsGkAtoms::img)) {
>+        dom::HTMLImageElement* imageElement = static_cast<dom::HTMLImageElement*>(node);
>+        MOZ_ASSERT(imageElement);
>+        imageElement->SetForm(formElement);
>+      }
Attachment #758015 - Flags: review?(hsivonen) → review-
> Is that last patch addressing some set of review comments?  If so, interdiff?

Here:

-nsFormControlList::AddElementToTable(nsGenericHTMLFormElement* aChild,
-                                     const nsAString& aName)
+static nsresult
+AddElementToTableInternal(
+  nsInterfaceHashtable<nsStringHashKey,nsISupports>& aTable,
+  nsIContent* aChild, const nsAString& aName, nsHTMLFormElement* aForm)

...

-      list->AppendElement(newFirst ? aChild : content);
-      list->AppendElement(newFirst ? content : aChild);
+      list->AppendElement(newFirst ? aChild : content.get());
+      list->AppendElement(newFirst ? content.get() : aChild);
> I can make them, since it would be annoying for you to have to get the
> generator up and running just for this. Shall I make the .java edits?

wow. Thanks!

> Also, I think there should be a spec bug on file for this parser change
> before we land the change. Is there a spec bug on file?

I don't think so.
Attached patch interdiff (obsolete) — Splinter Review
Attachment #758610 - Flags: review?(bzbarsky)
Comment on attachment 758610 [details] [diff] [review]
interdiff

Something went through and did lots of unrelated whitespace changes here.  :(  I guess just leave them in....

>+++ b/content/html/content/src/HTMLImageElement.cpp
>+HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+  if (aNameSpaceID == kNameSpaceID_None) {

This should probably just be:

  if (aNameSpaceID == kNameSpaceID_None && mForm &&
      (aName == nsGkAtoms::name || aName == nsGkAtoms::id)) {

because there's no reason to create the nsAutoString otherwise.
 
>+    if (mForm && aName == nsGkAtoms::type) {

The "type" attribute on images, unlike form controls, is meaningless.  Please take this entire bit out: only name and id changes matter here.

>+HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,

Similar comments here.

>+++ b/content/html/content/src/HTMLImageElement.h
>+  virtual nsresult BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+  virtual nsresult AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,

Both of those should be MOZ_OVERRIDE.

>+  nsHTMLFormElement* mForm;

Document that this is a weak reference that we and the form cooperate in maintaining?

>+++ b/content/html/content/src/nsGenericHTMLElement.h

>+  // that means that we have/ added ourselves to our mForm.  It's possible to

Stray '/' there.

>+++ b/content/html/content/src/nsHTMLFormElement.cpp

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsHTMLFormElement,
>+                                                  nsGenericHTMLElement)

Please fix the indent.

>+ * Compares the position of aImage1 and aImage2 in the document

How about we make CompareFormControlPosition take Element* for its first two arguments, and then we can just use it equally well for nsGenericHTMLFormElement* and HTMLImageElement*?  That seems better than duplicating it and adding more complexity to AddElementToList.

>+static bool
>+AddElementToList(nsTArray<ElementType*>& aList, ElementType* aChild,

Please document what the return value means.

>+  NS_ASSERTION(aChild->GetParent(),
>+               "Form element should have a parent");

That's not a valid assertion; consider this XHTML snippet:

  <input form="x">
    <form id="x"/>
  </input>

That's why right now AddElement() asserts something weaker than this..  Please take this assert out.

>+  NS_ASSERTION(aList.IndexOf(aChild) == aList.NoIndex,
>+               "FormElement already in form");

Not sure what FormElement is.  Maybe "aChild is already in aList"?

>+AddElementToTableInternal(
>     aTable.Put(aName, static_cast<nsISupports*>(aChild));

Again, drop the cast.

r=me with those fixed.
Attachment #758610 - Flags: review?(bzbarsky) → review+
So when I looked at TreeBuilder.java, I realized that this was more than just a matter of working backwards from the C++ change, since the C++ change didn't fit well with the idioms of the parser code.

Now that I've done more than just ported baku's change to Java, I guess *this* patch needs review. Consider this patch superseding the change for nsHtml5TreeBuilder.cpp in baku's patch.

This is clearly a spec violation per the current state of the spec, so I still think we shouldn't land this without making sure a spec bug is on file. Specifically, getting "img" needs to be added to the list at http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#form-associated-element for this patch to agree with the spec.
Attachment #759016 - Flags: review?(bugs)
Attached patch patch (obsolete) — Splinter Review
Do you need an interdiff? I removed the HTML5 parser changes.
Attachment #758015 - Attachment is obsolete: true
Attachment #758610 - Attachment is obsolete: true
We need more of a spec change than that, because we don't want to expose a .form on <img>.  So we want <img> to be form-associated internally but not have any of the external trappings of it.
> Do you need an interdiff?

I'd like one for addressing comment 36, yes.
Comment on attachment 759016 [details] [diff] [review]
Make the portable part of the parser treat img as a form-associated element

But yes, we need a spec change.
Attachment #759016 - Flags: review?(bugs) → review+
Attached patch interdiff (obsolete) — Splinter Review
Here interdiff for comment 36
Attachment #760814 - Flags: review?(bzbarsky)
Comment on attachment 760814 [details] [diff] [review]
interdiff

>+++ b/content/html/content/src/HTMLImageElement.cpp
> HTMLImageElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+    // remove the control from the hashtable as needed

s/control/image/.  And maybe "the form's hashtable"?

> HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>                               const nsAttrValue* aValue, bool aNotify)

Fix the indent?

>     // add the control to the hashtable as needed

And s/control/image/ and "the form's hashtable" here.

r=me.  Thank you for the interdiff!
Attachment #760814 - Flags: review?(bzbarsky) → review+
Attached patch form patch (obsolete) — Splinter Review
Attachment #759062 - Attachment is obsolete: true
Attachment #760814 - Attachment is obsolete: true
Talking with smaug on IRC I can say that:

chrome and opera implement the behaviour that this patch introduces.
IE9, does something similar, but it fails with Form2.input2 - it doesn't exist but it should.

I think we can land these patches.
2 patches to land.
Keywords: checkin-needed
Attached patch patch (obsolete) — Splinter Review
rebased
Attachment #761198 - Attachment is obsolete: true
I checked these patches in as:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/fd68811a4d63
  https://hg.mozilla.org/integration/mozilla-inbound/rev/43927377e41c

Then I backed out the non-parser part because it caused M1 test failures:

11:20:36 INFO - 137067 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug870787.html | Form3.img4 doesn't exists - got undefined, expected [object HTMLImageElement]
11:20:36 INFO - 137068 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug870787.html | Form3.img4id doesn't exists - got undefined, expected [object HTMLImageElement]
11:20:36 INFO - 137069 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug870787.html | Form3.img5 doesn't exists - got undefined, expected [object HTMLImageElement]
11:20:36 INFO - 137070 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug870787.html | Form3.img5id doesn't exists - got undefined, expected [object HTMLImageElement]

The backout is part of https://hg.mozilla.org/integration/mozilla-inbound/rev/26696aad9b5b
Keywords: checkin-needed
Whiteboard: [leave open]
Attached patch patchSplinter Review
Can you just review the HTML5 parser part?
Attachment #763597 - Attachment is obsolete: true
Attachment #763768 - Flags: review?(hsivonen)
Comment on attachment 763768 [details] [diff] [review]
patch

r+ on the nsHtml5TreeOperation part.
Attachment #763768 - Flags: review?(hsivonen) → review+
Just the patch 'patch' has to be landed.
Thanks Henri for the review!
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b2e2ee91e0
Assignee: nobody → amarchesini
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/e5b2e2ee91e0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
I'm seeing these same failures:

(In reply to Boris Zbarsky [:bz] from comment #48)
> Then I backed out the non-parser part because it caused M1 test failures:
> 
> 11:20:36 INFO - 137067 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug870787.html | Form3.img4 doesn't
> exists - got undefined, expected [object HTMLImageElement]
> 11:20:36 INFO - 137068 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug870787.html | Form3.img4id doesn't
> exists - got undefined, expected [object HTMLImageElement]
> 11:20:36 INFO - 137069 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug870787.html | Form3.img5 doesn't
> exists - got undefined, expected [object HTMLImageElement]
> 11:20:36 INFO - 137070 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug870787.html | Form3.img5id doesn't
> exists - got undefined, expected [object HTMLImageElement]

When running mochitest-plain on a win64 debug build locally.  Was whatever was causing them known/fixed?
.. that's because my tree seems to be from june. !@#$.
Looks like I forgot to land the Java patch for this bug wchen re-fixed the Java bits. Mentioning this for the benefit of those who do hg archeology.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.