We need to make TextManipulationController accessibility ready.
Created attachment 392856 [details] Patch
<rdar://problem/58092680>
Comment on attachment 392856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392856&action=review > Source/WebCore/editing/TextManipulationController.cpp:232 > + static const auto attributeNames = makeNeverDestroyed(HashSet<QualifiedName> { > + titleAttr, > + altAttr, > + placeholderAttr, > + aria_labelAttr, > + aria_placeholderAttr, > + aria_roledescriptionAttr, > + aria_valuetextAttr, > + }); > + return attributeNames.get().contains(name); I guess the easiest way to write this code is to use HashSet, and there’s no easy way to write code for other approaches. Seems a tiny bit unfortunate since to compare 1 pointer with 7 other pointers I suspect a linear search might be faster than hashing. > Source/WebCore/editing/TextManipulationController.cpp:239 > + ParagraphContentIterator iterator { start, end }; Unclear to me why it’s safe to make the iterator out of the Position objects, when the VisiblePosition construction might move the start and end. Do we have test cases that emphasize the stress test cases where VisiblePosition 'moves" the Position a long way? > Source/WebCore/editing/TextManipulationController.h:138 > + QualifiedName attributeName { nullQName() }; Truly puzzled why we have a function nullQName() rather than making the QualifiedName default constructor do that. > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:872 > + [webView synchronouslyLoadHTMLString:@"<!DOCTYPE html><html><head><title>hey</title></head>" > + "<body><div><span aria-label=\"this is greet\">hello</span><img src=\"apple.gif\" alt=\"fruit\"></body></html>"]; This is missing a "</div>"; I’m guessing that’s an accident, not intentional. > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:906 > + EXPECT_WK_STREQ("<head><title>Hello</title></head><body><div><span aria-label=\"This is a greeting\">hello</span>" > + "<img src=\"apple.gif\" alt=\"Apple\"></body>", [webView stringByEvaluatingJavaScript:@"document.documentElement.innerHTML"]); This is missing a "</div>", which seems to be the reason the test is failing.
(In reply to Darin Adler from comment #3) > Comment on attachment 392856 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392856&action=review > > > Source/WebCore/editing/TextManipulationController.cpp:232 > > + static const auto attributeNames = makeNeverDestroyed(HashSet<QualifiedName> { > > + titleAttr, > > + altAttr, > > + placeholderAttr, > > + aria_labelAttr, > > + aria_placeholderAttr, > > + aria_roledescriptionAttr, > > + aria_valuetextAttr, > > + }); > > + return attributeNames.get().contains(name); > > I guess the easiest way to write this code is to use HashSet, and there’s no > easy way to write code for other approaches. Seems a tiny bit unfortunate > since to compare 1 pointer with 7 other pointers I suspect a linear search > might be faster than hashing. Yeah, I think 7 is right around the corner of hash map being faster compared to a linear search but I think we can use a linear search for now. Fixed. > > Source/WebCore/editing/TextManipulationController.cpp:239 > > + ParagraphContentIterator iterator { start, end }; > > Unclear to me why it’s safe to make the iterator out of the Position > objects, when the VisiblePosition construction might move the start and end. > Do we have test cases that emphasize the stress test cases where > VisiblePosition 'moves" the Position a long way? Yes. the newly added test would fail because if we had used VisiblePosition, we'd skip over the first aria-label as the first canonicalized position is inside the span. > > Source/WebCore/editing/TextManipulationController.h:138 > > + QualifiedName attributeName { nullQName() }; > > Truly puzzled why we have a function nullQName() rather than making the > QualifiedName default constructor do that. Yeah, we should probably do that. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/TextManipulation.mm:872 > > + [webView synchronouslyLoadHTMLString:@"<!DOCTYPE html><html><head><title>hey</title></head>" > > + "<body><div><span aria-label=\"this is greet\">hello</span><img src=\"apple.gif\" alt=\"fruit\"></body></html>"]; > > This is missing a "</div>"; I’m guessing that’s an accident, not intentional. Weird, not sure what happened here because it was passing locally. Fixed.
Created attachment 392916 [details] Patch
Comment on attachment 392916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392916&action=review > Source/WebCore/editing/TextManipulationController.cpp:223 > + static const QualifiedName* attributeNames[] = { Could add one more const here: static const QualifiedName* const attributeNames[] = {
Comment on attachment 392916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392916&action=review >> Source/WebCore/editing/TextManipulationController.cpp:223 >> + static const QualifiedName* attributeNames[] = { > > Could add one more const here: > > static const QualifiedName* const attributeNames[] = { Or one of these? static constexpr const QualifiedName* attributeNames[] = { static constexpr const QualifiedName* const attributeNames[] = {
(In reply to Darin Adler from comment #7) > Comment on attachment 392916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392916&action=review > > >> Source/WebCore/editing/TextManipulationController.cpp:223 > >> + static const QualifiedName* attributeNames[] = { > > > > Could add one more const here: > > > > static const QualifiedName* const attributeNames[] = { > > Or one of these? > > static constexpr const QualifiedName* attributeNames[] = { > static constexpr const QualifiedName* const attributeNames[] = { I guess we can't have three const's here: ./editing/TextManipulationController.cpp:223:42: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier] static constexpr const QualifiedName const* attributeNames[] = { ^~~~~ We can do constexpr though.
./editing/TextManipulationController.cpp:223:43: error: constexpr variable 'attributeNames' must be initialized by a constant expression static constexpr const QualifiedName* attributeNames[] = { ^ ~ ./editing/TextManipulationController.cpp:224:20: note: non-constexpr function 'get' cannot be used in a constant expression &titleAttr.get(), ^ Hm... I guess this won't quite work either. I guess we can go with const const.
(In reply to Ryosuke Niwa from comment #8) > (In reply to Darin Adler from comment #7) > > Comment on attachment 392916 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=392916&action=review > > > > >> Source/WebCore/editing/TextManipulationController.cpp:223 > > >> + static const QualifiedName* attributeNames[] = { > > > > > > Could add one more const here: > > > > > > static const QualifiedName* const attributeNames[] = { > > > > Or one of these? > > > > static constexpr const QualifiedName* attributeNames[] = { > > static constexpr const QualifiedName* const attributeNames[] = { > > I guess we can't have three const's here: > > ./editing/TextManipulationController.cpp:223:42: error: duplicate 'const' > declaration specifier [-Werror,-Wduplicate-decl-specifier] > static constexpr const QualifiedName const* attributeNames[] = { > ^~~~~ > We can do constexpr though. Oops, this is obviously wrong. It should read const QualifiedName * const
Comment on attachment 392916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392916&action=review Looks reasonable to me. > Source/WebCore/ChangeLog:10 > + It also make observeParagraphs observe content across the entire document since canonicalizing Nit - It also makes > Source/WebCore/ChangeLog:17 > + (WebCore::TextManipulationController::startObservingParagraphs): Now takes two Position's instead (nit - no apostrophes after the plural nouns)
(In reply to Wenson Hsieh from comment #11) > Comment on attachment 392916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392916&action=review > > Looks reasonable to me. > > > Source/WebCore/ChangeLog:10 > > + It also make observeParagraphs observe content across the entire document since canonicalizing > > Nit - It also makes Fixed. > > Source/WebCore/ChangeLog:17 > > + (WebCore::TextManipulationController::startObservingParagraphs): Now takes two Position's instead > > (nit - no apostrophes after the plural nouns) Ditto.
Committed r258093: <https://trac.webkit.org/changeset/258093>
Comment on attachment 392916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392916&action=review >>>>> Source/WebCore/editing/TextManipulationController.cpp:223 >>>>> + static const QualifiedName* attributeNames[] = { >>>> >>>> Could add one more const here: >>>> >>>> static const QualifiedName* const attributeNames[] = { >>> >>> Or one of these? >>> >>> static constexpr const QualifiedName* attributeNames[] = { >>> static constexpr const QualifiedName* const attributeNames[] = { >> >> I guess we can't have three const's here: >> >> ./editing/TextManipulationController.cpp:223:42: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier] >> static constexpr const QualifiedName const* attributeNames[] = { >> ^~~~~ >> We can do constexpr though. > > Oops, this is obviously wrong. It should read const QualifiedName * const Did you re-try the constexpr after moving the second const to the right place?
(In reply to Darin Adler from comment #14) > Comment on attachment 392916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392916&action=review > > >>>>> Source/WebCore/editing/TextManipulationController.cpp:223 > >>>>> + static const QualifiedName* attributeNames[] = { > >>>> > >>>> Could add one more const here: > >>>> > >>>> static const QualifiedName* const attributeNames[] = { > >>> > >>> Or one of these? > >>> > >>> static constexpr const QualifiedName* attributeNames[] = { > >>> static constexpr const QualifiedName* const attributeNames[] = { > >> > >> I guess we can't have three const's here: > >> > >> ./editing/TextManipulationController.cpp:223:42: error: duplicate 'const' declaration specifier [-Werror,-Wduplicate-decl-specifier] > >> static constexpr const QualifiedName const* attributeNames[] = { > >> ^~~~~ > >> We can do constexpr though. > > > > Oops, this is obviously wrong. It should read const QualifiedName * const > > Did you re-try the constexpr after moving the second const to the right > place? Yes. I think the compiler doesn’t like the get() call because it’s LazyNeverDestroyed so it has to do some work at runtime.