Add a notification for elements that a child text node changed

[jdm]

The VirtualMethods trait contains a children_changed method, but that also triggers when a non-Text node is appended to an element. For cases like <title> where we only care about the text nodes, the extra notifications are wasteful. All of the code that calls children_changed exists in the Node manipulation code in node.rs; we'll want to consider in each case whether to call the new method as well.

Code: components/script/dom/virtualmethods.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/htmltitleelement.rs, components/script/dom/node.rs

[jdm]

@mattnenterprise If you're looking for something else to work on, this might be interesting!

[mattnenterprise]

I would like to work on this!

[jdm]

@mattnenterprise Did you make any progress? Are you still working on this?

[zimio]

I would like to pick this one up. Who is the mentor?

What should be the approach for this? If I understood correctly, we don't want to call children_changed in specific cases?

[jdm]

@zimio We will continue to always call children_changed, but certain pieces of code can move the code that runs in response to children_changed into the new handler which is called less often.

[jdhorwitz]

Is anyone still working on this? If not I would love to work on this with a Mentor. Thanks!

[zimio]

I'm looking into this, although it has taken me a while. Do we have a set deadline for this?

[KiChjang]

@zimio No, we don't have a deadline for any easy issues, although it's generally the practice to claim an issue that one would work on in the immediate future.
@jdhorwitz We can look for another issue for you instead, it looks like @zimio is still working on this one.

[jdhorwitz]

Thanks @KiChjang I'm up for anything.

[zimio]

I still not understanding this completely. Who is mentoring this? Matthews?

My first question is, where are the unit tests for node.rs?

I already tried servo/tests/unit, but not luck there. (would seem like the most likely place)

Second question is regarding the problem here.

I'm thinking maybe that children_changed method calls too many handlers for the simple case of adding text to a node. So there should be a new method child_text_changed which only cares about text changes for elements like a title or a h1 and don't call too many handlers for that. Am I being correct here?

Matthews, said that children_changed will always be called, so maybe instead of child_text_changed, I could just add some conditional to check whether or not this is text change only?

So if I were to solve this, the game plan should be:

1-) Have a list handlers that I don't want to be called when changing a title element

2-) Write a test to check if they are being called or not

I'm having a hard time figuring out how it is the current implementation wrong and what would be the desired state.

[jdm]

No, we do not have the ability to write unit tests for most code in component/script. Instead we write integration tests that are actual webpages. In this case, we would want to have a page that checks the value of document.title after updating the contents of the <title> element, since that would imply that our new notification has been called. I believe this is already covered by the tests in tests/wpt/web-platform-tests/html/semantics/document-metadata/the-title-element/, which can be run using ./mach test-wpt tests/wpt/web-platform-tests/html/semantics/document-metadata/the-title-element/.

The problem here is not really that too many handlers are being called. Instead, if the handlers only care about text nodes being changed then they each need to duplicate that logic. If the code that invokes children_changed contains that logic instead, it can choose to invoke child_text_changed in addition, and handler code that only cares about text nodes can migrate to child_text_changed from children_changed. Does that make sense?

[jdm]

A straightforward way to start would be to focus on the implementation of children_changed for HTMLTitleElement, and add logic that only invokes document.title_changed() if the observed mutation only involves text nodes. Once that's working (ie. the tests still pass), it will be easier to focus on extracting that logic and moving it into the callers of children_changed. Does that make sense?

[jdm]

Are you still working on this @zimio?

[zimio]

@jdm Yes. Though it's been a bit challenging I still need to wrap my head around the flow.

For instance, when I comment out code in the html element file the test doesn't fail.

I need to keep looking at this to get a deeper understanding of the flow.

@@ -60,7 +60,7 @@ impl VirtualMethods for HTMLTitleElement {
         }
         let node = self.upcast::<Node>();
         if node.is_in_doc() {
-            node.owner_doc().title_changed();
+            //node.owner_doc().title_changed();
         }
     }
 
@@ -70,7 +70,7 @@ impl VirtualMethods for HTMLTitleElement {
         }
         let node = self.upcast::<Node>();
         if tree_in_doc {
-            node.owner_doc().title_changed();
+            //node.owner_doc().title_changed();
         }
     }
 }

[jdm]

@zimio Oh, good catch. It turns out that Document::Title always checks the actual title contents; title_changed is only used to update the title that appears in the OS window. Looks like automated tests won't work for this, and we'll need to do manual inspection of the window title instead.

[jdm]

Actually, it's better than I feared - we also dispatch a DOM event for the mozbrowser embedding scenario (ie. a web browser that's build around privileged iframes). We have tests that use this event in tests/wpt/mozilla/tests/mozilla/mozbrowser/mozbrowsertitlechangedeagerly_event.html, so we can build other tests that verify this case.

[zimio]

I will check that test, see if I can adapt it for this case.

[zimio]

I'm having problems adapting this. As far as I can tell this test works for iframes, it doesn't work for the titleElement because mozbrowsertitlechange only works on iframes. I tried using a MutationObserver, but I'm not able to. Is it implemented in servo?

Any tips on how to capture the event when the title changes the text?

[jdm]

We don't support MutationObserver yet, no. I'm not sure I understand the problem - what's wrong with an iframe containing a title element that gets modified, and the test watches for the appropriate mozbrowsertitlechange events?

[zimio]

Yea, got confused I was trying to do it without the iframe.

[zimio]

Is there a way to test if a Node only contains text?

[jdm]

The Element.childElementCount API will return 0 if the only children of an element are text nodes.

[zimio]

Hello, so I have a function that I think can be called to check if children_changed should be called.

Where should I put it? Rust complains that it is out of scope if I put it inside the trait implementation and I don't want to spam "self" everywhere.

Also I couldn't find the enum for HTMLElementTypeId. I want to include elements like 'b', 's', 'h1', etc. to this check. How do I do that?

        fn accepts_only_text_nodes(node: &Node, parent: &Node) -> bool {
            let isTextNode = node.children_count() == 0;
            match parent.type_id() {
                //NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::)) |
                NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::HTMLTitleElement)) => isTextNode,
                _ => false
            }
        }

Thanks

[jdm]

The enum is generated by the build, so refer to docs.servo.org. The free function could live anywhere in the same file.

[zimio]

So it seems everything is good with the function that checks if the node only accepts text (except for some minor issues).

I got like this: if the node only accept text don't call children_changed.

Then the test started failing, that's because document.title_changed(); needs to be called to update the document's title.

I think this is the part where you wanted me to extract the logic of it. But I don't see how I can make a generalized solution for this, calling document.title_changed() if accepts_only_text_nodes returns true feels wrong because the parent node might be of a different type than a title element.

Perhaps a new method for HTMLElement (so that it could be inherited) that updates the DOM when called? Does that exist?

[jdm]

Calling document.title_changed() is the responsibility of HTMLTitleElement's handler, not the code that invokes the handler. I believe you're looking for the code that invokes VirtualMethods::children_changed; that's scattered in various places in components/script/dom/node.rs, so it would make sense to make a new method there that encapsulates the concept of "invoke the appropriate handlers depending on whether only text nodes are involved" and use that instead.

[zimio]

HI @jdm, so are we closing this issue or are we devising another strategy to do this refactor?

[jdm]

I've got no other ideas. Thanks for the reminder.