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, 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

chris fleizach

Reported 2019-04-04 17:07:02 PDT

Support accessibility platform API of accessibilityReplaceRange:withText <rdar://problem/49438073>

Attachments
patch (16.80 KB, patch) 2019-04-04 17:09 PDT, chris fleizach	no flags	Details Formatted Diff Diff
patch (17.86 KB, patch) 2019-04-04 17:10 PDT, chris fleizach	no flags	Details Formatted Diff Diff
patch (17.86 KB, patch) 2019-04-04 17:14 PDT, chris fleizach	no flags	Details Formatted Diff Diff
patch (17.86 KB, patch) 2019-04-05 13:00 PDT, chris fleizach	dbates: review+	Details Formatted Diff Diff
patch (17.85 KB, patch) 2019-04-08 16:18 PDT, chris fleizach	no flags	Details Formatted Diff Diff
patch for landing (17.85 KB, patch) 2019-04-08 16:19 PDT, chris fleizach	no flags	Details Formatted Diff Diff
patch for landing (17.82 KB, patch) 2019-04-08 16:22 PDT, chris fleizach	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

chris fleizach

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

Created attachment 366776 [details] patch

chris fleizach

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

Created attachment 366777 [details] patch

EWS Watchlist

Comment 3 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.

chris fleizach

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

Created attachment 366778 [details] patch

chris fleizach

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

Created attachment 366835 [details] patch

Daniel Bates

Comment 6 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.

chris fleizach

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

Created attachment 366993 [details] patch all comments addressed. thanks

chris fleizach

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

Created attachment 366994 [details] patch for landing

chris fleizach

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

Created attachment 366995 [details] patch for landing

WebKit Commit Bot

Comment 10 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>

WebKit Commit Bot

Comment 11 2019-04-08 17:39:58 PDT

All reviewed patches have been landed. Closing bug.

Fujii Hironori

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

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

Fujii Hironori

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

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

Note You need to log in before you can comment on or make changes to this bug.