fix getElementsByTagName()

[kevgs]

This is a continue of work started at #11705

Added test, fixed code for better use of Atom.

Fixes #11596.

r? @nox

When invoked with the same argument, and as long as root’s node document’s type has not changed, the same HTMLCollection object may be returned as returned by an earlier call.

This is implemented in document.rs with tag_map field.

---

This change is

[nox]

+S-needs-code-changes +S-awaiting-answer

---

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

components/script/dom/document.rs, line 2069 [r1] (raw file):

    fn GetElementsByTagName(&self, qualified_name: DOMString) -> Root<HTMLCollection> {
        let qualified_name = Atom::from(qualified_name);
        match self.tag_map.borrow_mut().entry(qualified_name.clone()) {

You don't need to clone the name here, instead in the Vacant arm, do:

let result = HTMLCollection::match_elements_by_qualified_name(
    &self.window, self.upcast(), entry.key().clone());

---

components/script/dom/htmlcollection.rs, line 116 [r1] (raw file):

    }

    pub fn match_elements_by_qualified_name(window: &Window, root: &Node, qualified_name: Atom)

Please just rename that to by_qualified_name.

---

components/script/dom/htmlcollection.rs, line 119 [r1] (raw file):

                                            -> Root<HTMLCollection> {
        #[derive(JSTraceable, HeapSizeOf)]
        struct TagNameFilter {

Nit: for consistency, rename that QualifiedNameFilter.

---

components/script/dom/htmlcollection.rs, line 140 [r1] (raw file):

            ascii_lower_qualified_name: ascii_lower_qualified_name,
        };
        HTMLCollection::create(window, root, box filter)

Where do you discriminate steps 2 and 3 from the spec here?

---

components/script/dom/htmlcollection.rs, line 150 [r1] (raw file):

                qualified_name.find(":") == Some((prefix as &str).len()) &&
                qualified_name.ends_with(elem.local_name() as &str),
        }

Why do you look at the prefix at all? To what does that correspond in the spec?

---

Comments from Reviewable

[kevgs]

Why do you look at the prefix at all? To what does that correspond in the spec?

We can't just compare element`s local name with qualified name because sometimes qualified name is prefix + ':' + local name. https://dom.spec.whatwg.org/#concept-element-qualified-name

Where do you discriminate steps 2 and 3 from the spec here?

You`re right. I have a bug here. But reading through spec I found a mismatch.

Otherwise, returns a HTMLCollection of all descendant elements whose qualified name is qualifiedName. (Matches case-insensitively against elements in the HTML namespace within an HTML document.)

https://dom.spec.whatwg.org/#dom-document-getelementsbytagname

Case insensitive compare and lowercasing qualified name before compare is not the same thing. I quess the right thing is lowercasing as written here but I think spec should be fixed.

[nox]

We can't just compare element`s local name with qualified name because sometimes qualified name is prefix + ':' + local name. https://dom.spec.whatwg.org/#concept-element-qualified-name

Got it.

Case insensitive compare and lowercasing qualified name before compare is not the same thing. I quess the right thing is lowercasing as written here but I think spec should be fixed.

The sentence you copied is from a non-normative note and when the DOM spec talks about case-insensitivity it means ASCII case-insensitivity. ASCII case-insensitivity comparison and comparison after ASCII lowercasing are the same things.

[nox]

-S-awaiting-answer -S-awaiting-review +S-needs-code-changes

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

components/script/dom/htmlcollection.rs, line 122 [r2] (raw file):

        if qualified_name == atom!("*") {
            #[derive(JSTraceable, HeapSizeOf)]
            struct QualifiedNameFilter {

If you disambiguate this way, please give each filter a name that makes sense.

For this one it could be:

struct AllFilter;

---

components/script/dom/htmlcollection.rs, line 133 [r2] (raw file):

        // case 2
        if root.is_in_html_doc() {

You can't do that here, because root could change tree after the collection was created, not being in an HTML document anymore.

---

Comments from Reviewable

[kevgs]

You can't do that here, because root could change tree after the collection was created, not being in an HTML document anymore.

I do not understand that at all. How root could change if it passed immutable in by_qualified_name?

[nox]

root is an element, and any node can be removed from their tree or adopted into another document.

[nox]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

components/script/dom/htmlcollection.rs, line 122 [r3] (raw file):

        if qualified_name == atom!("*") {
            #[derive(JSTraceable, HeapSizeOf)]
            struct AllFilter {

Make it a unit struct:

struct AllFilter;

---

Comments from Reviewable

[kevgs]

root is an element, and any node can be removed from their tree or adopted into another document.

Ok. But I still do not understand. I'm assuming that tree won't change while by_qualified_name() is running. Isn't it true?

[nox]

It's wrong. That method returns a HTMLCollection. These collections are live and reflect changes made on the tree.

If a collection is live, then the attributes and methods on that object must operate on the actual underlying data, not a snapshot of the data.

[nox]

@kevgs Need help with this?

[kevgs]

Yes. I have no idea how to replase root.is_in_html_doc().

[nox]

You need to merge the two different kind of collections into a single one, that checks whether root is in an HTML document when filtering nodes.

[bors-servo]

☔ The latest upstream changes (presumably #13210) made this pull request unmergeable. Please resolve the merge conflicts.

[kevgs]

Sorry, but it looks like I have neither time nor skill to accomplish this task. Hope this uncompleted patch will help someone to fix that issue.

[jdm]

Thanks for letting us know!

[mskrzypkows]

components/script/dom/htmlcollection.rs, line 140 at r1 (raw file):

Previously, nox (Anthony Ramine) wrote…

Where do you discriminate steps 2 and 3 from the spec here?

As I understand, discrimination of steps 2 and 3 is done by `if root.is_in_html_doc()` but is wrong as you said above. How to distinguish them when `root` can change? I need some way to decide which step has to be done.

---

Comments from Reviewable

[nox]

@mskrzypkows You need to use that function in the filter itself, and not use it to return different filters.