Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebDriver: Partial implementation of Element Clear endpoint #1356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hanpham32
Copy link
Contributor

@hanpham32 hanpham32 commented Sep 10, 2024

This PR addresses issue #1040 :~)

Overview:

  • Implemented sections 1, 2, and 3 of the Element Clear endpoint.
  • Reordered Web::WebDriver::Error entries alphabetically.

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@hanpham32 hanpham32 marked this pull request as draft September 10, 2024 06:43
@hanpham32 hanpham32 changed the title WebDriver: Partial Implementation of Element Clear Endpoint WebDriver: Partial Implementation of Element Clear Endpoint (#1040) Sep 10, 2024
@hanpham32 hanpham32 changed the title WebDriver: Partial Implementation of Element Clear Endpoint (#1040) WebDriver: Partial Implementation of Element Clear Endpoint Sep 10, 2024
@hanpham32 hanpham32 marked this pull request as ready for review September 10, 2024 07:15
@hanpham32 hanpham32 changed the title WebDriver: Partial Implementation of Element Clear Endpoint WebDriver: Partial implementation of Clement Clear endpoint Sep 10, 2024
@hanpham32 hanpham32 changed the title WebDriver: Partial implementation of Clement Clear endpoint WebDriver: Partial implementation of Element Clear endpoint Sep 10, 2024
Comment on lines 1523 to 1526
Web::DOM::Element* element = TRY(get_known_connected_element(element_id));
if (!element) {
return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchElement, "Element not found");
}
Copy link
Contributor

@jdahlin jdahlin Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a reviewer, but I would suggest that you either implement the whole line or keep FIXME.

The code below is untested, but something like it is what the spec expects.

Suggested change
Web::DOM::Element* element = TRY(get_known_connected_element(element_id));
if (!element) {
return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchElement, "Element not found");
}
Web::DOM::Element* element = TRY(get_known_connected_element(element_id));
if (!element) {
return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchElement, "Element not found");
}
auto inner_html = TRY(element->inner_html());
if (inner_html.is_empty()) {
return ""sv
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the element for null. Take a look at the implementation of get_known_connected_element - it only returns non-null pointers, and already returns a NoSuchElement if the element was null.

(It returns a pointer instead of a reference because ErrorOr<> cannot hold references. We could update it to return NonnullGCPtr instead, but that's a separate issue.)

So, this is just:

auto* element = TRY(get_known_connected_element(element_id));

// 1. If element's innerHTML IDL attribute is an empty string do nothing and return.
if (TRY(element->inner_html()).is_empty())
    return JsonValue {};

(Not sure TRY will work exactly here, since inner_html return a DOM exception, but that's the gist of it).

Comment on lines 1523 to 1526
Web::DOM::Element* element = TRY(get_known_connected_element(element_id));
if (!element) {
return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchElement, "Element not found");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the element for null. Take a look at the implementation of get_known_connected_element - it only returns non-null pointers, and already returns a NoSuchElement if the element was null.

(It returns a pointer instead of a reference because ErrorOr<> cannot hold references. We could update it to return NonnullGCPtr instead, but that's a separate issue.)

So, this is just:

auto* element = TRY(get_known_connected_element(element_id));

// 1. If element's innerHTML IDL attribute is an empty string do nothing and return.
if (TRY(element->inner_html()).is_empty())
    return JsonValue {};

(Not sure TRY will work exactly here, since inner_html return a DOM exception, but that's the gist of it).

{
dbgln("FIXME: WebDriverConnection::element_clear()");

// FIXME: 1. If element's innerHTML IDL attribute is an empty string do nothing and return.
// 1. If element's innerHTML IDL attribute is an empty string do nothing and return.
Web::DOM::Element* element = TRY(get_known_connected_element(element_id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really how this should be implemented. The steps starting here are "to clear a content editable element" and seem like they're meant to be implemented as a subroutine, which we can do with a lambda. So something like:

auto clear_content_editable_element = [&](auto& element) {
    // 1. If element's innerHTML IDL attribute is an empty string do nothing and return.
    if (TRY(element->inner_html()).is_empty())
        return JsonValue {};

    // and so on...
};

Then, the implementation of element_clear really begins at "the remote end" steps, invoking this subroutine as appropriate in step 11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for the code review. It makes more sense to make clear_content_editable_element as a lambda function :~) I've incorporated your reviews into the new push

- Implemented part 1, 2, and 3 of the Element Clear endpoint.
- Reordered Web::WebDriver::Error entries alphabetically.
@hanpham32 hanpham32 marked this pull request as ready for review September 12, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants