Skip to content

Commit c36dc4c

Browse files
malmsteinmalmstein
authored
Omnibar: Pixel and Clear action regressions (#5304)
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
1 parent 747c4af commit c36dc4c

File tree

4 files changed

+147
-6
lines changed

4 files changed

+147
-6
lines changed

‎app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -955,8 +955,10 @@ class BrowserTabFragment :
955955
}
956956

957957
private fun onOmnibarClearTextButtonPressed() {
958-
viewModel.onClearOmnibarTextInput()
959-
omnibar.setText("")
958+
if (!changeOmnibarPositionFeature.refactor().isEnabled()) {
959+
viewModel.onClearOmnibarTextInput()
960+
omnibar.setText("")
961+
}
960962
}
961963

962964
private fun configureCustomTab() {

‎app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,11 +296,15 @@ class OmnibarLayout @JvmOverloads constructor(
296296

297297
omnibarTextInput.replaceTextChangedListener(
298298
object : TextChangedWatcher() {
299+
var clearQuery = false
300+
var deleteLastCharacter = false
299301
override fun afterTextChanged(editable: Editable) {
300302
if (isAttachedToWindow) {
301303
viewModel.onInputStateChanged(
302304
omnibarTextInput.text.toString(),
303305
omnibarTextInput.hasFocus(),
306+
clearQuery,
307+
deleteLastCharacter,
304308
)
305309
}
306310
omnibarTextListener?.onOmnibarTextChanged(
@@ -310,6 +314,12 @@ class OmnibarLayout @JvmOverloads constructor(
310314
),
311315
)
312316
}
317+
318+
override fun beforeTextChanged(s: CharSequence, start: Int, count: Int, after: Int) {
319+
Timber.d("Omnibar: $count characters beginning at $start are about to be replaced by new text with length $after")
320+
clearQuery = start == 0 && after == 0
321+
deleteLastCharacter = count == 1 && clearQuery
322+
}
313323
},
314324
)
315325

@@ -445,7 +455,6 @@ class OmnibarLayout @JvmOverloads constructor(
445455
}
446456

447457
private fun renderBrowserMode(viewState: ViewState) {
448-
Timber.d("Omnibar: render browserMode $viewState")
449458
renderOutline(viewState.hasFocus)
450459
if (viewState.updateOmnibarText) {
451460
omnibarTextInput.setText(viewState.omnibarText)

‎app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import androidx.lifecycle.viewModelScope
2323
import com.duckduckgo.anvil.annotations.ContributesViewModel
2424
import com.duckduckgo.app.browser.DuckDuckGoUrlDetector
2525
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode
26+
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.Browser
2627
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.CustomTab
2728
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.Error
2829
import com.duckduckgo.app.browser.omnibar.Omnibar.ViewMode.NewTab
@@ -90,6 +91,7 @@ class OmnibarLayoutViewModel @Inject constructor(
9091
val leadingIconState: LeadingIconState = LeadingIconState.SEARCH,
9192
val privacyShield: PrivacyShield = PrivacyShield.UNKNOWN,
9293
val hasFocus: Boolean = false,
94+
val query: String = "",
9395
val omnibarText: String = "",
9496
val url: String = "",
9597
val expanded: Boolean = false,
@@ -171,6 +173,20 @@ class OmnibarLayoutViewModel @Inject constructor(
171173
}
172174
} else {
173175
_viewState.update {
176+
val shouldUpdateOmnibarText = it.viewMode is Browser
177+
Timber.d("Omnibar: lost focus in Browser mode $shouldUpdateOmnibarText")
178+
val omnibarText = if (shouldUpdateOmnibarText) {
179+
if (duckDuckGoUrlDetector.isDuckDuckGoQueryUrl(it.url)) {
180+
Timber.d("Omnibar: is DDG url, showing query ${it.query}")
181+
it.query
182+
} else {
183+
Timber.d("Omnibar: is url, showing URL ${it.url}")
184+
it.url
185+
}
186+
} else {
187+
Timber.d("Omnibar: not browser mode, not changing omnibar text")
188+
it.omnibarText
189+
}
174190
it.copy(
175191
hasFocus = false,
176192
expanded = false,
@@ -187,6 +203,8 @@ class OmnibarLayoutViewModel @Inject constructor(
187203
urlLoaded = _viewState.value.url,
188204
),
189205
shouldMoveCaretToStart = true,
206+
updateOmnibarText = shouldUpdateOmnibarText,
207+
omnibarText = omnibarText,
190208
)
191209
}
192210
}
@@ -386,13 +404,28 @@ class OmnibarLayoutViewModel @Inject constructor(
386404
fun onInputStateChanged(
387405
query: String,
388406
hasFocus: Boolean,
407+
clearQuery: Boolean,
408+
deleteLastCharacter: Boolean,
389409
) {
390-
Timber.d("Omnibar: onInputStateChanged")
391410
val showClearButton = hasFocus && query.isNotBlank()
392411
val showControls = !hasFocus || query.isBlank()
393412

413+
Timber.d("Omnibar: onInputStateChanged query $query hasFocus $hasFocus clearQuery $clearQuery deleteLastCharacter $deleteLastCharacter")
414+
394415
_viewState.update {
416+
val updatedQuery = if (deleteLastCharacter) {
417+
Timber.d("Omnibar: deleting last character, old query ${it.query} also deleted")
418+
it.url
419+
} else if (clearQuery) {
420+
Timber.d("Omnibar: clearing old query ${it.query}, we keep it as reference")
421+
it.query
422+
} else {
423+
Timber.d("Omnibar: not clearing or deleting old query ${it.query}, updating query to $query")
424+
query
425+
}
426+
395427
it.copy(
428+
query = updatedQuery,
396429
omnibarText = query,
397430
updateOmnibarText = false,
398431
hasFocus = hasFocus,
@@ -501,6 +534,7 @@ class OmnibarLayoutViewModel @Inject constructor(
501534
}
502535

503536
fun onUserTouchedOmnibarTextInput(touchAction: Int) {
537+
Timber.d("Omnibar: onUserTouchedOmnibarTextInput")
504538
if (touchAction == ACTION_UP) {
505539
firePixelBasedOnCurrentUrl(
506540
AppPixelName.ADDRESS_BAR_NEW_TAB_PAGE_CLICKED,
@@ -517,6 +551,12 @@ class OmnibarLayoutViewModel @Inject constructor(
517551
AppPixelName.ADDRESS_BAR_SERP_CANCELLED,
518552
AppPixelName.ADDRESS_BAR_WEBSITE_CANCELLED,
519553
)
554+
_viewState.update {
555+
it.copy(
556+
omnibarText = it.url,
557+
updateOmnibarText = true,
558+
)
559+
}
520560
}
521561

522562
fun onEnterKeyPressed() {

‎app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt‎

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -627,7 +627,9 @@ class OmnibarLayoutViewModelTest {
627627
fun whenInputStateChangedAndQueryEmptyThenViewStateCorrect() = runTest {
628628
val query = ""
629629
val hasFocus = true
630-
testee.onInputStateChanged(query, hasFocus)
630+
val clearQuery = true
631+
val deleteLastCharacter = false
632+
testee.onInputStateChanged(query, hasFocus, clearQuery, deleteLastCharacter)
631633

632634
testee.viewState.test {
633635
val viewState = awaitItem()
@@ -645,7 +647,9 @@ class OmnibarLayoutViewModelTest {
645647
fun whenInputStateChangedAndQueryNotEmptyThenViewStateCorrect() = runTest {
646648
val query = "query"
647649
val hasFocus = true
648-
testee.onInputStateChanged(query, hasFocus)
650+
val clearQuery = true
651+
val deleteLastCharacter = false
652+
testee.onInputStateChanged(query, hasFocus, clearQuery, deleteLastCharacter)
649653

650654
testee.viewState.test {
651655
val viewState = awaitItem()
@@ -815,6 +819,92 @@ class OmnibarLayoutViewModelTest {
815819
}
816820
}
817821

822+
@Test
823+
fun whenOmnibarTextClearedAndBackPressedThenUrlIsShown() = runTest {
824+
givenSiteLoaded(RANDOM_URL)
825+
testee.onClearTextButtonPressed()
826+
testee.onBackKeyPressed()
827+
828+
testee.viewState.test {
829+
val viewState = awaitItem()
830+
assertTrue(viewState.omnibarText == RANDOM_URL)
831+
}
832+
}
833+
834+
@Test
835+
fun whenHidingKeyboardAfterClearingInputWhileInSiteThenURLisShown() = runTest {
836+
givenSiteLoaded(RANDOM_URL)
837+
testee.onClearTextButtonPressed()
838+
val hasFocus = true
839+
val clearQuery = true
840+
val deleteLastCharacter = false
841+
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
842+
testee.onOmnibarFocusChanged(false, "")
843+
testee.onInputStateChanged(RANDOM_URL, false, false, false)
844+
845+
testee.viewState.test {
846+
val viewState = awaitItem()
847+
assertTrue(viewState.omnibarText == RANDOM_URL)
848+
}
849+
}
850+
851+
@Test
852+
fun whenHidingKeyboardAfterClearingInputWhileInSERPThenURLisShown() = runTest {
853+
givenSiteLoaded(SERP_URL)
854+
855+
val hasFocus = true
856+
val clearQuery = true
857+
val deleteLastCharacter = false
858+
859+
testee.onClearTextButtonPressed()
860+
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
861+
testee.onOmnibarFocusChanged(false, "")
862+
testee.onInputStateChanged(SERP_URL, false, false, false)
863+
864+
testee.viewState.test {
865+
val viewState = awaitItem()
866+
assertTrue(viewState.omnibarText == SERP_URL)
867+
}
868+
}
869+
870+
@Test
871+
fun whenClosingKeyboardAfterDeletingLastCharacterFromOmnibaWhileInSERPThenURLisShown() = runTest {
872+
givenSiteLoaded(SERP_URL)
873+
874+
val hasFocus = true
875+
val clearQuery = true
876+
val deleteLastCharacter = true
877+
878+
testee.onClearTextButtonPressed()
879+
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
880+
testee.onOmnibarFocusChanged(false, "")
881+
testee.onInputStateChanged(SERP_URL, false, false, deleteLastCharacter)
882+
883+
testee.viewState.test {
884+
val viewState = awaitItem()
885+
assertTrue(viewState.omnibarText == SERP_URL)
886+
}
887+
}
888+
889+
@Test
890+
fun whenClosingKeyboardAfterDeletingLastCharacterFromOmnibaWhileInSitehenURLisShown() = runTest {
891+
givenSiteLoaded(RANDOM_URL)
892+
893+
val hasFocus = true
894+
val clearQuery = true
895+
val deleteLastCharacter = true
896+
897+
testee.onClearTextButtonPressed()
898+
testee.onInputStateChanged("", hasFocus, clearQuery, deleteLastCharacter)
899+
testee.onOmnibarFocusChanged(false, "")
900+
testee.onInputStateChanged(RANDOM_URL, false, false, deleteLastCharacter)
901+
902+
testee.viewState.test {
903+
val viewState = awaitItem()
904+
assertTrue(viewState.omnibarText == RANDOM_URL)
905+
}
906+
}
907+
818908
private fun givenSiteLoaded(loadedUrl: String) {
819909
testee.onViewModeChanged(ViewMode.Browser(loadedUrl))
820910
testee.onExternalStateChange(

0 commit comments

Comments
 (0)