Bug 248717

<rdar://problem/102938762>

Created attachment 463871 [details]
Patch

Created attachment 463872 [details]
Patch

Comment on attachment 463872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463872&action=review

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:8
> +<div id="content">

I think you can either remove this `id`, or use it to hide the test text before `finishJSTest()` like so:

document.getElementById("content").style.visibility = "hidden";

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:29
> +            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);

Does this work instead of checking the length?

await waitFor(() => password1.stringValue === "AXValue: hello");

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:34
> +            document.getElementById("button-2").setAttribute("aria-labelledBy", "text-1");

I think aria-labelledBy is typically all lowercased:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:42
> +            output += expect("button2.description", "'AXDescription: This is before my password field This is after my password field'");

Could maybe make the test smaller while still providing the same diagnostics with:

"This is before my password field" -> "Before password"
"This is after my password field" -> "After password"

Created attachment 463873 [details]
Patch

(In reply to Tyler Wilcock from comment #4)
> Does this work instead of checking the length?
> 
> await waitFor(() => password1.stringValue === "AXValue: hello");
Unfortunately we can't do this here because we don't return the actual password value here only the text representation being rendered (sometimes ****). Length check seems to be what the other password tests use.

Created attachment 463874 [details]
Patch

(In reply to Tommy McHugh from comment #7)
> Created attachment 463874 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        if (input->isPasswordField())
+            return String();

This should return a string with the same length as input->value() consisting of a repeated mask char, e.g., if input->value() is "blah", the return value here should "****".

--- /dev/null
+++ b/LayoutTests/accessibility/aria-labelledby-on-password-input.html

+        setTimeout(async function() {
+            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);
+            output += expect("button1.description", "'AXDescription: '");

+        setTimeout(async function() {
+            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);
+            output += expect("button1.description", "'AXDescription: '");

Should be:

+            output += expect("button1.description", "'AXDescription: *****'");

The same applies to the other instances. In general the value of the password field should not be exposed as an empty string but instead as a mask string, so that the VO user can arrow through it, know how many chars are in the filed, etc., i.e., to have the same experience as the sighted user.

Created attachment 463911 [details]
Patch

(In reply to Andres Gonzalez from comment #8)
> The same applies to the other instances. In general the value of the
> password field should not be exposed as an empty string but instead as a
> mask string, so that the VO user can arrow through it, know how many chars
> are in the filed, etc., i.e., to have the same experience as the sighted
> user.
Sounds good! Made this the bullet character mask to match what iOS wrapper uses. AXValue already returns masked characters, it's a VoiceOver specific issue why we don't announce those.

(In reply to Tommy McHugh from comment #9)
> Created attachment 463911 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

Can we do a char replacement in place, something like:

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        String inputValue = input->value();
+        if (input->isPasswordField()) {
+            for (size_t i = 0; i < inputValue.length(); i++)
+                inputValue[i] = '•';
+        }
+        return inputValue;
+    }

(In reply to Andres Gonzalez from comment #11)
> (In reply to Tommy McHugh from comment #9)
> > Created attachment 463911 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> 
> Can we do a char replacement in place, something like:
This doesn't work, WTFString can't assign here for characters. Took a look at header for WTFString and didn't find another method either that would do the equivalent.

(In reply to Tommy McHugh from comment #12)
> (In reply to Andres Gonzalez from comment #11)
> > (In reply to Tommy McHugh from comment #9)
> > > Created attachment 463911 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> > +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> > 
> > Can we do a char replacement in place, something like:
> This doesn't work, WTFString can't assign here for characters. Took a look
> at header for WTFString and didn't find another method either that would do
> the equivalent.

this may be more efficient than multiple reallocations and concatenations:

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        String inputValue = input->value();
+        if (input->isPasswordField()) {
+            StringBuilder passwordValue;
+            passwordValue.reserveCapacity(inputValue.length());
+            for (size_t i = 0; i < inputValue.length(); i++)
+                passwordValue.append(''•');
+            return passwordValue.toString();
+        }
+        return inputValue;
+    }

Created attachment 465685 [details]
Patch

rdar://102815043

Committed 262433@main (3b6d017ba868): <https://commits.webkit.org/262433@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465685 [details].