870787 - Improve named getter for form

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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>.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

See:

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2245
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2246
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=2247

Comment 2

[Andrea Marchesini [:baku]]

Attachment: mochitest

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

What about this?

Comment 4

[Boris Zbarsky [: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?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

No, it works. But I added it in the mochitest to be sure.

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: WIP

Comment 9

[Boris Zbarsky [: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.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch

This works but let me know if you like this patch.

Comment 12

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-09-21)]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Apparently in IE, form3.img4 is undefined

In which exact IE, and in which mode?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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>.

Comment 15

[Boris Zbarsky [:bzbarsky]]

We can try to not make <img> form-associated in the table case, but I worry about site compat...

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Andrea Marchesini [:baku]]

> 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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> The separate hashtable is because images are used as fallback in the indexing.

Ah, I see.  That's worth documenting!

Comment 19

[Boris Zbarsky [:bzbarsky]]

In which case, let me read through this more carefully...

Comment 20

[Andrea Marchesini [:baku]]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yes, I saw that in the spec.  I'm saying it's worth documenting in our code why we have the separate hashtable.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 23

[Boris Zbarsky [: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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, and please make sure there's an interdiff?  I'd really love to not have to reread all of that!

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 755628 [details] [diff] [review]
patch

Henri, can you please review the parser changes at the end?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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;

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 28

[Andrea Marchesini [:baku]]

> >-      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>’

Comment 29

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 30

[Andrea Marchesini [:baku]]

Attachment: patch

Sorry for spamming.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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 32

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-09-21)]

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);
>+      }

Comment 33

[Andrea Marchesini [:baku]]

> 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);

Comment 34

[Andrea Marchesini [:baku]]

> 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.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 36

[Boris Zbarsky [: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.

Comment 37

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-09-21)]

Attachment: Make the portable part of the parser treat img as a form-associated element

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.

Comment 38

[Andrea Marchesini [:baku]]

Attachment: patch

Do you need an interdiff? I removed the HTML5 parser changes.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Do you need an interdiff?

I'd like one for addressing comment 36, yes.

Comment 41

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

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.

Comment 42

[Andrea Marchesini [:baku]]

Attachment: interdiff

Here interdiff for comment 36

Comment 43

[Boris Zbarsky [: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!

Comment 44

[Andrea Marchesini [:baku]]

Attachment: form patch

Comment 45

[Andrea Marchesini [:baku]]

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.

Comment 46

[Andrea Marchesini [:baku]]

2 patches to land.

Comment 47

[Andrea Marchesini [:baku]]

Attachment: patch

rebased

Comment 48

[Boris Zbarsky [:bzbarsky]]

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

Comment 49

[Andrea Marchesini [:baku]]

Attachment: patch

Can you just review the HTML5 parser part?

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd68811a4d63

Comment 51

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-09-21)]

Comment on attachment 763768 [details] [diff] [review]
patch

r+ on the nsHtml5TreeOperation part.

Comment 52

[Andrea Marchesini [:baku]]

Just the patch 'patch' has to be landed.
Thanks Henri for the review!

Comment 53

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b2e2ee91e0

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5b2e2ee91e0

Comment 55

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 56

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

.. that's because my tree seems to be from june. !@#$.

Comment 57

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-09-21)]

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.