feat: implement ARIA string reflection on Element #32080

nolanlawson · 2024-04-15T03:20:46Z

Partially addresses #32079. Implements ARIA string reflection on Element (e.g. ariaLabel), but not ARIA element reflection (ariaLabelledByElements and friends), and not on ElementInternals.

This PR won't compile until servo/html5ever#536 is merged and our dependency on html5ever is updated, but I thought I'd open the PR anyway for early feedback.

In my local testing, I was able to get all the aria-attribute-reflection.html tests to pass with this change.

Note that this involved adding reflections for ariaRelevant, which is still in the WPT tests but not in the actual spec (see web-platform-tests/wpt#43866). I guess it's up for debate whether Servo should implement this, since Chromium/Gecko/WebKit all do, so arguably it should be implemented for web compat reasons.

This PR is not ideal since the ARIAMixin getters/setters are put directly on Element rather than as a separate mixin/trait, but I figured this could be done in the future when these getters/setters are added to ElementInternals. Edit: fixed!

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes partially address Implement ARIAMixin on Element #32079

There are tests for these changes

nolanlawson · 2024-04-15T03:22:21Z

components/script/dom/element.rs

+                self.remove_attribute(&ns!(), local_name);
+            },
+        }
+    }


set_cross_origin_attribute is really similar to this function, but I wasn't sure if it made sense to combine them, especially since the getter (reflect_cross_origin_attribute) is pretty different.

mrobinson

Thanks for this contribution. Just a few small comments. We'll have to see if we need to release a new version of html5ever or not for this.

mrobinson · 2024-04-16T14:49:48Z

components/script/dom/element.rs

+    // Used for string attribute reflections where absence of the attribute returns null
+    pub fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {


Do you mind turning this comment into a proper rustdoc docstring? Please see https://doc.rust-lang.org/rust-by-example/meta/doc.html for how to do this.

components/script/dom/element.rs

mrobinson · 2024-04-16T14:50:47Z

components/script/dom/element.rs

+        }
+    }
+
+    // Used for string attribute reflections where setting null/undefined removes the attribute


This would also be a good rustdoc comment.

mrobinson · 2024-04-16T14:51:25Z

components/script/dom/element.rs

+    fn GetAriaAtomic(&self) -> Option<DOMString> {
+        self.get_nullable_string_attribute(&local_name!("aria-atomic"))
+    }
+    fn SetAriaAtomic(&self, value: Option<DOMString>) {
+        self.set_nullable_string_attribute(&local_name!("aria-atomic"), value);
+    }
+    fn GetAriaAutoComplete(&self) -> Option<DOMString> {
+        self.get_nullable_string_attribute(&local_name!("aria-autocomplete"))


I think normally there are spaces between functions. Be sure to run ./mach fmt before uploading your change -- though the lint didn't fail so maybe that isn't adding the spaces for some reason.

nolanlawson · 2024-04-17T04:58:57Z

Thanks for the review! Responded to the PR comments, let me know if I missed anything.

mrobinson

Apologies, this review was sitting as a draft for days. I'll go ahead and apply these changes and land this.

mrobinson · 2024-04-17T08:30:16Z

components/script/dom/element.rs

@@ -1718,6 +1718,29 @@ impl Element {
        self.set_attribute(local_name, AttrValue::String(value.into()));
    }

+    /// Used for string attribute reflections where absence of the attribute returns `null`,
+    /// e.g. `element.ariaLabel` returning `null` when the `aria-label` attribute is absent.
+    pub fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {


Oh, sorry I missed this before. These new functions do not need to be public because they are only used in this file.

Suggested change

pub fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {

fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {

No worries, I missed that too. Fixed: f87bf34

mrobinson · 2024-04-17T08:30:26Z

components/script/dom/element.rs

+
+    /// Used for string attribute reflections where setting `null`/`undefined` removes the
+    /// attribute, e.g. `element.ariaLabel = null` removing the `aria-label` attribute.
+    pub fn set_nullable_string_attribute(&self, local_name: &LocalName, value: Option<DOMString>) {


Suggested change

pub fn set_nullable_string_attribute(&self, local_name: &LocalName, value: Option<DOMString>) {

fn set_nullable_string_attribute(&self, local_name: &LocalName, value: Option<DOMString>) {

Fixed: f87bf34

mrobinson · 2024-04-23T14:08:46Z

Oh, it looks like before landing this we'll need to update stylo to the newest version of html5ever. I'll wait until that happens.

nolanlawson · 2024-04-23T14:10:10Z

Apologies, this review was sitting as a draft for days.

No worries! I think we still need to bump the versions of html5ever/markup5ever anyway. I gave it a shot myself, but it looks like it's a bit tricky because they are also transitive dependencies via xml5ever and stylo, and the compilation fails if there are multiple competing versions installed.

nolanlawson · 2024-04-24T15:04:10Z

Since I had a bit of extra time with this PR, I went ahead and implemented the proper ARIAMixin webidl.

I also reordered the properties to match the spec order (which puts role before aria-*).

nolanlawson · 2024-04-24T15:21:11Z

components/script/dom/webidls/Element.webidl

@@ -127,3 +127,4 @@ Element includes ChildNode;
 Element includes NonDocumentTypeChildNode;
 Element includes ParentNode;
 Element includes ActivatableElement;
+Element includes ARIAMixin;


The ARIAMixin spec has this includes statement there, but we have all our Element includes here. I wasn't sure which should take precedence.

Co-authored-by: Martin Robinson <mrobinson@igalia.com>

nolanlawson · 2024-04-27T00:05:30Z

I missed a WPT test that is newly-passing (AriaMixin-string-attributes.html). Should be fixed now.

nolanlawson · 2024-04-27T16:03:20Z

I'm sorry, I totally forgot to update the meta-legacy-layout file as well. 🤦‍♂️

mrobinson · 2024-04-27T16:06:46Z

I'm sorry, I totally forgot to update the meta-legacy-layout file as well. 🤦‍♂️

No problem! Hopefully someday soon we can stop running legacy layout tests on CI entirely.

nolanlawson commented Apr 15, 2024

View reviewed changes

nolanlawson force-pushed the nolan/aria branch from 0e7f2e1 to 6d052b8 Compare April 15, 2024 03:22

nolanlawson changed the title ~~fix: get ariaChecked working~~ feat: implement ARIA string reflection Apr 15, 2024

nolanlawson changed the title ~~feat: implement ARIA string reflection~~ feat: implement ARIA string reflection on Element Apr 15, 2024

mrobinson requested changes Apr 16, 2024

View reviewed changes

mrobinson approved these changes Apr 23, 2024

View reviewed changes

nolanlawson commented Apr 24, 2024

View reviewed changes

nolanlawson and others added 9 commits April 26, 2024 15:07

feat: implement ARIA string reflection

0a90bd8

Update components/script/dom/element.rs

e5aa412

Co-authored-by: Martin Robinson <mrobinson@igalia.com>

fix: respond to PR comments

27e6f69

fix: make functions non-public

fa085b8

fix: use proper ARIAMixin mixin

d971dda

fix: tidy issues

2c13ed5

fix: double newline at end of file

ae60604

fix: move role before aria-* to match spec order

804a7d7

fix: fix link to spec and format as spec does

fae5abb

mrobinson force-pushed the nolan/aria branch from 6525560 to fae5abb Compare April 26, 2024 13:07

mrobinson approved these changes Apr 26, 2024

View reviewed changes

mrobinson added this pull request to the merge queue Apr 26, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 26, 2024

fix: delete now-passing WPT tests

5654a47

mrobinson added this pull request to the merge queue Apr 27, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 27, 2024

fix: remove legacy-layout test

e735baa

mrobinson enabled auto-merge April 27, 2024 16:06

mrobinson added this pull request to the merge queue Apr 27, 2024

Merged via the queue into servo:main with commit 02b3dd0 Apr 27, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement ARIA string reflection on Element #32080

feat: implement ARIA string reflection on Element #32080

nolanlawson commented Apr 15, 2024 •

edited

nolanlawson Apr 15, 2024

mrobinson left a comment

mrobinson Apr 16, 2024

mrobinson Apr 16, 2024

mrobinson Apr 16, 2024

nolanlawson commented Apr 17, 2024

mrobinson left a comment

mrobinson Apr 17, 2024

nolanlawson Apr 23, 2024

mrobinson Apr 17, 2024

nolanlawson Apr 23, 2024

mrobinson commented Apr 23, 2024

nolanlawson commented Apr 23, 2024

nolanlawson commented Apr 24, 2024 •

edited

nolanlawson Apr 24, 2024

nolanlawson commented Apr 27, 2024

nolanlawson commented Apr 27, 2024

mrobinson commented Apr 27, 2024

		// Used for string attribute reflections where absence of the attribute returns null
		pub fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {

	pub fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {
	fn get_nullable_string_attribute(&self, local_name: &LocalName) -> Option<DOMString> {

	pub fn set_nullable_string_attribute(&self, local_name: &LocalName, value: Option<DOMString>) {
	fn set_nullable_string_attribute(&self, local_name: &LocalName, value: Option<DOMString>) {

feat: implement ARIA string reflection on Element #32080

feat: implement ARIA string reflection on Element #32080

Conversation

nolanlawson commented Apr 15, 2024 • edited

Choose a reason for hiding this comment

mrobinson left a 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

nolanlawson commented Apr 17, 2024

mrobinson left a 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

Choose a reason for hiding this comment

mrobinson commented Apr 23, 2024

nolanlawson commented Apr 23, 2024

nolanlawson commented Apr 24, 2024 • edited

Choose a reason for hiding this comment

nolanlawson commented Apr 27, 2024

nolanlawson commented Apr 27, 2024

mrobinson commented Apr 27, 2024

nolanlawson commented Apr 15, 2024 •

edited

nolanlawson commented Apr 24, 2024 •

edited