Aaron Train [:aaronmt] Reporter
	Description • 10 years ago

See screenshot

	:Margaret Leibovic
	Comment 1 • 10 years ago

I have also noticed this, but when I did, I think disabling search suggestions fixed it. Is this a regression?

	Teodora Vermesan (:TeoVermesan)
	Comment 2 • 10 years ago

Using an Nexus 7 (Android 5.0.1) I see the issue even on Firefox 32.0.1
I cannot install older builds than 32.0.1, because Firefox closes.

	Aaron Train [:aaronmt] Reporter
	Comment 3 • 10 years ago

Let's make this mentorable

	Aaron Train [:aaronmt] Reporter
	Comment 4 • 10 years ago

I just saw this tweet with a picture and there's an underline between results. Maybe only Lollipop is affected for some reason or another https://twitter.com/fennecnightly/status/569888592455176194 (Jellybean in the screenshot in the tweet)

Teodora can you check Jellybean/Kitkat?

	Teodora Vermesan (:TeoVermesan)
	Comment 5 • 10 years ago

I was able to reproduce the issue only on Nexus 5 (Android 5.0.1), Nexus 7 (Android 5.0.1) and Nexus 4 (Android 5.0.1).
I was not able to reproduce on Alcatel One Touch (Android 4.1.2), Nexus 4 (Android 4.4.4), Samsung S5 (Android 4.4.2), Note 3 (Android 4.4.2).

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 6 • 10 years ago

Not sure what the fix is here - it'll be a challenge. You'll probably have to dig into the search screen's ListView styles.

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 7 • 10 years ago

Since this affects 36, this is likely not a material design inheritance bug.

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 8 • 10 years ago

I looked into this issue and it turns out that there's actually a divider after the suggestions row but it's translucent (see debug screenshot with modified colors).

Android seems to not draw the dividers for disabled rows (a search engine row is disabled if there are suggestions) but still reserves the space for them. This is also the case for Android versions before 5 but was bypassed in Firefox because areAllItemsEnabled() returned true (default implementation). A recent change[1] in Android 5 now ignores the return value of areAllItemsEnabled() and always draws no dividers for disabled rows.

One fix would be to enable all the rows and handle the special case of search engine rows with suggestions in the click handling method. I'll attach a patch for this as proposal.

[1]: https://android.googlesource.com/platform/frameworks/base/+/20cc605b69a017316112929666255226c184a376%5E!/#F0

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 9 • 10 years ago

(In reply to :Sebastian Kaspari from comment #8)
> One fix would be to enable all the rows and handle the special case of
> search engine rows with suggestions in the click handling method. I'll
> attach a patch for this as proposal.

What I did not think of (and didn't see because of the debug colors) is that this would still trigger the visual clue of clicking the row. That's not better than the missing divider.

Other ideas:
* Setting the ListView background color to the same as the divider color. This way the translucent divider will have the same color as the others. (Disadvantage: Overdraw!)
* Replacing the currently white background color with the same color as the divider (Disadvantage: Slightly different Look&Feel and the row layouts probably need a white background color to be set -> Again overdraw).
* Just allowing to click the row and performing the search as if there were no suggestions (Disadvantage: Higher chance of clicking accidentally the row instead of a suggestion).

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 10 • 10 years ago

That's some awesome debugging, Sebastian! Thanks!

One more alternative:
* Overriding onDraw in the ListView to draw the divider in ourselves.

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 11 • 9 years ago

I briefly looked into drawing the dividers ourselves. Unfortunately all interesting methods in ListView are private. But yeah, there's yet another alternative: Refactoring the code to use RecyclerView instead of ListView. Using RecyclerView.ItemDecoration would give us the freedom to draw the dividers whenever and howsoever we want.

	Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]
	Comment 12 • 9 years ago

(In reply to :Sebastian Kaspari from comment #11)
> Refactoring the code to use RecyclerView instead of ListView.

https://bugzilla.mozilla.org/show_bug.cgi?id=1158303 :)

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 13 • 9 years ago

(In reply to Michael Comella (:mcomella) from comment #12)
> (In reply to :Sebastian Kaspari from comment #11)
> > Refactoring the code to use RecyclerView instead of ListView.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1158303 :)

Ah, perfect! I just created bug 1167942 for this specific ListView and linked it to both tickets (this and bug 1158303).

	BMO Automation
	Comment 14 • 4 years ago

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.