Commit 0a215449 authored by Angela Yoeurng's avatar Angela Yoeurng Committed by Commit Bot

[Omnibox] Deprecate BUTTON_FOCUSED state in favor of FOCUSED_BUTTON_FOO

The vague BUTTON_FOCUSED selection state was refactored into the
more descriptive FOCUSED_BUTTON_FOO states. MaybeTriggerSecondaryButton
is also deprecated and unit tests updated.

Bug: 1099043
Change-Id: Ib79b829a51552c55349f204fa1702a85317bb803
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2284384
Commit-Queue: Angela Yoeurng <yoangela@chromium.org>
Reviewed-by: default avatarmanuk hovanesian <manukh@chromium.org>
Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786146}
parent 9fd27b69
...@@ -208,10 +208,6 @@ bool OmniboxPopupContentsView::IsSelectedIndex(size_t index) const { ...@@ -208,10 +208,6 @@ bool OmniboxPopupContentsView::IsSelectedIndex(size_t index) const {
return index == model_->selected_line(); return index == model_->selected_line();
} }
bool OmniboxPopupContentsView::IsButtonSelected() const {
return model_->selected_line_state() == OmniboxPopupModel::BUTTON_FOCUSED;
}
void OmniboxPopupContentsView::UnselectButton() { void OmniboxPopupContentsView::UnselectButton() {
model_->SetSelectedLineState(OmniboxPopupModel::NORMAL); model_->SetSelectedLineState(OmniboxPopupModel::NORMAL);
} }
......
...@@ -57,11 +57,6 @@ class OmniboxPopupContentsView : public views::View, ...@@ -57,11 +57,6 @@ class OmniboxPopupContentsView : public views::View,
// Returns true if the line specified by |index| is selected. // Returns true if the line specified by |index| is selected.
virtual bool IsSelectedIndex(size_t index) const; virtual bool IsSelectedIndex(size_t index) const;
// If the selected index has a tab switch button, whether it's "focused" via
// the tab key. Invalid if the selected index does not have a tab switch
// button.
bool IsButtonSelected() const;
// Called by the active result view to inform model (due to mouse event). // Called by the active result view to inform model (due to mouse event).
void UnselectButton(); void UnselectButton();
......
...@@ -79,7 +79,8 @@ OmniboxResultView::OmniboxResultView( ...@@ -79,7 +79,8 @@ OmniboxResultView::OmniboxResultView(
views::FocusRing::Install(remove_suggestion_button_); views::FocusRing::Install(remove_suggestion_button_);
remove_suggestion_focus_ring_->SetHasFocusPredicate([&](View* view) { remove_suggestion_focus_ring_->SetHasFocusPredicate([&](View* view) {
return view->GetVisible() && IsMatchSelected() && return view->GetVisible() && IsMatchSelected() &&
popup_contents_view_->IsButtonSelected(); (popup_contents_view_->model()->selected_line_state() ==
OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION);
}); });
if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) { if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) {
...@@ -254,7 +255,7 @@ bool OmniboxResultView::IsMatchSelected() const { ...@@ -254,7 +255,7 @@ bool OmniboxResultView::IsMatchSelected() const {
// The header button being focused means the match itself is NOT focused. // The header button being focused means the match itself is NOT focused.
return popup_contents_view_->IsSelectedIndex(model_index_) && return popup_contents_view_->IsSelectedIndex(model_index_) &&
popup_contents_view_->model()->selected_line_state() != popup_contents_view_->model()->selected_line_state() !=
OmniboxPopupModel::HEADER_BUTTON_FOCUSED; OmniboxPopupModel::FOCUSED_BUTTON_HEADER;
} }
views::Button* OmniboxResultView::GetSecondaryButton() { views::Button* OmniboxResultView::GetSecondaryButton() {
...@@ -267,15 +268,6 @@ views::Button* OmniboxResultView::GetSecondaryButton() { ...@@ -267,15 +268,6 @@ views::Button* OmniboxResultView::GetSecondaryButton() {
return nullptr; return nullptr;
} }
bool OmniboxResultView::MaybeTriggerSecondaryButton(const ui::Event& event) {
views::Button* button = GetSecondaryButton();
if (!button)
return false;
ButtonPressed(button, event);
return true;
}
OmniboxPartState OmniboxResultView::GetThemeState() const { OmniboxPartState OmniboxResultView::GetThemeState() const {
if (IsMatchSelected()) if (IsMatchSelected())
return OmniboxPartState::SELECTED; return OmniboxPartState::SELECTED;
......
...@@ -79,9 +79,6 @@ class OmniboxResultView : public views::View, ...@@ -79,9 +79,6 @@ class OmniboxResultView : public views::View,
// if none exists for this suggestion. // if none exists for this suggestion.
views::Button* GetSecondaryButton(); views::Button* GetSecondaryButton();
// If this view has a secondary button, triggers the action and returns true.
bool MaybeTriggerSecondaryButton(const ui::Event& event);
OmniboxPartState GetThemeState() const; OmniboxPartState GetThemeState() const;
// Notification that the match icon has changed and schedules a repaint. // Notification that the match icon has changed and schedules a repaint.
......
...@@ -60,7 +60,7 @@ class OmniboxRowView::HeaderView : public views::View, ...@@ -60,7 +60,7 @@ class OmniboxRowView::HeaderView : public views::View,
row_view_->popup_model_->selection() == row_view_->popup_model_->selection() ==
OmniboxPopupModel::Selection( OmniboxPopupModel::Selection(
row_view_->line_, row_view_->line_,
OmniboxPopupModel::HEADER_BUTTON_FOCUSED); OmniboxPopupModel::FOCUSED_BUTTON_HEADER);
}); });
if (row_view_->pref_service_) { if (row_view_->pref_service_) {
...@@ -129,7 +129,7 @@ class OmniboxRowView::HeaderView : public views::View, ...@@ -129,7 +129,7 @@ class OmniboxRowView::HeaderView : public views::View,
DCHECK_EQ(sender, header_toggle_button_); DCHECK_EQ(sender, header_toggle_button_);
row_view_->popup_model_->TriggerSelectionAction( row_view_->popup_model_->TriggerSelectionAction(
OmniboxPopupModel::Selection(row_view_->line_, OmniboxPopupModel::Selection(row_view_->line_,
OmniboxPopupModel::HEADER_BUTTON_FOCUSED)); OmniboxPopupModel::FOCUSED_BUTTON_HEADER));
// The PrefChangeRegistrar will update the actual button toggle state. // The PrefChangeRegistrar will update the actual button toggle state.
} }
...@@ -138,7 +138,7 @@ class OmniboxRowView::HeaderView : public views::View, ...@@ -138,7 +138,7 @@ class OmniboxRowView::HeaderView : public views::View,
OmniboxPartState part_state = OmniboxPartState::NORMAL; OmniboxPartState part_state = OmniboxPartState::NORMAL;
if (row_view_->popup_model_->selection() == if (row_view_->popup_model_->selection() ==
OmniboxPopupModel::Selection( OmniboxPopupModel::Selection(
row_view_->line_, OmniboxPopupModel::HEADER_BUTTON_FOCUSED)) { row_view_->line_, OmniboxPopupModel::FOCUSED_BUTTON_HEADER)) {
part_state = OmniboxPartState::SELECTED; part_state = OmniboxPartState::SELECTED;
} else if (IsMouseHovered()) { } else if (IsMouseHovered()) {
part_state = OmniboxPartState::HOVERED; part_state = OmniboxPartState::HOVERED;
...@@ -255,7 +255,7 @@ void OmniboxRowView::OnSelectionStateChanged() { ...@@ -255,7 +255,7 @@ void OmniboxRowView::OnSelectionStateChanged() {
views::View* OmniboxRowView::GetActiveAuxiliaryButtonForAccessibility() const { views::View* OmniboxRowView::GetActiveAuxiliaryButtonForAccessibility() const {
DCHECK(popup_model_->selection().IsButtonFocused()); DCHECK(popup_model_->selection().IsButtonFocused());
if (popup_model_->selected_line_state() == if (popup_model_->selected_line_state() ==
OmniboxPopupModel::HEADER_BUTTON_FOCUSED) { OmniboxPopupModel::FOCUSED_BUTTON_HEADER) {
return header_view_->header_toggle_button(); return header_view_->header_toggle_button();
} }
......
...@@ -72,7 +72,8 @@ OmniboxTabSwitchButton::~OmniboxTabSwitchButton() = default; ...@@ -72,7 +72,8 @@ OmniboxTabSwitchButton::~OmniboxTabSwitchButton() = default;
void OmniboxTabSwitchButton::StateChanged(ButtonState old_state) { void OmniboxTabSwitchButton::StateChanged(ButtonState old_state) {
if (state() == STATE_NORMAL && old_state == STATE_PRESSED) { if (state() == STATE_NORMAL && old_state == STATE_PRESSED) {
SetMouseHandler(parent()); SetMouseHandler(parent());
if (popup_contents_view_->IsButtonSelected()) if (popup_contents_view_->model()->selected_line_state() ==
OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH)
popup_contents_view_->UnselectButton(); popup_contents_view_->UnselectButton();
} }
MdTextButton::StateChanged(old_state); MdTextButton::StateChanged(old_state);
...@@ -108,7 +109,8 @@ void OmniboxTabSwitchButton::ProvideWidthHint(int parent_width) { ...@@ -108,7 +109,8 @@ void OmniboxTabSwitchButton::ProvideWidthHint(int parent_width) {
bool OmniboxTabSwitchButton::IsSelected() const { bool OmniboxTabSwitchButton::IsSelected() const {
// Is this result selected and is button selected? // Is this result selected and is button selected?
return result_view_->IsMatchSelected() && return result_view_->IsMatchSelected() &&
popup_contents_view_->IsButtonSelected(); popup_contents_view_->model()->selected_line_state() ==
OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH;
} }
int OmniboxTabSwitchButton::CalculateGoalWidth(int parent_width, int OmniboxTabSwitchButton::CalculateGoalWidth(int parent_width,
......
...@@ -815,34 +815,6 @@ void OmniboxViewViews::AnnounceFriendlySuggestionText() { ...@@ -815,34 +815,6 @@ void OmniboxViewViews::AnnounceFriendlySuggestionText() {
} }
#endif #endif
bool OmniboxViewViews::MaybeTriggerSecondaryButton(const ui::KeyEvent& event) {
// TODO(tommycli): If we have a WebUI omnibox popup, we should move the
// secondary button logic out of the View and into the OmniboxPopupModel.
if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup))
return false;
if (model()->popup_model()->selected_line_state() !=
OmniboxPopupModel::BUTTON_FOCUSED)
return false;
OmniboxPopupModel* popup_model = model()->popup_model();
if (!popup_model)
return false;
size_t selected_line = popup_model->selected_line();
if (selected_line == OmniboxPopupModel::kNoMatch)
return false;
// TODO(tommycli): https://crbug.com/1063071
// Diving into |popup_view_| was a mistake. Here's a hotfix to stop the crash,
// but the ultimate fix should be to move this logic into OmniboxPopupModel.
if (!popup_view_ || popup_view_->result_view_at(selected_line) == nullptr)
return false;
return popup_view_->result_view_at(selected_line)
->MaybeTriggerSecondaryButton(event);
}
void OmniboxViewViews::SetWindowTextAndCaretPos(const base::string16& text, void OmniboxViewViews::SetWindowTextAndCaretPos(const base::string16& text,
size_t caret_pos, size_t caret_pos,
bool update_popup, bool update_popup,
...@@ -1898,8 +1870,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield, ...@@ -1898,8 +1870,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
if (popup_model && popup_model->TriggerSelectionAction( if (popup_model && popup_model->TriggerSelectionAction(
popup_model->selection(), event.time_stamp())) { popup_model->selection(), event.time_stamp())) {
return true; return true;
} else if (MaybeTriggerSecondaryButton(event)) {
return true;
} else if ((alt && !shift) || (shift && command)) { } else if ((alt && !shift) || (shift && command)) {
model()->AcceptInput(WindowOpenDisposition::NEW_FOREGROUND_TAB, model()->AcceptInput(WindowOpenDisposition::NEW_FOREGROUND_TAB,
event.time_stamp()); event.time_stamp());
...@@ -2055,9 +2025,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield, ...@@ -2055,9 +2025,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
popup_model->selection(), event.time_stamp())) { popup_model->selection(), event.time_stamp())) {
return true; return true;
} }
if (MaybeTriggerSecondaryButton(event))
return true;
} }
break; break;
} }
......
...@@ -312,10 +312,6 @@ class OmniboxViewViews : public OmniboxView, ...@@ -312,10 +312,6 @@ class OmniboxViewViews : public OmniboxView,
// Like SelectionAtEnd(), but accounts for RTL. // Like SelectionAtEnd(), but accounts for RTL.
bool DirectionAwareSelectionAtEnd() const; bool DirectionAwareSelectionAtEnd() const;
// If the Secondary button for the current suggestion is focused, clicks it
// and returns true.
bool MaybeTriggerSecondaryButton(const ui::KeyEvent& event);
#if defined(OS_MACOSX) #if defined(OS_MACOSX)
void AnnounceFriendlySuggestionText(); void AnnounceFriendlySuggestionText();
#endif #endif
......
...@@ -171,14 +171,14 @@ void OmniboxPopupModel::SetSelection(Selection new_selection, ...@@ -171,14 +171,14 @@ void OmniboxPopupModel::SetSelection(Selection new_selection,
TemplateURLService* service = edit_model_->client()->GetTemplateURLService(); TemplateURLService* service = edit_model_->client()->GetTemplateURLService();
match.GetKeywordUIState(service, &keyword, &is_keyword_hint); match.GetKeywordUIState(service, &keyword, &is_keyword_hint);
if (selection_.state == HEADER_BUTTON_FOCUSED) { if (selection_.state == FOCUSED_BUTTON_HEADER) {
// If the new selection is a Header, the temporary text is an empty string. // If the new selection is a Header, the temporary text is an empty string.
edit_model_->OnPopupDataChanged(base::string16(), edit_model_->OnPopupDataChanged(base::string16(),
/*is_temporary_text=*/true, /*is_temporary_text=*/true,
base::string16(), keyword, is_keyword_hint, base::string16(), keyword, is_keyword_hint,
base::string16()); base::string16());
} else if (old_selection.line != selection_.line || } else if (old_selection.line != selection_.line ||
old_selection.state == HEADER_BUTTON_FOCUSED) { old_selection.state == FOCUSED_BUTTON_HEADER) {
// Otherwise, only update the edit model for line number changes, or // Otherwise, only update the edit model for line number changes, or
// when the old selection was a Header. Updating the edit model for every // when the old selection was a Header. Updating the edit model for every
// state change breaks keyword mode. // state change breaks keyword mode.
...@@ -254,10 +254,10 @@ void OmniboxPopupModel::OnResultChanged() { ...@@ -254,10 +254,10 @@ void OmniboxPopupModel::OnResultChanged() {
if (result.default_match()) { if (result.default_match()) {
Selection selection(0, selected_line_state()); Selection selection(0, selected_line_state());
// If selected line state was |BUTTON_FOCUSED| and nothing has changed, // If selected line state was |BUTTON_FOCUSED_TAB_SWITCH| and nothing has
// leave it. // changed leave it.
const bool has_focused_match = const bool has_focused_match =
selection.state == BUTTON_FOCUSED && selection.state == FOCUSED_BUTTON_TAB_SWITCH &&
result.match_at(selection.line).has_tab_match; result.match_at(selection.line).has_tab_match;
const bool has_changed = const bool has_changed =
selection.line != old_selected_line || selection.line != old_selected_line ||
...@@ -347,7 +347,7 @@ OmniboxPopupModel::GetAllAvailableSelectionsSorted(Direction direction, ...@@ -347,7 +347,7 @@ OmniboxPopupModel::GetAllAvailableSelectionsSorted(Direction direction,
} else { } else {
// Arrow keys should never reach the header controls. // Arrow keys should never reach the header controls.
if (step == kStateOrLine) if (step == kStateOrLine)
all_states.push_back(HEADER_BUTTON_FOCUSED); all_states.push_back(FOCUSED_BUTTON_HEADER);
all_states.push_back(NORMAL); all_states.push_back(NORMAL);
...@@ -368,7 +368,8 @@ OmniboxPopupModel::GetAllAvailableSelectionsSorted(Direction direction, ...@@ -368,7 +368,8 @@ OmniboxPopupModel::GetAllAvailableSelectionsSorted(Direction direction,
all_states.push_back(KEYWORD); all_states.push_back(KEYWORD);
} }
} }
all_states.push_back(BUTTON_FOCUSED); all_states.push_back(FOCUSED_BUTTON_REMOVE_SUGGESTION);
all_states.push_back(FOCUSED_BUTTON_TAB_SWITCH);
} }
} }
DCHECK(std::is_sorted(all_states.begin(), all_states.end())) DCHECK(std::is_sorted(all_states.begin(), all_states.end()))
...@@ -490,7 +491,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const { ...@@ -490,7 +491,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
const auto& match = result().match_at(selection.line); const auto& match = result().match_at(selection.line);
// Skip rows that are hidden because their header is collapsed, unless the // Skip rows that are hidden because their header is collapsed, unless the
// user is trying to focus the header itself (which is still shown). // user is trying to focus the header itself (which is still shown).
if (selection.state != HEADER_BUTTON_FOCUSED && if (selection.state != FOCUSED_BUTTON_HEADER &&
match.suggestion_group_id.has_value() && pref_service_ && match.suggestion_group_id.has_value() && pref_service_ &&
omnibox::IsSuggestionGroupIdHidden(pref_service_, omnibox::IsSuggestionGroupIdHidden(pref_service_,
match.suggestion_group_id.value())) { match.suggestion_group_id.value())) {
...@@ -498,7 +499,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const { ...@@ -498,7 +499,7 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
} }
switch (selection.state) { switch (selection.state) {
case HEADER_BUTTON_FOCUSED: { case FOCUSED_BUTTON_HEADER: {
// For the first match, if it a suggestion_group_id, then it has a header. // For the first match, if it a suggestion_group_id, then it has a header.
if (selection.line == 0) if (selection.line == 0)
return match.suggestion_group_id.has_value(); return match.suggestion_group_id.has_value();
...@@ -513,33 +514,29 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const { ...@@ -513,33 +514,29 @@ bool OmniboxPopupModel::IsControlPresentOnMatch(Selection selection) const {
return true; return true;
case KEYWORD: case KEYWORD:
return match.associated_keyword != nullptr; return match.associated_keyword != nullptr;
case BUTTON_FOCUSED: {
// TODO(orinj): Here is an opportunity to clean up the presentational
// logic that pkasting wanted to take out of AutocompleteMatch. The view
// should be driven by the model, so this is really the place to decide.
// In other words, this duplicates logic within OmniboxResultView.
// This is the proper place. OmniboxResultView should refer to here.
// Buttons are suppressed for matches with an associated keyword.
if (match.associated_keyword != nullptr)
return false;
if (match.ShouldShowTabMatchButton())
return true;
if (match.SupportsDeletion())
return true;
return false; // TODO(orinj): Here is an opportunity to clean up the presentational
} // logic that pkasting wanted to take out of AutocompleteMatch. The view
// should be driven by the model, so this is really the place to decide.
// In other words, this duplicates logic within OmniboxResultView.
// This is the proper place. OmniboxResultView should refer to here.
case FOCUSED_BUTTON_REMOVE_SUGGESTION: case FOCUSED_BUTTON_REMOVE_SUGGESTION:
// Buttons are suppressed for matches with an associated keyword. // Remove suggestion buttons are suppressed for matches with an associated
if (match.associated_keyword != nullptr) // keyword or tab match, unless dedicated button row is enabled.
return false; if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled())
return match.SupportsDeletion();
return match.SupportsDeletion(); else
return !match.associated_keyword && !match.has_tab_match &&
match.SupportsDeletion();
case FOCUSED_BUTTON_KEYWORD: case FOCUSED_BUTTON_KEYWORD:
return match.associated_keyword != nullptr; return match.associated_keyword != nullptr;
case FOCUSED_BUTTON_TAB_SWITCH: case FOCUSED_BUTTON_TAB_SWITCH:
return match.has_tab_match; // Buttons are suppressed for matches with an associated keyword, unless
// dedicated button row is enabled.
if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled())
return match.has_tab_match;
else
return match.ShouldShowTabMatchButton();
case FOCUSED_BUTTON_PEDAL: case FOCUSED_BUTTON_PEDAL:
return match.pedal != nullptr; return match.pedal != nullptr;
default: default:
...@@ -558,7 +555,7 @@ bool OmniboxPopupModel::TriggerSelectionAction(Selection selection, ...@@ -558,7 +555,7 @@ bool OmniboxPopupModel::TriggerSelectionAction(Selection selection,
auto& match = result().match_at(selection.line); auto& match = result().match_at(selection.line);
switch (selection.state) { switch (selection.state) {
case HEADER_BUTTON_FOCUSED: case FOCUSED_BUTTON_HEADER:
DCHECK(match.suggestion_group_id.has_value()); DCHECK(match.suggestion_group_id.has_value());
omnibox::ToggleSuggestionGroupIdVisibility( omnibox::ToggleSuggestionGroupIdVisibility(
pref_service_, match.suggestion_group_id.value()); pref_service_, match.suggestion_group_id.value());
...@@ -615,7 +612,7 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection( ...@@ -615,7 +612,7 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
int additional_message_id = 0; int additional_message_id = 0;
switch (selection_.state) { switch (selection_.state) {
case HEADER_BUTTON_FOCUSED: { case FOCUSED_BUTTON_HEADER: {
bool group_hidden = omnibox::IsSuggestionGroupIdHidden( bool group_hidden = omnibox::IsSuggestionGroupIdHidden(
pref_service_, match.suggestion_group_id.value()); pref_service_, match.suggestion_group_id.value());
int message_id = group_hidden ? IDS_ACC_HEADER_SHOW_SUGGESTIONS_BUTTON int message_id = group_hidden ? IDS_ACC_HEADER_SHOW_SUGGESTIONS_BUTTON
...@@ -635,13 +632,6 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection( ...@@ -635,13 +632,6 @@ base::string16 OmniboxPopupModel::GetAccessibilityLabelForCurrentSelection(
// TODO(tommycli): Investigate whether the accessibility messaging for // TODO(tommycli): Investigate whether the accessibility messaging for
// Keyword mode belongs here. // Keyword mode belongs here.
break; break;
case BUTTON_FOCUSED:
if (IsControlPresentOnMatch(Selection(line, FOCUSED_BUTTON_TAB_SWITCH))) {
additional_message_id = IDS_ACC_TAB_SWITCH_BUTTON_FOCUSED_PREFIX;
} else if (match.SupportsDeletion()) {
additional_message_id = IDS_ACC_REMOVE_SUGGESTION_FOCUSED_PREFIX;
}
break;
case FOCUSED_BUTTON_REMOVE_SUGGESTION: case FOCUSED_BUTTON_REMOVE_SUGGESTION:
additional_message_id = IDS_ACC_REMOVE_SUGGESTION_FOCUSED_PREFIX; additional_message_id = IDS_ACC_REMOVE_SUGGESTION_FOCUSED_PREFIX;
break; break;
......
...@@ -61,7 +61,7 @@ class OmniboxPopupModel { ...@@ -61,7 +61,7 @@ class OmniboxPopupModel {
enum LineState { enum LineState {
// This means the Header above this row is highlighted, and the // This means the Header above this row is highlighted, and the
// header collapse/expand button is focused. // header collapse/expand button is focused.
HEADER_BUTTON_FOCUSED = 0, FOCUSED_BUTTON_HEADER = 0,
// NORMAL means the row is focused, and Enter key navigates to the match. // NORMAL means the row is focused, and Enter key navigates to the match.
NORMAL = 1, NORMAL = 1,
...@@ -70,23 +70,22 @@ class OmniboxPopupModel { ...@@ -70,23 +70,22 @@ class OmniboxPopupModel {
// FOCUSED_BUTTON_KEYWORD state, which is only for button focus. // FOCUSED_BUTTON_KEYWORD state, which is only for button focus.
KEYWORD = 2, KEYWORD = 2,
// The single (ambiguous) button focus state is not used when button row
// is enabled. Instead, the specific FOCUSED_* states below apply.
//
// TODO(tommycli): The BUTTON_FOCUSED state is a holdover from when we only
// had one button type. At this point, it's too ambiguous, and we should
// gradually deprecate this state in favor of the FOCUSED_* states below.
// Also see Selection::IsButtonFocused().
BUTTON_FOCUSED = 3,
// FOCUSED_BUTTON_REMOVE_SUGGESTION state means the Remove Suggestion (X) // FOCUSED_BUTTON_REMOVE_SUGGESTION state means the Remove Suggestion (X)
// button is focused. Pressing enter will attempt to remove this suggestion. // button is focused. Pressing enter will attempt to remove this suggestion.
FOCUSED_BUTTON_REMOVE_SUGGESTION = 4, FOCUSED_BUTTON_REMOVE_SUGGESTION = 3,
// FOCUSED_BUTTON_KEYWORD is used when the keyword button is in focus, not
// actually in Keyword Mode. This is currently only used if deciated button
// row is enabled
FOCUSED_BUTTON_KEYWORD = 4,
// FOCUSED_BUTTON_TAB_SWITCH state means the Switch Tab button is focused.
// Pressing enter will switch to the tab match.
FOCUSED_BUTTON_TAB_SWITCH = 5,
// Button row focus states: // FOCUSED_BUTTON_PEDAL state means a Pedal button is in focus. This is
FOCUSED_BUTTON_KEYWORD = 5, // currently only used when dedicated button row and pedals are enabled.
FOCUSED_BUTTON_TAB_SWITCH = 6, FOCUSED_BUTTON_PEDAL = 6,
FOCUSED_BUTTON_PEDAL = 7,
}; };
...@@ -98,8 +97,8 @@ class OmniboxPopupModel { ...@@ -98,8 +97,8 @@ class OmniboxPopupModel {
// If the selected line has both a normal match and a keyword match, this // If the selected line has both a normal match and a keyword match, this
// determines whether the normal match (if NORMAL) or the keyword match // determines whether the normal match (if NORMAL) or the keyword match
// (if KEYWORD) is selected. Likewise, if the selected line has a normal // (if KEYWORD) is selected. Likewise, if the selected line has a normal
// match and a tab switch match, this determines whether the tab switch // match and a secondary button match, this determines whether the button
// match (if BUTTON_FOCUSED) is selected. // match (if FOCUSED_BUTTON_*) is selected.
LineState state; LineState state;
explicit Selection(size_t line, LineState state = NORMAL) explicit Selection(size_t line, LineState state = NORMAL)
......
...@@ -231,11 +231,10 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) { ...@@ -231,11 +231,10 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
// Step by states forward. // Step by states forward.
for (auto selection : { for (auto selection : {
Selection(1, OmniboxPopupModel::NORMAL), Selection(1, OmniboxPopupModel::NORMAL),
// Focused button is the Suggestion Removal button. Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
Selection(1, OmniboxPopupModel::BUTTON_FOCUSED),
Selection(2, OmniboxPopupModel::NORMAL), Selection(2, OmniboxPopupModel::NORMAL),
Selection(2, OmniboxPopupModel::KEYWORD), Selection(2, OmniboxPopupModel::KEYWORD),
Selection(3, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
Selection(3, OmniboxPopupModel::NORMAL), Selection(3, OmniboxPopupModel::NORMAL),
Selection(0, OmniboxPopupModel::NORMAL), Selection(0, OmniboxPopupModel::NORMAL),
}) { }) {
...@@ -248,14 +247,13 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) { ...@@ -248,14 +247,13 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
// should land on KEYWORD, but stepping backward should not. // should land on KEYWORD, but stepping backward should not.
for (auto selection : { for (auto selection : {
Selection(3, OmniboxPopupModel::NORMAL), Selection(3, OmniboxPopupModel::NORMAL),
Selection(3, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
Selection(2, OmniboxPopupModel::NORMAL), Selection(2, OmniboxPopupModel::NORMAL),
// Focused button is the Suggestion Removal button. Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
Selection(1, OmniboxPopupModel::BUTTON_FOCUSED),
Selection(1, OmniboxPopupModel::NORMAL), Selection(1, OmniboxPopupModel::NORMAL),
Selection(0, OmniboxPopupModel::NORMAL), Selection(0, OmniboxPopupModel::NORMAL),
Selection(3, OmniboxPopupModel::NORMAL), Selection(3, OmniboxPopupModel::NORMAL),
Selection(3, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), Selection(3, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
Selection(2, OmniboxPopupModel::NORMAL), Selection(2, OmniboxPopupModel::NORMAL),
}) { }) {
popup_model()->StepSelection(OmniboxPopupModel::kBackward, popup_model()->StepSelection(OmniboxPopupModel::kBackward,
...@@ -283,15 +281,15 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) { ...@@ -283,15 +281,15 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
model()->popup_model()->selection()); model()->popup_model()->selection());
popup_model()->StepSelection(OmniboxPopupModel::kForward, popup_model()->StepSelection(OmniboxPopupModel::kForward,
OmniboxPopupModel::kStateOrNothing); OmniboxPopupModel::kStateOrNothing);
EXPECT_EQ(Selection(1, OmniboxPopupModel::BUTTON_FOCUSED), EXPECT_EQ(Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
model()->popup_model()->selection()); model()->popup_model()->selection());
// Verify that another step forward doesn't wrap back to the NORMAL state. // Verify that another step forward doesn't wrap back to the NORMAL state.
popup_model()->StepSelection(OmniboxPopupModel::kForward, popup_model()->StepSelection(OmniboxPopupModel::kForward,
OmniboxPopupModel::kStateOrNothing); OmniboxPopupModel::kStateOrNothing);
EXPECT_EQ(Selection(1, OmniboxPopupModel::BUTTON_FOCUSED), EXPECT_EQ(Selection(1, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION),
model()->popup_model()->selection()); model()->popup_model()->selection());
// Try the kAllLines step behavior. // Try the kAllLines step behavio.
popup_model()->StepSelection(OmniboxPopupModel::kBackward, popup_model()->StepSelection(OmniboxPopupModel::kBackward,
OmniboxPopupModel::kAllLines); OmniboxPopupModel::kAllLines);
EXPECT_EQ(Selection(0, OmniboxPopupModel::NORMAL), EXPECT_EQ(Selection(0, OmniboxPopupModel::NORMAL),
...@@ -337,7 +335,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) { ...@@ -337,7 +335,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) {
// Test the kStateOrLine case, forwards and backwards. // Test the kStateOrLine case, forwards and backwards.
for (auto selection : { for (auto selection : {
Selection(1, OmniboxPopupModel::NORMAL), Selection(1, OmniboxPopupModel::NORMAL),
Selection(2, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
Selection(0, OmniboxPopupModel::NORMAL), Selection(0, OmniboxPopupModel::NORMAL),
}) { }) {
popup_model()->StepSelection(OmniboxPopupModel::kForward, popup_model()->StepSelection(OmniboxPopupModel::kForward,
...@@ -345,7 +343,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) { ...@@ -345,7 +343,7 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelectionWithHiddenGroupIds) {
EXPECT_EQ(selection, model()->popup_model()->selection()); EXPECT_EQ(selection, model()->popup_model()->selection());
} }
for (auto selection : { for (auto selection : {
Selection(2, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
Selection(1, OmniboxPopupModel::NORMAL), Selection(1, OmniboxPopupModel::NORMAL),
}) { }) {
popup_model()->StepSelection(OmniboxPopupModel::kBackward, popup_model()->StepSelection(OmniboxPopupModel::kBackward,
...@@ -424,7 +422,7 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) { ...@@ -424,7 +422,7 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) {
// string for our temporary text. // string for our temporary text.
popup_model()->StepSelection(OmniboxPopupModel::kForward, popup_model()->StepSelection(OmniboxPopupModel::kForward,
OmniboxPopupModel::kStateOrLine); OmniboxPopupModel::kStateOrLine);
EXPECT_EQ(Selection(2, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), EXPECT_EQ(Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
model()->popup_model()->selection()); model()->popup_model()->selection());
EXPECT_EQ(base::string16(), model()->text()); EXPECT_EQ(base::string16(), model()->text());
EXPECT_TRUE(model()->is_temporary_text()); EXPECT_TRUE(model()->is_temporary_text());
...@@ -442,7 +440,7 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) { ...@@ -442,7 +440,7 @@ TEST_F(OmniboxPopupModelTest, PopupInlineAutocompleteAndTemporaryText) {
// for our temporary text. // for our temporary text.
popup_model()->StepSelection(OmniboxPopupModel::kBackward, popup_model()->StepSelection(OmniboxPopupModel::kBackward,
OmniboxPopupModel::kStateOrLine); OmniboxPopupModel::kStateOrLine);
EXPECT_EQ(Selection(2, OmniboxPopupModel::HEADER_BUTTON_FOCUSED), EXPECT_EQ(Selection(2, OmniboxPopupModel::FOCUSED_BUTTON_HEADER),
model()->popup_model()->selection()); model()->popup_model()->selection());
EXPECT_EQ(base::string16(), model()->text()); EXPECT_EQ(base::string16(), model()->text());
EXPECT_TRUE(model()->is_temporary_text()); EXPECT_TRUE(model()->is_temporary_text());
...@@ -618,8 +616,9 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) { ...@@ -618,8 +616,9 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) {
// Focus the selection. // Focus the selection.
popup_model()->SetSelection(OmniboxPopupModel::Selection(0)); popup_model()->SetSelection(OmniboxPopupModel::Selection(0));
popup_model()->SetSelectedLineState(OmniboxPopupModel::BUTTON_FOCUSED); popup_model()->SetSelectedLineState(
EXPECT_EQ(OmniboxPopupModel::BUTTON_FOCUSED, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH);
EXPECT_EQ(OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH,
popup_model()->selected_line_state()); popup_model()->selected_line_state());
// Adding a match at end won't change that we selected first suggestion, so // Adding a match at end won't change that we selected first suggestion, so
...@@ -631,7 +630,7 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) { ...@@ -631,7 +630,7 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) {
result->AppendMatches(input, matches); result->AppendMatches(input, matches);
result->SortAndCull(input, nullptr); result->SortAndCull(input, nullptr);
popup_model()->OnResultChanged(); popup_model()->OnResultChanged();
EXPECT_EQ(OmniboxPopupModel::BUTTON_FOCUSED, EXPECT_EQ(OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH,
popup_model()->selected_line_state()); popup_model()->selected_line_state());
// Changing selection should change focused state. // Changing selection should change focused state.
...@@ -640,7 +639,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) { ...@@ -640,7 +639,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) {
// Adding a match at end will reset selection to first, so should change // Adding a match at end will reset selection to first, so should change
// selected line, and thus focus. // selected line, and thus focus.
popup_model()->SetSelectedLineState(OmniboxPopupModel::BUTTON_FOCUSED); popup_model()->SetSelectedLineState(
OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH);
matches[0].relevance = 999; matches[0].relevance = 999;
matches[0].contents = base::ASCIIToUTF16("match3.com"); matches[0].contents = base::ASCIIToUTF16("match3.com");
matches[0].destination_url = GURL("http://match3.com"); matches[0].destination_url = GURL("http://match3.com");
...@@ -652,7 +652,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) { ...@@ -652,7 +652,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) {
// Prepending a match won't change selection, but since URL is different, // Prepending a match won't change selection, but since URL is different,
// should clear the focus state. // should clear the focus state.
popup_model()->SetSelectedLineState(OmniboxPopupModel::BUTTON_FOCUSED); popup_model()->SetSelectedLineState(
OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH);
matches[0].relevance = 1100; matches[0].relevance = 1100;
matches[0].contents = base::ASCIIToUTF16("match4.com"); matches[0].contents = base::ASCIIToUTF16("match4.com");
matches[0].destination_url = GURL("http://match4.com"); matches[0].destination_url = GURL("http://match4.com");
...@@ -663,7 +664,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) { ...@@ -663,7 +664,8 @@ TEST_F(OmniboxPopupModelTest, TestFocusFixing) {
EXPECT_EQ(OmniboxPopupModel::NORMAL, popup_model()->selected_line_state()); EXPECT_EQ(OmniboxPopupModel::NORMAL, popup_model()->selected_line_state());
// Selecting |kNoMatch| should clear focus. // Selecting |kNoMatch| should clear focus.
popup_model()->SetSelectedLineState(OmniboxPopupModel::BUTTON_FOCUSED); popup_model()->SetSelectedLineState(
OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH);
popup_model()->SetSelection( popup_model()->SetSelection(
OmniboxPopupModel::Selection(OmniboxPopupModel::kNoMatch)); OmniboxPopupModel::Selection(OmniboxPopupModel::kNoMatch));
popup_model()->OnResultChanged(); popup_model()->OnResultChanged();
......
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