Skip to content

Commit

Permalink
[Autofill Assistant] Restore touchable area after interrupt
Browse files Browse the repository at this point in the history
In https://crrev.com/c/3275456 we started restoring the state after
an interrupt has finished running.
However this introduced a bug which causes the touchable area not to be
restored correctly (see comment #9 in the bug).
This change "manually" restores the touchable area to its previous state
after an interrupt is run.

Bug: b/207435278
Change-Id: Id8fc5b96f1a4d2f2b9386b08490bc2d241ad6e67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3312491
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Commit-Queue: Luca Hunkeler <hluca@google.com>
Cr-Commit-Position: refs/heads/main@{#949486}
  • Loading branch information
Luca Hunkeler authored and Chromium LUCI CQ committed Dec 8, 2021
1 parent ad410e1 commit 63a6497
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ AutofillAssistantState FakeScriptExecutorDelegate::GetState() {
}

void FakeScriptExecutorDelegate::SetTouchableElementArea(
const ElementAreaProto& element) {}
const ElementAreaProto& element_area) {
touchable_element_area_history_.emplace_back(element_area);
}

void FakeScriptExecutorDelegate::SetStatusMessage(const std::string& message) {
status_message_ = message;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
std::vector<AutofillAssistantState> GetStateHistory() {
return state_history_;
}
std::vector<ElementAreaProto> GetTouchableElementAreaHistory() {
return touchable_element_area_history_;
}

const std::vector<Details>& GetDetails() { return details_; }

Expand Down Expand Up @@ -172,6 +175,7 @@ class FakeScriptExecutorDelegate : public ScriptExecutorDelegate {
raw_ptr<WebController> web_controller_ = nullptr;
std::unique_ptr<TriggerContext> trigger_context_;
std::vector<AutofillAssistantState> state_history_;
std::vector<ElementAreaProto> touchable_element_area_history_;
std::string status_message_;
std::string tts_message_;
std::string bubble_message_;
Expand Down
5 changes: 5 additions & 0 deletions components/autofill_assistant/browser/script_executor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1248,6 +1248,8 @@ void ScriptExecutor::WaitForDomOperation::RunInterrupt(
/* listener= */ this, &no_interrupts_, delegate_);
delegate_->EnterState(AutofillAssistantState::RUNNING);
delegate_->SetUserActions(nullptr);
// Note that we don't clear the touchable area in the delegate here.
// TODO(b/209732258): check whether this is a bug.
interrupt_executor_->Run(
main_script_->user_data_,
base::BindOnce(&ScriptExecutor::WaitForDomOperation::OnInterruptDone,
Expand Down Expand Up @@ -1312,6 +1314,9 @@ void ScriptExecutor::WaitForDomOperation::RestorePreInterruptState() {

delegate_->SetStatusMessage(saved_pre_interrupt_state_->status_message);
delegate_->EnterState(saved_pre_interrupt_state_->controller_state);
if (main_script_->touchable_element_area_) {
delegate_->SetTouchableElementArea(*main_script_->touchable_element_area_);
}
}

void ScriptExecutor::WaitForDomOperation::RestorePreInterruptScroll() {
Expand Down
55 changes: 49 additions & 6 deletions components/autofill_assistant/browser/script_executor_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ using ::testing::StrictMock;
using ::testing::UnorderedElementsAreArray;
using ::testing::WithArgs;

ElementAreaProto MakeElementAreaProto(const std::string& id) {
Selector touchable_element({id});
ElementAreaProto area;
*area.add_touchable()->add_elements() = touchable_element.proto;
return area;
}

const char* kScriptPath = "script_path";

class ScriptExecutorTest : public testing::Test,
Expand Down Expand Up @@ -1196,12 +1203,33 @@ TEST_F(ScriptExecutorTest, InterruptReturnsShutdown) {
}

TEST_F(ScriptExecutorTest, RunInterruptDuringPrompt) {
SetupInterrupt("interrupt", "interrupt_trigger");
RegisterInterrupt("interrupt", "interrupt_trigger");

ActionsResponseProto interrupt_actions;
InitInterruptActions(&interrupt_actions, "interrupt");
ElementAreaProto interrupt_area =
MakeElementAreaProto(/* id = */ "interrupt_area");
*interrupt_actions.add_actions()
->mutable_set_touchable_area()
->mutable_element_area() = interrupt_area;
auto* interrupt_prompt = interrupt_actions.add_actions()->mutable_prompt();
*interrupt_prompt->add_choices()
->mutable_auto_select_when()
->mutable_match() = ToSelectorProto("end_prompt");

EXPECT_CALL(mock_service_, OnGetActions("interrupt", _, _, _, _, _))
.WillRepeatedly(
RunOnceCallback<5>(net::HTTP_OK, Serialize(interrupt_actions)));

// Main script has a prompt with an "auto_select" element. This functions very
// much like a WaitForDom, except for the UI changes triggered by the switches
// between PROMPT and RUNNING states.
ActionsResponseProto interruptible;
ElementAreaProto interruptible_area =
MakeElementAreaProto(/* id = */ "interruptible_area");
*interruptible.add_actions()
->mutable_set_touchable_area()
->mutable_element_area() = interruptible_area;
auto* prompt_action = interruptible.add_actions()->mutable_prompt();
prompt_action->set_allow_interrupt(true);
*prompt_action->add_choices()->mutable_auto_select_when()->mutable_match() =
Expand Down Expand Up @@ -1237,15 +1265,30 @@ TEST_F(ScriptExecutorTest, RunInterruptDuringPrompt) {
// - show prompt (enter PROMPT state)
// - notice interrupt_trigger element
// - run interrupt (enter RUNNING state)
// - show the interrupt's prompt (enter PROMPT state)
// - the interrupt finishes (enter RUNNING state)
// - show prompt again (enter PROMPT state)
// - notice end_prompt element
// - end prompt, continue main script (enter RUNNING state)
// - run tell, which sets message to "done"
EXPECT_THAT(delegate_.GetStateHistory(),
ElementsAre(AutofillAssistantState::PROMPT,
AutofillAssistantState::RUNNING,
AutofillAssistantState::PROMPT,
AutofillAssistantState::RUNNING));
EXPECT_THAT(
delegate_.GetStateHistory(),
ElementsAre(
AutofillAssistantState::PROMPT, AutofillAssistantState::RUNNING,
AutofillAssistantState::PROMPT, AutofillAssistantState::RUNNING,
AutofillAssistantState::PROMPT, AutofillAssistantState::RUNNING));
// Expected scenario:
// - the main script's SetTouchableArea sets |interruptible_area|
// - the interrupt starts
// - the interrupt's SetTouchableArea sets |interrupt_area|
// - the area is cleaned up at the end of the interrupt's prompt
// - when the main script resumes, we restore |interruptible_area|
// - the area is cleaned up again at the end of the main script's prompt
EXPECT_THAT(
delegate_.GetTouchableElementAreaHistory(),
ElementsAre(interruptible_area, interrupt_area,
ElementAreaProto::default_instance(), interruptible_area,
ElementAreaProto::default_instance()));
EXPECT_EQ("done", delegate_.GetStatusMessage());
}

Expand Down

0 comments on commit 63a6497

Please sign in to comment.