Commit e634e583 authored by Mathias Carlen's avatar Mathias Carlen Committed by Commit Bot

[Autofill Assistant] Make empty strings valid status messages.

Before this patch, empty strings would be filtered before sent to the
UI. The result of that was some tell_actions with single space ' '
messages to clear the status bar. This patch removes that constraint and
empty strings will just remove the status message shown.

As I drive by there's now a unittest for the tell action, even if that's
going the wrong way in the stack ...

R=arbesser@google.com

Bug: b/143757287
Change-Id: I843a0cf36e246c4480de4ffa8fe59709ef576bd7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1911211
Commit-Queue: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: default avatarClemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/master@{#715240}
parent 297c73f8
...@@ -67,7 +67,9 @@ class AssistantInfoBoxViewBinder ...@@ -67,7 +67,9 @@ class AssistantInfoBoxViewBinder
} }
private void setInfoBox(AssistantInfoBox infoBox, ViewHolder viewHolder) { private void setInfoBox(AssistantInfoBox infoBox, ViewHolder viewHolder) {
viewHolder.mExplanationView.setText(infoBox.getExplanation()); String explanation = infoBox.getExplanation();
viewHolder.mExplanationView.setText(explanation);
viewHolder.mExplanationView.announceForAccessibility(explanation);
if (infoBox.getImagePath().isEmpty()) { if (infoBox.getImagePath().isEmpty()) {
viewHolder.mExplanationView.setCompoundDrawablesWithIntrinsicBounds(0, 0, 0, 0); viewHolder.mExplanationView.setCompoundDrawablesWithIntrinsicBounds(0, 0, 0, 0);
} else { } else {
......
...@@ -404,12 +404,10 @@ void UiControllerAndroid::SetupForState() { ...@@ -404,12 +404,10 @@ void UiControllerAndroid::SetupForState() {
} }
void UiControllerAndroid::OnStatusMessageChanged(const std::string& message) { void UiControllerAndroid::OnStatusMessageChanged(const std::string& message) {
if (!message.empty()) { JNIEnv* env = AttachCurrentThread();
JNIEnv* env = AttachCurrentThread(); Java_AssistantHeaderModel_setStatusMessage(
Java_AssistantHeaderModel_setStatusMessage( env, GetHeaderModel(),
env, GetHeaderModel(), base::android::ConvertUTF8ToJavaString(env, message));
base::android::ConvertUTF8ToJavaString(env, message));
}
} }
void UiControllerAndroid::OnBubbleMessageChanged(const std::string& message) { void UiControllerAndroid::OnBubbleMessageChanged(const std::string& message) {
......
...@@ -197,6 +197,7 @@ source_set("unit_tests") { ...@@ -197,6 +197,7 @@ source_set("unit_tests") {
"actions/prompt_action_unittest.cc", "actions/prompt_action_unittest.cc",
"actions/set_form_field_value_action_unittest.cc", "actions/set_form_field_value_action_unittest.cc",
"actions/show_details_action_unittest.cc", "actions/show_details_action_unittest.cc",
"actions/tell_action_unittest.cc",
"actions/use_address_action_unittest.cc", "actions/use_address_action_unittest.cc",
"actions/use_credit_card_action_unittest.cc", "actions/use_credit_card_action_unittest.cc",
"actions/wait_for_document_action_unittest.cc", "actions/wait_for_document_action_unittest.cc",
......
...@@ -25,7 +25,9 @@ FocusElementAction::~FocusElementAction() {} ...@@ -25,7 +25,9 @@ FocusElementAction::~FocusElementAction() {}
void FocusElementAction::InternalProcessAction(ProcessActionCallback callback) { void FocusElementAction::InternalProcessAction(ProcessActionCallback callback) {
const FocusElementProto& focus_element = proto_.focus_element(); const FocusElementProto& focus_element = proto_.focus_element();
if (!focus_element.title().empty()) { if (focus_element.has_title()) {
// TODO(crbug.com/806868): Deprecate and remove message from this action and
// use tell instead.
delegate_->SetStatusMessage(focus_element.title()); delegate_->SetStatusMessage(focus_element.title());
} }
Selector selector = Selector(focus_element.element()).MustBeVisible(); Selector selector = Selector(focus_element.element()).MustBeVisible();
......
...@@ -33,7 +33,11 @@ void PromptAction::InternalProcessAction(ProcessActionCallback callback) { ...@@ -33,7 +33,11 @@ void PromptAction::InternalProcessAction(ProcessActionCallback callback) {
} }
callback_ = std::move(callback); callback_ = std::move(callback);
delegate_->SetStatusMessage(proto_.prompt().message()); if (proto_.prompt().has_message()) {
// TODO(b/144468818): Deprecate and remove message from this action and use
// tell instead.
delegate_->SetStatusMessage(proto_.prompt().message());
}
SetupPreconditions(); SetupPreconditions();
UpdateUserActions(); UpdateUserActions();
......
...@@ -28,6 +28,7 @@ using ::testing::IsNull; ...@@ -28,6 +28,7 @@ using ::testing::IsNull;
using ::testing::Pointee; using ::testing::Pointee;
using ::testing::Property; using ::testing::Property;
using ::testing::SizeIs; using ::testing::SizeIs;
using ::testing::StrEq;
using ::testing::UnorderedElementsAre; using ::testing::UnorderedElementsAre;
class PromptActionTest : public testing::Test { class PromptActionTest : public testing::Test {
...@@ -252,5 +253,43 @@ TEST_F(PromptActionTest, Terminate) { ...@@ -252,5 +253,43 @@ TEST_F(PromptActionTest, Terminate) {
(*user_actions_)[0].Call(TriggerContext::CreateEmpty()); (*user_actions_)[0].Call(TriggerContext::CreateEmpty());
} }
TEST_F(PromptActionTest, NoMessageSet) {
auto* ok_proto = prompt_proto_->add_choices();
ok_proto->mutable_chip()->set_text("Ok");
ok_proto->mutable_chip()->set_type(HIGHLIGHTED_ACTION);
ok_proto->set_server_payload("ok");
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(_)).Times(0);
PromptAction action(&mock_action_delegate_, proto_);
action.ProcessAction(callback_.Get());
}
TEST_F(PromptActionTest, EmptyMessage) {
prompt_proto_->set_message("");
auto* ok_proto = prompt_proto_->add_choices();
ok_proto->mutable_chip()->set_text("Ok");
ok_proto->mutable_chip()->set_type(HIGHLIGHTED_ACTION);
ok_proto->set_server_payload("ok");
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(StrEq("")));
PromptAction action(&mock_action_delegate_, proto_);
action.ProcessAction(callback_.Get());
}
TEST_F(PromptActionTest, NormalMessageSet) {
prompt_proto_->set_message(" test message ");
auto* ok_proto = prompt_proto_->add_choices();
ok_proto->mutable_chip()->set_text("Ok");
ok_proto->mutable_chip()->set_type(HIGHLIGHTED_ACTION);
ok_proto->set_server_payload("ok");
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(StrEq(" test message ")));
PromptAction action(&mock_action_delegate_, proto_);
action.ProcessAction(callback_.Get());
}
} // namespace } // namespace
} // namespace autofill_assistant } // namespace autofill_assistant
...@@ -24,7 +24,9 @@ ShowProgressBarAction::~ShowProgressBarAction() {} ...@@ -24,7 +24,9 @@ ShowProgressBarAction::~ShowProgressBarAction() {}
void ShowProgressBarAction::InternalProcessAction( void ShowProgressBarAction::InternalProcessAction(
ProcessActionCallback callback) { ProcessActionCallback callback) {
if (!proto_.show_progress_bar().message().empty()) { if (proto_.show_progress_bar().has_message()) {
// TODO(crbug.com/806868): Deprecate and remove message from this action and
// use tell instead.
delegate_->SetStatusMessage(proto_.show_progress_bar().message()); delegate_->SetStatusMessage(proto_.show_progress_bar().message());
} }
int progress = int progress =
......
...@@ -19,8 +19,9 @@ TellAction::TellAction(ActionDelegate* delegate, const ActionProto& proto) ...@@ -19,8 +19,9 @@ TellAction::TellAction(ActionDelegate* delegate, const ActionProto& proto)
TellAction::~TellAction() {} TellAction::~TellAction() {}
void TellAction::InternalProcessAction(ProcessActionCallback callback) { void TellAction::InternalProcessAction(ProcessActionCallback callback) {
// tell.message in the proto is localized. if (proto_.tell().has_message()) {
delegate_->SetStatusMessage(proto_.tell().message()); delegate_->SetStatusMessage(proto_.tell().message());
}
if (proto_.tell().needs_ui()) if (proto_.tell().needs_ui())
delegate_->RequireUI(); delegate_->RequireUI();
......
// Copyright 2019 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 "components/autofill_assistant/browser/actions/tell_action.h"
#include "base/test/gmock_callback_support.h"
#include "base/test/mock_callback.h"
#include "components/autofill_assistant/browser/actions/mock_action_delegate.h"
#include "components/autofill_assistant/browser/client_memory.h"
#include "components/autofill_assistant/browser/service.pb.h"
#include "testing/gmock/include/gmock/gmock.h"
namespace autofill_assistant {
namespace {
using ::testing::_;
using ::testing::Invoke;
using ::testing::Property;
using ::testing::Return;
using ::testing::StrEq;
class TellActionTest : public testing::Test {
public:
TellActionTest() {}
void SetUp() override {
ON_CALL(mock_action_delegate_, SetStatusMessage(_)).WillByDefault(Return());
}
protected:
void Run() {
ActionProto action_proto;
*action_proto.mutable_tell() = proto_;
TellAction action(&mock_action_delegate_, action_proto);
action.ProcessAction(callback_.Get());
}
MockActionDelegate mock_action_delegate_;
base::MockCallback<Action::ProcessActionCallback> callback_;
TellProto proto_;
};
TEST_F(TellActionTest, EmptyProtoSetsMessageDoesNothing) {
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(_)).Times(0);
// The needs_ui default is true.
EXPECT_CALL(mock_action_delegate_, RequireUI());
EXPECT_CALL(
callback_,
Run(Pointee(Property(&ProcessedActionProto::status, ACTION_APPLIED))));
Run();
}
TEST_F(TellActionTest, EmptyStringSetsMessage) {
proto_.set_message("");
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(StrEq("")));
// The needs_ui default is true.
EXPECT_CALL(mock_action_delegate_, RequireUI());
EXPECT_CALL(
callback_,
Run(Pointee(Property(&ProcessedActionProto::status, ACTION_APPLIED))));
Run();
}
TEST_F(TellActionTest, SetStatusMessageCalled) {
proto_.set_message(" test_string ");
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(StrEq(" test_string ")));
EXPECT_CALL(mock_action_delegate_, RequireUI());
EXPECT_CALL(
callback_,
Run(Pointee(Property(&ProcessedActionProto::status, ACTION_APPLIED))));
Run();
}
TEST_F(TellActionTest, RequireUI) {
proto_.set_message(" test_string ");
proto_.set_needs_ui(false);
EXPECT_CALL(mock_action_delegate_, SetStatusMessage(StrEq(" test_string ")));
EXPECT_CALL(mock_action_delegate_, RequireUI()).Times(0);
EXPECT_CALL(
callback_,
Run(Pointee(Property(&ProcessedActionProto::status, ACTION_APPLIED))));
Run();
}
} // namespace
} // namespace autofill_assistant
...@@ -822,6 +822,11 @@ message SelectOptionProto { ...@@ -822,6 +822,11 @@ message SelectOptionProto {
// Contain a localized text message from the server. // Contain a localized text message from the server.
message TellProto { message TellProto {
// The message to show in the status bar. The behavior is now the following
//
// * If the field is set, the status bar is updated (explicitly setting an
// empty string clears the status bar).
// * If the field is not set, the status bar is not updated.
optional string message = 1; optional string message = 1;
// Show the UI if it's not shown yet, such as when a script has been started // Show the UI if it's not shown yet, such as when a script has been started
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment