[Autofill Assistant] Replace the pick_one filter by nth_match.

This CL replaces the pick_one Selector filter by the nth_match filter,
that matches the nth element of the elements currently matching.

See [1] for more background.

[1] https://docs.google.com/document/d/167LaI_WQNr31wBuO3DNlCPD2sx3If2GOHpPsT4atXIk

Change-Id: I3024239d256201fe604c0c72b4985a9de3d45201
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517535
Commit-Queue: Jordan Demeulenaere <jdemeulenaere@chromium.org>
Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#824854}

chrome/android/features/autofill_assistant/javatests/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantKeyboardIntegrationTest.java

@@ -251,8 +251,8 @@ public class AutofillAssistantKeyboardIntegrationTest {
         SelectorProto element =
                 (SelectorProto) SelectorProto.newBuilder()
                         .addFilters(SelectorProto.Filter.newBuilder().setCssSelector("#iframe"))
-                        .addFilters(SelectorProto.Filter.newBuilder().setPickOne(
-                                SelectorProto.EmptyFilter.getDefaultInstance()))
+                        .addFilters(SelectorProto.Filter.newBuilder().setNthMatch(
+                                SelectorProto.NthMatchFilter.newBuilder().setIndex(0)))
                         .addFilters(SelectorProto.Filter.newBuilder().setEnterFrame(
                                 SelectorProto.EmptyFilter.getDefaultInstance()))
                         .addFilters(SelectorProto.Filter.newBuilder().setCssSelector("#name"))

chrome/android/features/autofill_assistant/javatests/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantOverlayIntegrationTest.java

@@ -255,8 +255,8 @@ public class AutofillAssistantOverlayIntegrationTest {
         SelectorProto element =
                 (SelectorProto) SelectorProto.newBuilder()
                         .addFilters(SelectorProto.Filter.newBuilder().setCssSelector("#iframe"))
-                        .addFilters(SelectorProto.Filter.newBuilder().setPickOne(
-                                SelectorProto.EmptyFilter.getDefaultInstance()))
+                        .addFilters(SelectorProto.Filter.newBuilder().setNthMatch(
+                                SelectorProto.NthMatchFilter.newBuilder().setIndex(0)))
                         .addFilters(SelectorProto.Filter.newBuilder().setEnterFrame(
                                 SelectorProto.EmptyFilter.getDefaultInstance()))
 
@@ -310,8 +310,8 @@ public class AutofillAssistantOverlayIntegrationTest {
         SelectorProto element =
                 (SelectorProto) SelectorProto.newBuilder()
                         .addFilters(SelectorProto.Filter.newBuilder().setCssSelector("#iframe"))
-                        .addFilters(SelectorProto.Filter.newBuilder().setPickOne(
-                                SelectorProto.EmptyFilter.getDefaultInstance()))
+                        .addFilters(SelectorProto.Filter.newBuilder().setNthMatch(
+                                SelectorProto.NthMatchFilter.newBuilder().setIndex(0)))
                         .addFilters(SelectorProto.Filter.newBuilder().setEnterFrame(
                                 SelectorProto.EmptyFilter.getDefaultInstance()))
 

components/autofill_assistant/browser/selector.cc

@@ -71,7 +71,6 @@ bool operator<(const SelectorProto::Filter& a, const SelectorProto::Filter& b) {
 
     case SelectorProto::Filter::kBoundingBox:
     case SelectorProto::Filter::kEnterFrame:
-    case SelectorProto::Filter::kPickOne:
     case SelectorProto::Filter::kLabelled:
       return false;
 
@@ -85,6 +84,9 @@ bool operator<(const SelectorProto::Filter& a, const SelectorProto::Filter& b) {
     case SelectorProto::Filter::kMatchCssSelector:
       return a.match_css_selector() < b.match_css_selector();
 
+    case SelectorProto::Filter::kNthMatch:
+      return a.nth_match().index() < b.nth_match().index();
+
     case SelectorProto::Filter::FILTER_NOT_SET:
       return false;
   }
@@ -100,7 +102,7 @@ SelectorProto ToSelectorProto(const std::vector<std::string>& s) {
   if (!s.empty()) {
     for (size_t i = 0; i < s.size(); i++) {
       if (i > 0) {
-        proto.add_filters()->mutable_pick_one();
+        proto.add_filters()->mutable_nth_match()->set_index(0);
         proto.add_filters()->mutable_enter_frame();
       }
       proto.add_filters()->set_css_selector(s[i]);
@@ -226,10 +228,11 @@ base::Optional<std::string> Selector::ExtractSingleCssSelectorForAutofill()
         break;
 
       case SelectorProto::Filter::kBoundingBox:
-      case SelectorProto::Filter::kPickOne:
-        // Ignore these; they're not relevant for the autofill use-case
+      case SelectorProto::Filter::kNthMatch:
+        if (filter.nth_match().index() == 0)
           break;
 
+        FALLTHROUGH;
       case SelectorProto::Filter::kInnerText:
       case SelectorProto::Filter::kValue:
       case SelectorProto::Filter::kPseudoType:
@@ -351,8 +354,8 @@ std::ostream& operator<<(std::ostream& out, const SelectorProto::Filter& f) {
       out << "bounding_box";
       return out;
 
-    case SelectorProto::Filter::kPickOne:
-      out << "pick_one";
+    case SelectorProto::Filter::kNthMatch:
+      out << "nth_match[" << f.nth_match().index() << "]";
       return out;
 
     case SelectorProto::Filter::kLabelled:

components/autofill_assistant/browser/selector_unittest.cc

@@ -23,10 +23,11 @@ TEST(SelectorTest, Constructor_WithIframe) {
   Selector selector({"#frame", "#test"});
   ASSERT_EQ(4, selector.proto.filters().size());
   EXPECT_EQ("#frame", selector.proto.filters(0).css_selector());
-  EXPECT_EQ(selector.proto.filters(1).filter_case(),
-            SelectorProto::Filter::kPickOne);
-  EXPECT_EQ(selector.proto.filters(2).filter_case(),
-            SelectorProto::Filter::kEnterFrame);
+  EXPECT_EQ(SelectorProto::Filter::kNthMatch,
+            selector.proto.filters(1).filter_case());
+  EXPECT_EQ(0, selector.proto.filters(1).nth_match().index());
+  EXPECT_EQ(SelectorProto::Filter::kEnterFrame,
+            selector.proto.filters(2).filter_case());
   EXPECT_EQ("#test", selector.proto.filters(3).css_selector());
 }
 

components/autofill_assistant/browser/service.proto

@@ -992,11 +992,9 @@ message SelectorProto {
       // renamed as having a bounding box is not enough to imply visibility.
       EmptyFilter bounding_box = 6;
 
-      // Pick just one match and continue. Ignore all other matches.
-      //
-      // For backward-compatibility with older element references, pick_one can
-      // be put before enter_frame.
-      EmptyFilter pick_one = 7;
+      // Take the nth match. Fails with ELEMENT_RESOLUTION_FAILED if there are
+      // not enough matches.
+      NthMatchFilter nth_match = 7;
 
       // Only keep elements that have a pseudo-element with the given content.
       //
@@ -1208,6 +1206,11 @@ message SelectorProto {
 
   message EmptyFilter {}
 
+  message NthMatchFilter {
+    // Take the match at the given |index|.
+    optional int32 index = 1;
+  }
+
   reserved 1 to 8;
 }
 

components/autofill_assistant/browser/web/element_finder.cc

@@ -222,7 +222,7 @@ bool ElementFinder::JsFilterBuilder::AddFilter(
 
     case SelectorProto::Filter::kEnterFrame:
     case SelectorProto::Filter::kPseudoType:
-    case SelectorProto::Filter::kPickOne:
+    case SelectorProto::Filter::kNthMatch:
     case SelectorProto::Filter::kClosest:
     case SelectorProto::Filter::FILTER_NOT_SET:
       return false;
@@ -368,7 +368,7 @@ void ElementFinder::ExecuteNextTask() {
         break;
 
       case ResultType::kAnyMatch:
-        if (!ConsumeAnyMatchOrFail(object_id)) {
+        if (!ConsumeMatchAtOrFail(0, object_id)) {
           return;
         }
         break;
@@ -410,9 +410,9 @@ void ElementFinder::ExecuteNextTask() {
       return;
     }
 
-    case SelectorProto::Filter::kPickOne: {
+    case SelectorProto::Filter::kNthMatch: {
       std::string object_id;
-      if (!ConsumeAnyMatchOrFail(object_id))
+      if (!ConsumeMatchAtOrFail(filter.nth_match().index(), object_id))
         return;
 
       next_filter_index_++;
@@ -479,9 +479,10 @@ bool ElementFinder::ConsumeOneMatchOrFail(std::string& object_id_out) {
   return true;
 }
 
-bool ElementFinder::ConsumeAnyMatchOrFail(std::string& object_id_out) {
-  if (current_matches_.size() > 0) {
-    object_id_out = current_matches_[0];
+bool ElementFinder::ConsumeMatchAtOrFail(size_t index,
+                                         std::string& object_id_out) {
+  if (index < current_matches_.size()) {
+    object_id_out = current_matches_[index];
     current_matches_.clear();
     return true;
   }

components/autofill_assistant/browser/web/element_finder.h

@@ -220,17 +220,11 @@ class ElementFinder : public WebControllerWorker {
   // required data is available.
   bool ConsumeOneMatchOrFail(std::string& object_id_out);
 
-  // Make sure there's at least one match, take one and put it in
-  // |object_id_out|, then return true.
+  // Make sure there's at least |index + 1| matches, take the one at that index
+  // and put it in |object_id_out|, then return true.
   //
-  // If there are no matches, send an error response and return false.
-  // If there are not enough matches yet, fetch them in the background and
-  // return false. This calls ExecuteNextTask() once matches have been fetched.
-  //
-  // If this returns true, continue processing. If this returns false, return
-  // from ExecuteNextTask(). ExecuteNextTask() will be called again once the
-  // required data is available.
-  bool ConsumeAnyMatchOrFail(std::string& object_id_out);
+  // If there are not enough matches, send an error response and return false.
+  bool ConsumeMatchAtOrFail(size_t index, std::string& object_id_out);
 
   // Make sure there's at least one match and move them all into
   // |matches_out|.

components/autofill_assistant/browser/web/web_controller_browsertest.cc

@@ -1147,7 +1147,7 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, PseudoTypeThenBoundingBox) {
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, PseudoTypeThenPickOne) {
   Selector selector({"span"});
   selector.SetPseudoType(PseudoType::BEFORE);
-  selector.proto.add_filters()->mutable_pick_one();
+  selector.proto.add_filters()->mutable_nth_match()->set_index(0);
 
   RunStrictElementCheck(selector, true);
 }
@@ -2291,7 +2291,7 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest,
   Selector selector({"input"});
   auto* closest = selector.proto.add_filters()->mutable_closest();
   closest->add_target()->set_css_selector("#iframe");
-  closest->add_target()->mutable_pick_one();
+  closest->add_target()->mutable_nth_match()->set_index(0);
   closest->add_target()->mutable_enter_frame();
   closest->add_target()->set_css_selector("div");
 
@@ -2435,4 +2435,34 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, OnTopFindsElementInShadow) {
   RunLaxElementCheck(button, false);
 }
 
+IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, NthMatch) {
+  Selector selector;
+  selector.proto.add_filters()->set_css_selector(".nth_match_parent");
+  selector.proto.add_filters()->mutable_nth_match()->set_index(1);
+  selector.proto.add_filters()->set_css_selector(".nth_match_child");
+
+  auto* pick_at_filter = selector.proto.add_filters();
+  std::string element_tag;
+
+  pick_at_filter->mutable_nth_match()->set_index(0);
+  ASSERT_EQ(ACTION_APPLIED,
+            GetElementTag(selector, &element_tag).proto_status());
+  EXPECT_EQ("P", element_tag);
+
+  pick_at_filter->mutable_nth_match()->set_index(1);
+  ASSERT_EQ(ACTION_APPLIED,
+            GetElementTag(selector, &element_tag).proto_status());
+  EXPECT_EQ("UL", element_tag);
+
+  pick_at_filter->mutable_nth_match()->set_index(2);
+  ASSERT_EQ(ACTION_APPLIED,
+            GetElementTag(selector, &element_tag).proto_status());
+  EXPECT_EQ("LI", element_tag);
+
+  pick_at_filter->mutable_nth_match()->set_index(3);
+  ASSERT_EQ(ACTION_APPLIED,
+            GetElementTag(selector, &element_tag).proto_status());
+  EXPECT_EQ("STRONG", element_tag);
+}
+
 }  // namespace autofill_assistant

components/test/data/autofill_assistant/html/autofill_assistant_target_website.html

@@ -375,5 +375,17 @@ found in the LICENSE file.
     <div id="overlay">
       This is an overlay that covers the whole page.
     </div>
+
+    <div class="nth_match_parent">
+      <span class="nth_match_child"></span>
+    </div>
+
+    <div class="nth_match_parent">
+      <p class="nth_match_child"></p>
+      <ul class="nth_match_child">
+        <li class="nth_match_child"></li>
+      </ul>
+      <strong class="nth_match_child"></strong>
+    </div>
   </body>
 </html>