Bug 196636

Summary:

AX: Support API: accessibilityReplaceRange:withText

Product:

WebKit

Reporter:

chris fleizach <cfleizach>

Component:

Accessibility

Assignee:

chris fleizach <cfleizach>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, commit-queue, dbates, dmazzoni, ews-watchlist, Hironori.Fujii, jcraig, jdiggs, samuel_white, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	none
patch	none
patch	none
patch	dbates: review+
patch	none
patch for landing	none
patch for landing	none

Description chris fleizach 2019-04-04 17:07:02 PDT

Support accessibility platform API of

accessibilityReplaceRange:withText

<rdar://problem/49438073>

Comment 1 chris fleizach 2019-04-04 17:09:23 PDT

Created attachment 366776 [details]
patch

Comment 2 chris fleizach 2019-04-04 17:10:47 PDT

Created attachment 366777 [details]
patch

Comment 3 EWS Watchlist 2019-04-04 17:12:59 PDT

Attachment 366777 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/AccessibilityObject.h:297:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 4 chris fleizach 2019-04-04 17:14:31 PDT

Created attachment 366778 [details]
patch

Comment 5 chris fleizach 2019-04-05 13:00:29 PDT

Created attachment 366835 [details]
patch

Comment 6 Daniel Bates 2019-04-08 12:55:28 PDT

Comment on attachment 366835 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2287
> +    if (auto frame = renderer()->document().frame()) {

auto* please

I could have sworn RnederObject has a frame() convenience method instead of deref document.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2296
> +        downcast<HTMLInputElement>(element).setRangeText(replacementString, range.start, range.length, "");

Bad cast if element is HTMLTextAreaElement.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:811
> +    JSStringRef text = 0;

nullptr? Don't we have some smart pointer like class we can use to manage this value. Pretty sure we have one. Can you take a look at other code that make use of JSStringRef?

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:820
> +    JSValueRef result = JSValueMakeBoolean(context, toAXElement(thisObject)->replaceTextInRange(text, position, length));

Assuming this handles 0 for position/length.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:822
> +    if (text)
> +        JSStringRelease(text);

Ok as-is. Smart pointer would be preferred. If we don't have one then does JSStringRelease() null check for us?

> LayoutTests/accessibility/mac/replace-text-with-range-expected.txt:2
> +HelloBlorg
> + 

Ok as-is. In my opinion this makes the test look a little messy. If I was writing this test I would figure out a way to omit this.

> LayoutTests/accessibility/mac/replace-text-with-range.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Old Doctype. Intentional? If not, use new hotness: <!DOCTYPE html> 😀

> LayoutTests/accessibility/mac/replace-text-with-range.html:4
> +<script src="../../resources/js-test-pre.js"></script>

New hotness is to just include js-test.js and omit -pre and -post.js files.

Comment 7 chris fleizach 2019-04-08 16:18:39 PDT

Created attachment 366993 [details]
patch

all comments addressed. thanks

Comment 8 chris fleizach 2019-04-08 16:19:48 PDT

Created attachment 366994 [details]
patch for landing

Comment 9 chris fleizach 2019-04-08 16:22:28 PDT

Created attachment 366995 [details]
patch for landing

Comment 10 WebKit Commit Bot 2019-04-08 17:39:56 PDT

Comment on attachment 366995 [details]
patch for landing

Clearing flags on attachment: 366995

Committed r244059: <https://trac.webkit.org/changeset/244059>

Comment 11 WebKit Commit Bot 2019-04-08 17:39:58 PDT

All reviewed patches have been landed.  Closing bug.

Comment 12 Fujii Hironori 2019-04-08 18:26:58 PDT

Committed r244062: <https://trac.webkit.org/changeset/244062>

Comment 13 Fujii Hironori 2019-04-08 18:54:59 PDT

Committed r244066: <https://trac.webkit.org/changeset/244066>