208759 – TextManipulationController should work with ARIA labels

RESOLVED FIXED 208759

TextManipulationController should work with ARIA labels

https://bugs.webkit.org/show_bug.cgi?id=208759

Summary TextManipulationController should work with ARIA labels

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	Details Formatted Diff Diff
Patch (16.64 KB, patch) 2020-03-07 17:09 PST, Ryosuke Niwa	wenson_hsieh: review+	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Ryosuke Niwa

Reported

2020-03-07 00:55 PST

Modified

2020-03-07 23:16 PST History

CC List

7 users Show

URL

Keywords InRadar

Depends on

Blocks