Quick search bar hides behind the VKB when choosing not to display search suggestions

RESOLVED FIXED in Firefox 43

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: TeoVermesan, Assigned: Sergej Kravcenko)

Tracking

({regression})

Trunk
Firefox 44
ARM
Android
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox42 affected, firefox43+ verified, firefox44+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8665307 [details]
Screenshot_2015-09-24-10-34-03.png

Tested using:
Device: LG G4 (Android 5.1);

Steps to reproduce:
1. Tap something in the URL Bar
2. Choose "No" for "Would you like to turn on search suggestions?"

Expected result:
- no additional suggestions are displayed 
- quick search bar is still displayed above the VKB

Actual result:
- Quick search bar disappears 
- VKB remains displayed
- Tapping outside to dismiss it, quick search bar is displayed on the middle of the screen
- Please take a look at the following video: https://www.youtube.com/watch?v=KQXVOr8a_cU&feature=youtu.be

Note:
- if you open a new tab and type in the URL Bar, quick search bar is not displayed above the VKB on 44 Nightly and 43 Aurora, but is displayed on 42Beta 1 and 41
(Assignee)

Comment 1

3 years ago
Created attachment 8665482 [details] [diff] [review]
always reset height

Always reset height, not only on enable.
Attachment #8665482 - Flags: review?(michael.l.comella)
Assignee: nobody → sergej
Comment on attachment 8665482 [details] [diff] [review]
always reset height

Review of attachment 8665482 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Works for me!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5f371b2560f

::: mobile/android/base/home/BrowserSearch.java
@@ +729,5 @@
>                          mSuggestionsOptInPrompt = null;
>  
> +                        // Reset the view height
> +                        mView.getLayoutParams().height = LayoutParams.MATCH_PARENT;
> +                        

nit: remove excess whitespace.
Attachment #8665482 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 3

3 years ago
Actually there must be a comment. Two blocks are not related.
(Assignee)

Comment 4

3 years ago
Created attachment 8667627 [details] [diff] [review]
always reset height

Added comment
Attachment #8665482 - Attachment is obsolete: true
Attachment #8667627 - Flags: review?(michael.l.comella)
Comment on attachment 8667627 [details] [diff] [review]
always reset height

Review of attachment 8667627 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me. Just be careful about adding excessive comments – if the code speaks for itself, there is no need to add a comment. If the code doesn't speak for itself, consider rewriting it to do so. :) It's a little harder in this case because of the pre-existing code.
Attachment #8667627 - Flags: review?(michael.l.comella) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d1baecb503a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44

Comment 9

3 years ago
Verified as fixed in build 44.0a1 2015-10-09;
Device: Motorola Razr (Android 4.4.4).
status-firefox44: fixed → verified

Updated

3 years ago
status-firefox43: --- → affected
Sergej, can you flag this for uplift to 43 (aurora)? You can click details next to the uploaded patch, add approval-aurora = ? (or whatever the name is), and complete in the auto-filled comment.
Flags: needinfo?(sergej)
(Assignee)

Comment 11

3 years ago
Do I need to fill all info fields (Risks, Bug numbers and etc)? I don't know what to write there.
Flags: needinfo?(sergej) → needinfo?(michael.l.comella)
(In reply to Sergej Kravcenko from comment #11)
> Do I need to fill all info fields (Risks, Bug numbers and etc)? I don't know
> what to write there.

Add a comment on the fields you're not sure how to fill in, NI me, and I'll fill those in.
Flags: needinfo?(michael.l.comella) → needinfo?(sergej)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8667627 [details] [diff] [review]
always reset height

Approval Request Comment
[Feature/regressing bug #]: 1207961
[User impact if declined]: Quick search bar will not be visible when user decline search suggestions
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Flags: needinfo?(sergej) → needinfo?(michael.l.comella)
Attachment #8667627 - Flags: approval-mozilla-aurora?
Tracking since this looks like a regression from the search suggestions feature.  
42 should be unaffected since search suggestions is aimed for 43.
status-firefox42: --- → unaffected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
Keywords: regression
(In reply to Sergej Kravcenko from comment #13)
> [Describe test coverage new/current, TreeHerder]: Sergej & I tested locally, no tests
> [Risks and why]: Low, we choose to do a working piece of code all the time, rather than just under a small subset of conditions. We branch to avoid doing excess work so it'd be fine to do it all the time.

> [String/UUID change made/needed]: None
Flags: needinfo?(michael.l.comella)
Comment on attachment 8667627 [details] [diff] [review]
always reset height

Low risk fix, local testing, ok on m-c. Please uplift to aurora.
Attachment #8667627 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Teo can you verify this on aurora once it lands? Thanks very much!
Flags: qe-verify+
Flags: needinfo?(teodora.vermesan)
(Reporter)

Comment 19

3 years ago
After choosing "No" for "Would you like to turn on search suggestions?" the quick search bar is still displayed above the VKB, so:
Verified as fixed using:
Device: Nexus 6 (Android 5.1.1)
Build: Firefox for Android 43.0a2 (2015-10-14)
status-firefox43: fixed → verified
Flags: needinfo?(teodora.vermesan)
(Reporter)

Comment 20

3 years ago
This is also affected on Firefox for Android 42 Beta 8
status-firefox42: unaffected → affected
You need to log in before you can comment on or make changes to this bug.