Handle classes within Node #92

Kiyoshika · 2023-10-03T21:20:04Z

This PR introduces utility methods to work with class names within a node according to this issue and this discussion. This will be useful for user agents / renderers to check which/if classes need to be applied to a given element and will also be used in a future PR for querying the DOM by class name.

Note that I am not doing any "error handling" in ElementClass since this mimics the behaviour when using javascript.

For instance, if you try

var element = document.getElementById("myDIV");
element.classList.toggle("mystyle");

on a class that doesn't exist, it will essentially do nothing (at least, from the end user's perspective.) I'm not sure if it's worth it to throw an actual error if trying to remove/edit a class that doesn't exist, so for now they are silent (but intentional) errors.

From the parser, when a new node is created & added to the document, we check if a class attribute exists and take its string which is split on whitespace (extra whitespaces are ignored) and an ElementClass struct is created to hold the classes which is stored inside the Node struct. We can then toggle/add/remove classes from the node using the interface provided.

EDIT: this also simplifies the Node::get_attribute and Node::get_mut_attribute methods to return an Option instead of a Result<Option>. Even though this is an unrelated change, I don't think it was worth creating an entirely separate PR for this and snuck it into this one

Kiyoshika · 2023-10-03T21:27:51Z

I just realised I pulled off of my outdated main (hence the conflict), going to rebase before continuing

Kiyoshika · 2023-10-04T02:15:49Z

re-synced fork, added some starter tests but didn't have much time today to work on it, hopefully can make more progress tomorrow

didn't have time to really work on this today, just some small fixes

TODO: integrate new struct into Node struct

Got classes added when node is created from parser, but need to do some cleanup such as removing the unwraps() and adding some tests in the node itself. But ran out of time and need to head out to dinner with some people

We should now have a usable API for handling classes (at least, the bare minimum) for element nodes

Kiyoshika · 2023-10-05T03:28:40Z

PR finished and ready to review

blaumeise20 · 2023-10-05T05:36:27Z

src/html5_parser/element_class.rs

+
+    /// Count the number of classes (active or inactive)
+    /// assigned to an element
+    pub fn count(&self) -> usize {


Wouldn't it be better to stick with the Rust style here and use len as the method name? Especially because there is an is_empty method, which gets suggested by clippy when doing any comparison to zero, e.g. list.len() == 0.

yes, good suggestion. i'm still not entirely familiar with all of Rust's conventions yet. Will fix now

updated in 98e6130

emwalker · 2023-10-05T13:03:35Z

src/html5_parser/node.rs

        }

        let mut value: Option<&String> = None;
        if let NodeData::Element { attributes, .. } = &self.data {
            value = attributes.get(name);
        }

-        Ok(value)
+        value


We'll have to keep our eye on whether there are error cases in CSS that need to be handled. But this change feels like a good next step. I'm thinking we can backfill error handling when we discover that it's needed.

emwalker · 2023-10-05T13:07:08Z

src/html5_parser/element_class.rs

+        // however, map.insert will update a key if it exists
+        // and we don't want to overwrite an inactive class to make it active unintentionally
+        // so we ignore this operation if the class already exists
+        if !self.contains(name) {


~~Probably want to keep an eye on this kind of substring matching. Might be fine indefinitely, or might cause problems later on.~~

I see we're using a HashMap.

emwalker · 2023-10-05T13:09:46Z

src/html5_parser/parser/mod.rs

+        let mut parser = Html5Parser::new(&mut stream);
+        let (doc, _) = parser.parse();
+
+        assert_eq!(doc.get_root().classes.is_none(), true);


This and elsewhere would be

assert!(doc.get_root().classes.is_none());

I wonder whether you would get this automatically by running cargo clippy --fix.

I'll add the suggestions this and others, although cargo clippy --fix doesn't complain about anything for me (cargo 1.72.1, MacOS 13.5)

Although going forward for boolean tests I will just use assert!

Oh, now I understand how it works, it just made the changes for me

emwalker · 2023-10-05T13:10:13Z

src/html5_parser/parser/mod.rs

+
+            assert_eq!(classes.is_active("one"), true);
+            assert_eq!(classes.is_active("two"), true);
+            assert_eq!(classes.is_active("three"), true);


See comment above.

emwalker

LGTM with suggestions

this also adjusted others outside of my code changes but it's probably better to stay consistent

Kiyoshika · 2023-10-05T13:59:46Z

@emwalker I ran clippy fix which also adjusted some assertions outside my original changes but thought it's probably best to stay consistent. I'll get your final sign off before merging

emwalker · 2023-10-06T00:04:56Z

@Kiyoshika I'm thinking you have the ability to merge when you're ready. Let me know if you don't.

emwalker · 2023-10-06T00:21:21Z

Looks like our PRs overlapped, so there's a conflict you'll have to fix.

Kiyoshika closed this Oct 4, 2023

Kiyoshika force-pushed the feat/handle-node-classes branch from fc1c0cc to 3e20a39 Compare October 4, 2023 02:13

Kiyoshika reopened this Oct 4, 2023

Kiyoshika added 4 commits October 4, 2023 23:21

fixed some formatting, add starter tests

a4f0f4f

didn't have time to really work on this today, just some small fixes

finish tests in ElementClass

d9e0866

TODO: integrate new struct into Node struct

finish ElementClass API and tests

888dc2a

We should now have a usable API for handling classes (at least, the bare minimum) for element nodes

Kiyoshika force-pushed the feat/handle-node-classes branch from 07cad7e to 888dc2a Compare October 5, 2023 03:21

Kiyoshika marked this pull request as ready for review October 5, 2023 03:28

Kiyoshika requested review from jaytaph and emwalker October 5, 2023 03:28

blaumeise20 reviewed Oct 5, 2023

View reviewed changes

rename count() to len() in ElementClass to be more 'rusty'

98e6130

jaytaph approved these changes Oct 5, 2023

View reviewed changes

emwalker reviewed Oct 5, 2023

View reviewed changes

emwalker approved these changes Oct 5, 2023

View reviewed changes

simplify boolean assertions with clippy

92ef529

this also adjusted others outside of my code changes but it's probably better to stay consistent

Kiyoshika requested a review from emwalker October 5, 2023 14:00

emwalker approved these changes Oct 5, 2023

View reviewed changes

Merge branch 'main' into feat/handle-node-classes

6cec23c

Kiyoshika merged commit 5781a42 into gosub-io:main Oct 6, 2023
4 checks passed

Kiyoshika deleted the feat/handle-node-classes branch October 6, 2023 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle classes within Node #92

Handle classes within Node #92

Kiyoshika commented Oct 3, 2023 •

edited

Loading

Kiyoshika commented Oct 3, 2023

Kiyoshika commented Oct 4, 2023

Kiyoshika commented Oct 5, 2023

blaumeise20 Oct 5, 2023

Kiyoshika Oct 5, 2023

Kiyoshika Oct 5, 2023

emwalker Oct 5, 2023

emwalker Oct 5, 2023 •

edited

Loading

emwalker Oct 5, 2023 •

edited

Loading

Kiyoshika Oct 5, 2023

Kiyoshika Oct 5, 2023

Kiyoshika Oct 5, 2023

emwalker Oct 5, 2023

emwalker left a comment

Kiyoshika commented Oct 5, 2023

emwalker commented Oct 6, 2023

emwalker commented Oct 6, 2023

Handle classes within Node #92

Handle classes within Node #92

Conversation

Kiyoshika commented Oct 3, 2023 • edited Loading

Kiyoshika commented Oct 3, 2023

Kiyoshika commented Oct 4, 2023

Kiyoshika commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

emwalker Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emwalker left a comment

Choose a reason for hiding this comment

Kiyoshika commented Oct 5, 2023

emwalker commented Oct 6, 2023

emwalker commented Oct 6, 2023

Kiyoshika commented Oct 3, 2023 •

edited

Loading

emwalker Oct 5, 2023 •

edited

Loading

emwalker Oct 5, 2023 •

edited

Loading