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

LibWeb: Implemented basic access_key_label function. #24050

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

Conversation

bokendell
Copy link

This contribution is mainly due to a group project from the University of Utah CS4560 class.

The access_key_label function implementation was started and tested to provide the desired output. There are comments and debug lines included in the method, as we were not sure what the system modifier key was for serenity or how to get it per each OS. The same follows for determining a valid system key. The method defaults to providing "Alt+{access_key}" as the access_key_label and an empty string if not access_key is found.

Documentation followed: https://html.spec.whatwg.org/multipage/interaction.html#dom-accesskeylabel

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 21, 2024
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS 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.

Still missing valid access key / modifier.
@@ -10,7 +10,7 @@
#include <LibCore/MappedFile.h>
#include <LibMain/Main.h>
#include <LibVideo/PlaybackManager.h>
#include <SDL2/SDL.h>
#include <SDL.h>
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change.

Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

I'm not sure how much time y'all are trying to put into this, but if I was implementing this, I would do it the following way:

  • create a member variable Optional<String> m_assigned_access_key; (or even better, Web::KeyEvent or a similar type from LibWeb/Page/InputEvent.h. Though Optional<String> would be fine for now, I suspect @trflynn89 will have some ideas on how to flesh this out after this initial implementation :).

  • create a member function update_assigned_access_key(). This member function will have the algorithm described in https://html.spec.whatwg.org/multipage/interaction.html#assigned-access-key.

  • change this function, access_key_label to instead "stringify" the Access key label. In the case of the member being an Optional, that's just return m_assigned_access_key.value_or(String{});

  • add conditional statements (an else if) to HTMLElement::attribute_changed() to handle when the "accesskey"sv attribute is changed, and call the update_assigned_access_key() method.

@@ -566,4 +566,20 @@ void HTMLElement::did_receive_focus()
browsing_context->set_cursor_position(DOM::Position::create(realm(), *this, 0));
}

String HTMLElement::access_key_label() const
Copy link
Member

Choose a reason for hiding this comment

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

Please reference the applicable spec section in a comment above the function here, as is the style in LibWeb. In this case,
https://html.spec.whatwg.org/multipage/interaction.html#dom-accesskeylabel

Comment on lines +571 to +573
auto access_key = get_attribute(HTML::AttributeNames::accesskey);
if (!access_key.has_value() || access_key->is_empty())
return {};
Copy link
Member

Choose a reason for hiding this comment

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

Add the spec steps from the algorithm in the spec above the functions as comments.

For the start of the method, something like this:

Suggested change
auto access_key = get_attribute(HTML::AttributeNames::accesskey);
if (!access_key.has_value() || access_key->is_empty())
return {};
// Whenever an element's accesskey attribute is set, changed, or removed, the user agent must update the element's assigned access key by running the following steps:
// FIXME: We need to update the assigned access key on attribute change, set and remove.
// 1. If the element has no accesskey attribute, then skip to the fallback step below.
auto access_key = get_attribute(HTML::AttributeNames::accesskey);
if (!access_key.has_value() || access_key->is_empty()) {
// 4. Fallback: Optionally, the user agent may assign a key combination of its choosing as the element's assigned access key and then return.
return {};
}

Comment on lines +576 to +579
dbgln("FIXME: Implement HTMLElement::access_key_label() valid system key check");
// Loop through words in access_key to find first valid system key
dbgln("FIXME: Implement HTMLElement::access_key_label() get system modifier key");
// get system modifier key and append that instead of this line
Copy link
Member

Choose a reason for hiding this comment

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

For steps that you don't want to implement in this change set, it's preferred to do something more like this:

// FIXME: N. Some verbatim text from the spec
// FIXME: N+1. Some other verbatim text from the spec

If the step that you're marking as FIXME has substeps, you can skip them if the parent step is FIXME'd.

Copy link

stale bot commented May 22, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member stale student project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants