[omnibox] Fix omnibox popup visibility condition for active drafts [chromium/src : main]
Paul Adedeji (Gerrit)
Paul Adedeji voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Marlon Facey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marlon Facey (Gerrit)
Marlon Facey voted and added 1 comment![]( "Open in Gerrit")
- Paul Adedeji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[omnibox] Fix omnibox popup visibility condition for active drafts
Ensure the omnibox popup state is set to full when an active draft is
present, regardless of focus status. Previously, the popup state
condition inadvertently required focus even when user input was in
progress with user text.
- M chrome/browser/ui/views/omnibox/omnibox_popup_view_full_webui.cc
Code-Review: +1 by Marlon Facey
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
manuk hovanesian (Gerrit)
manuk hovanesian added 1 comment![]( "Open in Gerrit")
// The popup must be visible (`OmniboxPopupState::kFull`) if there is an
// active draft or if the omnibox should have visible focus.
const bool has_non_empty_draft =
state->model_state.user_input_in_progress &&
!state->model_state.user_text.empty();
target_popup_state = (has_non_empty_draft || should_focus_popup)
? OmniboxPopupState::kFull
: OmniboxPopupState::kNone;I'm not sure what this block of code is supposed to do.
Based on the CL description and the comment in the code, it seems like the intent was that if you switch to a tab with user edited input, the popup should automatically open?
if that was the intent, then it doesn't seem to occur at tip of tree nor canary.
it also doesn't occur in views omnibox, so i'm unsure if we should even try to fix it but rather remove the code altogether?
i'm possibly just misunderstanding the intent of this CL.
printf shows `target_popup_state` is being set to kFull (sometimes incorrectly when there is no user drafted edit), but it *almost* never actually opens the popup.
so there seems to be 3 bugs,
a) target_popup_state is being set to kFull even when there is no edit, e.g switching to an NTP with an unmodified omnibox.
b) even when target_popup_state is set to kFull, the popup almost never opens (like 95% of the time)
c) why are we even trying to open the popup since the views omnibox doesn't seem to do so.
Related details
- Paul Adedeji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |