Skip to content

Commit

Permalink
Omnibar: Pixel and Clear action regressions (#5304)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/1205782002757341/1208820359395089/f

### Description

### Steps to test this PR
Looks at this task for different scenarios:
https://app.asana.com/0/1174433894299346/1208812333027437/f

_Pixel for clearing address bar - Site_
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Swipe gesture to close keyboard 
- [x] Verify pixel `m_addressbar_focus_clear_entry_website` is fired
only once

_Pixel for clearing address bar - SERP_
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Swipe gesture to close keyboard 
- [x] Verify pixel `m_addressbar_focus_clear_entry_serp ` is fired only
once

_Pixel for clearing address bar - NTP_
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Swipe gesture to close keyboard 
- [x] Verify pixel `m_addressbar_focus_clear_entry_ntp ` is fired only
once

_Swipe gesture to close keyboard - Site_
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Swipe gesture to close keyboard 
- [x] Verify Site url is displayed

_Swipe gesture to close keyboard - SERP_
- [x] Enter a search query on SERP
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Swipe gesture to close keyboard 
- [x] Verify query is displayed

_Manually close keyboard  + Scroll- Site_
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Manually close keyboard
- [x] Scroll down the site
- [x] Verify Site url is displayed

_Manually close keyboard  + Scroll- SERP_
- [x] Enter a search query on SERP
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Manually close keyboard
- [x] Scroll down the site
- [x] Verify query is displayed

_Close keyboard - Site_
- [x] Ensure 3 button navigation is enabled (via device Settings)
- [x] Navigate to a site
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Close keyboard (press back)
- [x] Verify Site url is displayed

_Close keyboard - SERP_
- [x] Ensure 3 button navigation is enabled (via device Settings)
- [x] Enter a search query on SERP
- [x] Tap into address bar
- [x] X to delete entry (don’t type anything new)
- [x] Close keyboard (press back)
- [x] Verify query is displayed
  • Loading branch information
malmstein authored Dec 10, 2024
1 parent 747c4af commit c36dc4c
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -955,8 +955,10 @@ class BrowserTabFragment :
}

private fun onOmnibarClearTextButtonPressed() {
viewModel.onClearOmnibarTextInput()
omnibar.setText("")
if (!changeOmnibarPositionFeature.refactor().isEnabled()) {
viewModel.onClearOmnibarTextInput()
omnibar.setText("")
}
}

private fun configureCustomTab() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,11 +296,15 @@ class OmnibarLayout @JvmOverloads constructor(

omnibarTextInput.replaceTextChangedListener(
object : TextChangedWatcher() {
var clearQuery = false
var deleteLastCharacter = false
override fun afterTextChanged(editable: Editable) {
if (isAttachedToWindow) {
viewModel.onInputStateChanged(
omnibarTextInput.text.toString(),
omnibarTextInput.hasFocus(),
clearQuery,
deleteLastCharacter,
)
}
omnibarTextListener?.onOmnibarTextChanged(
Expand All @@ -310,6 +314,12 @@ class OmnibarLayout @JvmOverloads constructor(
),
)
}

override fun beforeTextChanged(s: CharSequence, start: Int, count: Int, after: Int) {
Timber.d("Omnibar: $count characters beginning at $start are about to be replaced by new text with length $after")
clearQuery = start == 0 && after == 0
deleteLastCharacter = count == 1 && clearQuery
}
},
)

Expand Down Expand Up @@ -445,7 +455,6 @@ class OmnibarLayout @JvmOverloads constructor(
}

private fun renderBrowserMode(viewState: ViewState) {
Timber.d("Omnibar: render browserMode $viewState")
renderOutline(viewState.hasFocus)
if (viewState.updateOmnibarText) {
omnibarTextInput.setText(viewState.omnibarText)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import androidx.lifecycle.viewModelScope
import com.duckduckgo.anvil.annotations.ContributesViewModel
import com.duckduckgo.app.browser.DuckDuckGoUrlDetector
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.Browser
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.CustomTab
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.Error
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.NewTab
Expand Down Expand Up @@ -90,6 +91,7 @@ class OmnibarLayoutViewModel @Inject constructor(
val leadingIconState: LeadingIconState = LeadingIconState.SEARCH,
val privacyShield: PrivacyShield = PrivacyShield.UNKNOWN,
val hasFocus: Boolean = false,
val query: String = "",
val omnibarText: String = "",
val url: String = "",
val expanded: Boolean = false,
Expand Down Expand Up @@ -171,6 +173,20 @@ class OmnibarLayoutViewModel @Inject constructor(
}
} else {
_viewState.update {
val shouldUpdateOmnibarText = it.viewMode is Browser
Timber.d("Omnibar: lost focus in Browser mode $shouldUpdateOmnibarText")
val omnibarText = if (shouldUpdateOmnibarText) {
if (duckDuckGoUrlDetector.isDuckDuckGoQueryUrl(it.url)) {
Timber.d("Omnibar: is DDG url, showing query ${it.query}")
it.query
} else {
Timber.d("Omnibar: is url, showing URL ${it.url}")
it.url
}
} else {
Timber.d("Omnibar: not browser mode, not changing omnibar text")
it.omnibarText
}
it.copy(
hasFocus = false,
expanded = false,
Expand All @@ -187,6 +203,8 @@ class OmnibarLayoutViewModel @Inject constructor(
urlLoaded = _viewState.value.url,
),
shouldMoveCaretToStart = true,
updateOmnibarText = shouldUpdateOmnibarText,
omnibarText = omnibarText,
)
}
}
Expand Down Expand Up @@ -386,13 +404,28 @@ class OmnibarLayoutViewModel @Inject constructor(
fun onInputStateChanged(
query: String,
hasFocus: Boolean,
clearQuery: Boolean,
deleteLastCharacter: Boolean,
) {
Timber.d("Omnibar: onInputStateChanged")
val showClearButton = hasFocus && query.isNotBlank()
val showControls = !hasFocus || query.isBlank()

Timber.d("Omnibar: onInputStateChanged query $query hasFocus $hasFocus clearQuery $clearQuery deleteLastCharacter $deleteLastCharacter")

_viewState.update {
val updatedQuery = if (deleteLastCharacter) {
Timber.d("Omnibar: deleting last character, old query ${it.query} also deleted")
it.url
} else if (clearQuery) {
Timber.d("Omnibar: clearing old query ${it.query}, we keep it as reference")
it.query
} else {
Timber.d("Omnibar: not clearing or deleting old query ${it.query}, updating query to $query")
query
}

it.copy(
query = updatedQuery,
omnibarText = query,
updateOmnibarText = false,
hasFocus = hasFocus,
Expand Down Expand Up @@ -501,6 +534,7 @@ class OmnibarLayoutViewModel @Inject constructor(
}

fun onUserTouchedOmnibarTextInput(touchAction: Int) {
Timber.d("Omnibar: onUserTouchedOmnibarTextInput")
if (touchAction == ACTION_UP) {
firePixelBasedOnCurrentUrl(
AppPixelName.ADDRESS_BAR_NEW_TAB_PAGE_CLICKED,
Expand All @@ -517,6 +551,12 @@ class OmnibarLayoutViewModel @Inject constructor(
AppPixelName.ADDRESS_BAR_SERP_CANCELLED,
AppPixelName.ADDRESS_BAR_WEBSITE_CANCELLED,
)
_viewState.update {
it.copy(
omnibarText = it.url,
updateOmnibarText = true,
)
}
}

fun onEnterKeyPressed() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,9 @@ class OmnibarLayoutViewModelTest {
fun whenInputStateChangedAndQueryEmptyThenViewStateCorrect() = runTest {
val query = ""
val hasFocus = true
testee.onInputStateChanged(query, hasFocus)
val clearQuery = true
val deleteLastCharacter = false
testee.onInputStateChanged(query, hasFocus, clearQuery, deleteLastCharacter)

testee.viewState.test {
val viewState = awaitItem()
Expand All @@ -645,7 +647,9 @@ class OmnibarLayoutViewModelTest {
fun whenInputStateChangedAndQueryNotEmptyThenViewStateCorrect() = runTest {
val query = "query"
val hasFocus = true
testee.onInputStateChanged(query, hasFocus)
val clearQuery = true
val deleteLastCharacter = false
testee.onInputStateChanged(query, hasFocus, clearQuery, deleteLastCharacter)

testee.viewState.test {
val viewState = awaitItem()
Expand Down Expand Up @@ -815,6 +819,92 @@ class OmnibarLayoutViewModelTest {
}
}

@Test
fun whenOmnibarTextClearedAndBackPressedThenUrlIsShown() = runTest {
givenSiteLoaded(RANDOM_URL)
testee.onClearTextButtonPressed()
testee.onBackKeyPressed()

testee.viewState.test {
val viewState = awaitItem()
assertTrue(viewState.omnibarText == RANDOM_URL)
}
}

@Test
fun whenHidingKeyboardAfterClearingInputWhileInSiteThenURLisShown() = runTest {
givenSiteLoaded(RANDOM_URL)
testee.onClearTextButtonPressed()
val hasFocus = true
val clearQuery = true
val deleteLastCharacter = false
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
testee.onOmnibarFocusChanged(false, "")
testee.onInputStateChanged(RANDOM_URL, false, false, false)

testee.viewState.test {
val viewState = awaitItem()
assertTrue(viewState.omnibarText == RANDOM_URL)
}
}

@Test
fun whenHidingKeyboardAfterClearingInputWhileInSERPThenURLisShown() = runTest {
givenSiteLoaded(SERP_URL)

val hasFocus = true
val clearQuery = true
val deleteLastCharacter = false

testee.onClearTextButtonPressed()
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
testee.onOmnibarFocusChanged(false, "")
testee.onInputStateChanged(SERP_URL, false, false, false)

testee.viewState.test {
val viewState = awaitItem()
assertTrue(viewState.omnibarText == SERP_URL)
}
}

@Test
fun whenClosingKeyboardAfterDeletingLastCharacterFromOmnibaWhileInSERPThenURLisShown() = runTest {
givenSiteLoaded(SERP_URL)

val hasFocus = true
val clearQuery = true
val deleteLastCharacter = true

testee.onClearTextButtonPressed()
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
testee.onOmnibarFocusChanged(false, "")
testee.onInputStateChanged(SERP_URL, false, false, deleteLastCharacter)

testee.viewState.test {
val viewState = awaitItem()
assertTrue(viewState.omnibarText == SERP_URL)
}
}

@Test
fun whenClosingKeyboardAfterDeletingLastCharacterFromOmnibaWhileInSitehenURLisShown() = runTest {
givenSiteLoaded(RANDOM_URL)

val hasFocus = true
val clearQuery = true
val deleteLastCharacter = true

testee.onClearTextButtonPressed()
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
testee.onOmnibarFocusChanged(false, "")
testee.onInputStateChanged(RANDOM_URL, false, false, deleteLastCharacter)

testee.viewState.test {
val viewState = awaitItem()
assertTrue(viewState.omnibarText == RANDOM_URL)
}
}

private fun givenSiteLoaded(loadedUrl: String) {
testee.onViewModeChanged(ViewMode.Browser(loadedUrl))
testee.onExternalStateChange(
Expand Down

0 comments on commit c36dc4c

Please sign in to comment.