Support accessibility platform API of accessibilityReplaceRange:withText <rdar://problem/49438073>
Created attachment 366776 [details] patch
Created attachment 366777 [details] patch
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.
Created attachment 366778 [details] patch
Created attachment 366835 [details] patch
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.
Created attachment 366993 [details] patch all comments addressed. thanks
Created attachment 366994 [details] patch for landing
Created attachment 366995 [details] patch for landing
Comment on attachment 366995 [details] patch for landing Clearing flags on attachment: 366995 Committed r244059: <https://trac.webkit.org/changeset/244059>
All reviewed patches have been landed. Closing bug.
Committed r244062: <https://trac.webkit.org/changeset/244062>
Committed r244066: <https://trac.webkit.org/changeset/244066>