Expand test defaults to include more states, include more blink test results

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/path_service.h"
@@ skipping 37 matching lines @@
 //
 // The flow of the test is as outlined below.
 // 1. Load an html file from content/test/data/accessibility.
 // 2. Read the expectation.
 // 3. Browse to the page and serialize the platform specific tree into a human
 //    readable string.
 // 4. Perform a comparison between actual and expected and fail if they do not
 //    exactly match.
 class DumpAccessibilityTreeTest : public DumpAccessibilityTestBase {
  public:
   void AddDefaultFilters(std::vector<Filter>* filters) override {

[dmazzoni, 2017/06/13 20:25:33]

Since this function is getting long, declare it here and then
define it below the class. That lets you indent by 2 less,
might help wrapping a tiny bit but definitely will make the
class declaration a bit more readable.

[aleventhal, 2017/06/14 13:17:26]

Done.

-    filters->push_back(Filter(base::ASCIIToUTF16("FOCUSABLE"), Filter::ALLOW));
+    // TODO(aleventhal) Each platform deserves separate default filters.
+    // Windows
+    // Note: SELECTABLE, IA2_STATE_OPAQUE, LINKED off by default -- too noisy.
+    filters->push_back(Filter(base::ASCIIToUTF16("ALERT*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("ANIMATED*"), Filter::ALLOW));

[dmazzoni, 2017/06/13 20:25:33]

Do we actually use this?

I'm tempted to get rid of ones we don't actually use *at all*,
or at least group them separately with a comment "not actually
used yet".

[aleventhal, 2017/06/14 13:17:25]

I want to keep them when we don't use them, to make sure we never do
unintentionally.

+    filters->push_back(Filter(base::ASCIIToUTF16("BUSY"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("CHECKED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("COLLAPSED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("DEFAULT"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("EXPANDED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("FLOATING"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("FOCUS*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("HASPOPUP"), Filter::ALLOW));
+    // Too noisy:
+    // filters->push_back(Filter(base::ASCIIToUTF16("HOTTRACKED"),

[dmazzoni, 2017/06/13 20:25:33]

Maybe put all of the cpmmented-out off by default ones at the top or bottom.

Feel free to use more whitespace in this function, like:

//
// Windows
//

// Windows states to log by default
filters->push_back(
filters->push_back(
filters->push_back(

// Other Windows attributes

[aleventhal, 2017/06/14 13:17:26]

Done.

+    // Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("INVISIBLE"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("MARQUEED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("MIXED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("MOVEABLE"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("MULTISELECTABLE"), Filter::ALLOW));
+    // Produces false-positive for contentless elements, including <br>
+    // filters->push_back(Filter(base::ASCIIToUTF16("OFFSCREEN"),
+    // Filter::ALLOW));
     filters->push_back(Filter(base::ASCIIToUTF16("READONLY"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("PRESSED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("PROTECTED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("SELECTED"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("SIZEABLE"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("TRAVERSED"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("UNAVAILABLE"), Filter::ALLOW));
+    // IA2- No editable, opaque, single_line, selectable_text, vertical (noisy):
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_ACTIVE"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_ARMED"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_CHECKABLE"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_DEFUNCT"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_HORIZONTAL"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_ICONIFIED"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_INVALID_ENTRY"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_MODAL"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_MULTI_LINE"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_PINNED"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_REQUIRED"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_STALE"), Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("IA2_STATE_TRANSIENT"), Filter::ALLOW));
+
+    // Blink
+    filters->push_back(Filter(base::ASCIIToUTF16("check*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("descript*"), Filter::ALLOW));

[dmazzoni, 2017/06/13 20:25:33]

I vote for either both nameFrom and descriptionFrom, or neither - but having
descriptionFrom but not nameFrom feels wrong

[aleventhal, 2017/06/14 13:17:25]

I wanted to keep descriptionFrom because it's rare, whereas nameFrom is crazy
common.

+    // TODO(aleventhal) Add disabled back after control mode refactor
+    // filters->push_back(Filter(base::ASCIIToUTF16("disabled"),
+    // Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("invalid"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("busy"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("collapsed"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("default"), Filter::ALLOW));
+    // TODO(aleventhal) Perhaps add editable and richlyEditable in later
+    // filters->push_back(Filter(base::ASCIIToUTF16("editable"),
+    // Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("expandable"), Filter::ALLOW));
+    // TODO(aleventhal) focus* causing a lot of changes, maybe add later
+    // filters->push_back(Filter(base::ASCIIToUTF16("focus*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("haspopup"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("horizontal"), Filter::ALLOW));

[dmazzoni, 2017/06/13 20:25:33]

This should probably be an orientation enum {horizontal, veritical,
not_applicable}, right?

Does this one show up a lot? I think it might be distracting

[aleventhal, 2017/06/14 13:17:26]

Let's make it an enum at some point. For now, horizontal is rare so not very
distracting.

+    // Testing hovered would be flaky:
+    // filters->push_back(Filter(base::ASCIIToUTF16("hovered"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("invisible"), Filter::ALLOW));
+    // Linked is disallowed by default -- too noisy
+    // filters->push_back(Filter(base::ASCIIToUTF16("linked"), Filter::ALLOW));
+    // TODO(aleventhal) Add multiline in separate patch
+    // filters->push_back(Filter(base::ASCIIToUTF16("multiline"),
+    // Filter::ALLOW));
+    filters->push_back(
+        Filter(base::ASCIIToUTF16("multiselectable"), Filter::ALLOW));
+    // Produces false-positive for contentless elements, including <br>
+    // filters->push_back(Filter(base::ASCIIToUTF16("offscreen"),
+    // Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("protected"), Filter::ALLOW));
+    // TODO(aleventhal) Add readonly support back after control mode refactor
+    // filters->push_back(Filter(base::ASCIIToUTF16("read*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("required"), Filter::ALLOW));
+    // TODO(aleventhal) Perhaps add editable and richlyEditable in later
+    // filters->push_back(Filter(base::ASCIIToUTF16("richly*"), Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("select*"), Filter::ALLOW));
+    // Vertical is a bit noisy -- we test for horizontal though:
+    // filters->push_back(Filter(base::ASCIIToUTF16("vertical"),
+    // Filter::ALLOW));
+    filters->push_back(Filter(base::ASCIIToUTF16("visited"), Filter::ALLOW));
+
+    // OS X
     filters->push_back(Filter(base::ASCIIToUTF16("roleDescription=*"),
                               Filter::ALLOW));
+
+    // General
     filters->push_back(Filter(base::ASCIIToUTF16("*=''"), Filter::DENY));
     // After denying empty values, because we want to allow name=''
     filters->push_back(
         Filter(base::ASCIIToUTF16("name=*"), Filter::ALLOW_EMPTY));
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     DumpAccessibilityTestBase::SetUpCommandLine(command_line);
     // Enable <dialog>, which is used in some tests.
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ skipping 1413 matching lines @@
 // crbug.com/281952
 IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, DISABLED_AccessibilityVideo) {
   RunHtmlTest(FILE_PATH_LITERAL("video.html"));
 }
 
 IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityWbr) {
   RunHtmlTest(FILE_PATH_LITERAL("wbr.html"));
 }
 
 }  // namespace content