Support querying numeric HTML properties from `elements::Element::prop()` by johnDeSilencio · Pull Request #289 · jonhoo/fantoccini

johnDeSilencio · 2025-03-02T00:38:37Z

Preface

Hi, Jon! Longtime fan of yours. Your Crust of Rust series was HUGE for me going from a beginner Rust developer to more intermediate (I've got a long way to go before the rank of "master" 😄). Thank you for all the amazing work you do and how you give back to the community!

Problem

I'm building my own personal portfolio website. One of the features of the application is a terminal emulator, letting you run fake commands on the website.

One of the commands is clear, which clears the screen and the history of all previous commands. To write a functional test for this feature, I'd like to be able to tell that my <div id="previous-commands> has no children which I check through the childElementCount property.

However, when I try use fantoccini::elements::Element::prop("childElementCount"), I get the following error:

...webdriver returned non-conforming response: Number(0)

Proposed Solution

It appears that the fantoccini::elements::Element::prop method currently only handles Strings, Bools, and Null values from serde_json and returns an error for everything else. This PR also supports matching serde_json::Number to support getting numeric properties like childElementCount.

Tests

I've also added another assert_eq!() to the element_test function. Let me know if you'd like me to add more test coverage or to find another way to support numeric HTML properties.

Functional Tests

I used this fork in the E2E tests for my website and now my functional tests pass:

…Count

tests/elements.rs

jonhoo

Thank you^2!

Arguably, prop should just return a serde_json::Value instead of coercing everything to a string. I'm honestly not quite sure why I made it be string only 🤔 I think it's because attribute can only return strings, and I guess I incorrectly assumed that the same would apply to prop. But property values can be "any ECMAScript language type". Given we're going to have to cut a breaking change for #285 anyway, let's maybe just go ahead and change prop to return serde_json::Value right now?

… from prop() method

…prop()

…oc example

johnDeSilencio · 2025-03-10T00:10:04Z

Hey, Jon!

I've made the change you suggested so that prop() now returns a serde_json::Value. I also added a doc comment example to the prop() method, but feel free to let me know if you think I should take it out or change it

jonhoo

That looks way better, thank you! One more question/change and then I think we're good!

jonhoo · 2025-03-22T13:57:26Z

src/elements.rs

    pub async fn html(&self, inner: bool) -> Result<String, error::CmdError> {
        let prop = if inner { "innerHTML" } else { "outerHTML" };
-        Ok(self.prop(prop).await?.unwrap())
+        Ok(self.prop(prop).await?.unwrap().to_string())


I wonder if we should be a bit more principled here and instead of .unwrap().to_string() specifically match for Some(Json::String and error anything else?

jonhoo · 2025-03-22T14:02:06Z

Some of the tests in CI seem unhappy 🤔

jonhoo · 2025-03-22T14:13:16Z

Ah, some of the CI is just racy: fixed in #293. Merging in main should fix those.

src/elements.rs

jonhoo · 2025-06-14T10:22:31Z

@johnDeSilencio Gentle nudge on this — it's very close to getting over the line!

…ic-properties

…rns Json::String, returns Err() otherwise

johnDeSilencio · 2025-07-10T05:52:26Z

src/elements.rs

+            // Requesting `innerHTML` or `outerHTML` should normally return a string
+            Some(Json::String(contents)) => Ok(contents),
+            Some(res) => Err(error::CmdError::NotW3C(res)),
+            _ => Err(error::CmdError::NotW3C(Json::Null)),


Not sure if this is the most appropriate error variant for the None case. Open to suggestions

Yeah, I'm fine with this!

johnDeSilencio · 2025-07-10T05:52:37Z

The prodigal PR author returns 😅

I've merged up my branch with the most recent work on main. I've also made the change to the Element::html() method that you suggested. Wasn't sure exactly what variant of error::CmdError to return. Feel free to let me know if you think a different variant would be more appropriate!

As a side note, I want to apologize for taking so long to get back to you and to this PR. My personal life got kinda hectic, and I let this PR fall through the cracks. Totally my fault that this change missed v0.22. Thank you for your patience and the gentle reminder last month 😃

Moving forward, I've gotten my schedule mapped out pretty well in self-hosted CalDav calendars (shout-out to Stalwart), so if I do submit future PRs, the turn-around time should be significantly less than 4 months!

jonhoo

No worries at all — it happens to all of us from time to time!

As for releasing this, I think I'll then bundle this with #275 in a 0.23, but that does mean it may take a little while to get out :)

jonhoo · 2025-07-19T08:12:56Z

src/elements.rs

+            // Requesting `innerHTML` or `outerHTML` should normally return a string
+            Some(Json::String(contents)) => Ok(contents),
+            Some(res) => Err(error::CmdError::NotW3C(res)),
+            _ => Err(error::CmdError::NotW3C(Json::Null)),


Yeah, I'm fine with this!

codecov · 2025-07-19T08:20:35Z

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.52%. Comparing base (a2457c7) to head (5499fb2).

Files with missing lines	Patch %	Lines
src/elements.rs	33.33%	4 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/elements.rs	`58.80% <33.33%> (+1.19%)`	⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

johnDeSilencio · 2025-07-19T14:33:12Z

No worries at all — it happens to all of us from time to time!

As for releasing this, I think I'll then bundle this with #275 in a 0.23, but that does mean it may take a little while to get out :)

Thanks for understanding! And yeah, no rush on the release either :)

johnDeSilencio added 2 commits March 1, 2025 14:26

feat(elements): support querying numeric properties like childElement…

8f91d11

…Count

test(elements): add test case for numeric prop value in the sample page

7063371

johnDeSilencio commented Mar 2, 2025

View reviewed changes

tests/elements.rs Outdated Show resolved Hide resolved

jonhoo requested changes Mar 8, 2025

View reviewed changes

johnDeSilencio added 4 commits March 9, 2025 14:08

refactor(element-prop)!: 🔄 return serde_json::Value instead of String…

3fbf333

… from prop() method

fix(element-html): 🐛 html() method stringifies the return value from …

3e80945

…prop()

test(element-prop): 🧪 failing test cases

f52ed85

docs(element-prop): 📖 correct out of date documentation and add new d…

623685f

…oc example

johnDeSilencio requested a review from jonhoo March 10, 2025 00:10

jonhoo reviewed Mar 22, 2025

View reviewed changes

jonhoo mentioned this pull request Mar 22, 2025

feat(client): implement printing pages as PDF #285

Merged

paolobarbolini reviewed Mar 22, 2025

View reviewed changes

src/elements.rs Show resolved Hide resolved

jonhoo mentioned this pull request Mar 29, 2025

cargo update #294

Merged

johnDeSilencio added 2 commits July 9, 2025 20:44

Merge remote-tracking branch 'origin/main' into feature/support-numer…

c0e34bd

…ic-properties

fix(element-html): 🐛 html() method only returns Ok() when prop() retu…

5499fb2

…rns Json::String, returns Err() otherwise

johnDeSilencio commented Jul 10, 2025

View reviewed changes

johnDeSilencio requested a review from jonhoo July 14, 2025 05:41

jonhoo approved these changes Jul 19, 2025

View reviewed changes

Conversation

johnDeSilencio commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preface

Problem

Proposed Solution

Tests

Functional Tests

Uh oh!

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

johnDeSilencio commented Mar 10, 2025

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

jonhoo Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

jonhoo commented Mar 22, 2025

Uh oh!

jonhoo commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonhoo commented Jun 14, 2025

Uh oh!

johnDeSilencio Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

jonhoo Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

johnDeSilencio commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

jonhoo Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnDeSilencio commented Jul 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnDeSilencio commented Mar 2, 2025 •

edited

Loading

jonhoo commented Mar 22, 2025 •

edited

Loading

johnDeSilencio commented Jul 10, 2025 •

edited

Loading

codecov bot commented Jul 19, 2025 •

edited

Loading