Bug 208759

Summary: TextManipulationController should work with ARIA labels
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, darin, ews-watchlist, jcraig, megan_gardner, mifenton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch wenson_hsieh: review+

Ryosuke Niwa
Reported 2020-03-07 00:55:32 PST
We need to make TextManipulationController accessibility ready.
Attachments
Patch (16.52 KB, patch)
2020-03-07 01:04 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch (16.64 KB, patch)
2020-03-07 17:09 PST, Ryosuke Niwa 		wenson_hsieh: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2020-03-07 01:04:42 PST
Created attachment 392856 [details] Patch
Ryosuke Niwa
Comment 2 2020-03-07 01:04:59 PST
<rdar://problem/58092680>
Darin Adler
Comment 3 2020-03-07 12:51:44 PST
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.
Ryosuke Niwa
Comment 4 2020-03-07 17:08:24 PST
(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.
Ryosuke Niwa
Comment 5 2020-03-07 17:09:03 PST
Created attachment 392916 [details] Patch
Darin Adler
Comment 6 2020-03-07 17:25:19 PST
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[] = {
Darin Adler
Comment 7 2020-03-07 17:26:07 PST
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[] = {
Ryosuke Niwa
Comment 8 2020-03-07 19:09:41 PST
(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.
Ryosuke Niwa
Comment 9 2020-03-07 19:10:54 PST
./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.
Ryosuke Niwa
Comment 10 2020-03-07 19:18:26 PST
(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
Wenson Hsieh
Comment 11 2020-03-07 19:43:41 PST
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)
Ryosuke Niwa
Comment 12 2020-03-07 19:49:47 PST
(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.
Ryosuke Niwa
Comment 13 2020-03-07 19:52:08 PST
Committed r258093: <https://trac.webkit.org/changeset/258093>
Darin Adler
Comment 14 2020-03-07 20:33:27 PST
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?
Ryosuke Niwa
Comment 15 2020-03-07 23:16:27 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.