Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Hide autofill popup only when webcontent is focused #170

Merged
merged 1 commit into from
Mar 21, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Mar 18, 2017

There will be a follow-up cleanup in browser-laptop and I will mark
issue fixed there.

Address fcaecc2

Auditors: @bridiver, @bbondy, @bsclifton

There will be a follow-up cleanup in browser-laptop and I will mark
issue fixed there.

Address fcaecc2

Auditors: @bridiver, @bbondy, @bsclifton
@darkdh darkdh self-assigned this Mar 18, 2017
@darkdh darkdh requested review from bridiver, bbondy and bsclifton March 18, 2017 07:18
darkdh added a commit to brave/browser-laptop that referenced this pull request Mar 18, 2017
fix #7763
fix #7768

Auditors: @bridiver, @bbondy, @bsclifton

Test Plan: Covered by automatic test
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

darkdh added a commit to brave/browser-laptop that referenced this pull request Mar 18, 2017
requires brave/muon#170

fix #7763
fix #7768

Auditors: @bridiver, @bbondy, @bsclifton

Test Plan: Covered by automatic test
@bbondy bbondy merged commit 3023797 into master Mar 21, 2017
bsclifton added a commit to brave/browser-laptop that referenced this pull request Mar 21, 2017
@bsclifton bsclifton deleted the hide-autofill-popup branch March 21, 2017 20:27
@@ -196,7 +196,7 @@ void AtomAutofillClient::UpdateAutofillPopupDataListValues(
}

void AtomAutofillClient::HideAutofillPopup() {
if (api_web_contents_) {
if (api_web_contents_ && api_web_contents_->IsFocused()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the best way to handle this. I think the root of the problem is that we don't check the tabId before clearing the contextMenuDetail state in browser-laptop WINDOW_AUTOFILL_POPUP_HIDDEN, but if we are going to keep something like this I think it should track the current popup_visible state by setting it along with the calls to OnPopupShown and OnPopupHidden

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in brave/browser-laptop@396cdbc
and we will keep api_web_contents_->IsFocused() for dismiss popup when scrolling page

darkdh added a commit to brave/browser-laptop that referenced this pull request Mar 25, 2017
follow-up of brave/muon#170 (comment)

Auditors: @bridiver, @bbondy

Test Plan: Covered by automatic test
bsclifton pushed a commit to brave/browser-laptop that referenced this pull request Mar 25, 2017
follow-up of brave/muon#170 (comment)

Auditors: @bridiver, @bbondy

Test Plan: Covered by automatic test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants