911830 - Regression: About:Home completely blank after exiting out of search results screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (09/02) - Screenshot

Currently the entirety of about:home is blank after backing out of the search suggestions screen.

Steps to Reproduce

On a new profile

i) Tap the address-bar
ii) Enter 'Cat' and accept search suggestions
iii) Hit back twice

See screenshot

Only possibly related console output

D/AbsListView(16555): onVisibilityChanged() is called, visibility : 4
D/AbsListView(16555): unregisterIRListener() is called 
D/AbsListView(16555): unregisterIRListener() is called 
D/AbsListView(16555): onDetachedFromWindow

--
Nightly (09/02)
LG Nexus 4 (Android 4.3), Samsung Galaxy SIV (Android 4.3), Samsung Galaxy Note II (Android 4.2)

Comment 1

[Aaron Train [:aaronmt]]

Closing the active about:home tab does not restore about:home to a working state. State is restored after viewing a new website. Accessing the URL bar does not redisplay about:home.

This is a major loss of functionality.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Regression from here:
https://hg.mozilla.org/integration/fx-team/rev/e355c7aae965

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 799106 [details] [diff] [review]
Patch

This seems broken: show it! hide it!

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 799106 [details] [diff] [review]
> Patch
> 
> This seems broken: show it! hide it!

Hide doesnt hide it if its about:home. Basically setVisibility() used by the regression patch is wrong. We need to clean that up. filterEditMode() is causing all the problems.

Comment 6

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > Comment on attachment 799106 [details] [diff] [review]
> > Patch
> > 
> > This seems broken: show it! hide it!
> 
> Hide doesnt hide it if its about:home. Basically setVisibility() used by the
> regression patch is wrong. We need to clean that up. filterEditMode() is
> causing all the problems.

It sounds like you have a better fix in mind... How hard would it be to just do that here, rather than land this workaround?

Comment 7

[Sriram Ramasubramanian [:sriram]]

I was thinking about it. But that too basically does the same thing by calling a show(), hide() method. Basically the regression changeset hides the visibility of about:home. This should be restored on dismissing the url-edit. That's what my patch does.

Comment 8

[:Margaret Leibovic]

Comment on attachment 799106 [details] [diff] [review]
Patch

Okay, well let's add a comment here explaining that this reverses what we're doing in filterEditingMode.

Maybe those changes should have gone in hideBrowserSearch/showBrowserSearch? Although hideBrowserSearch is called in a bunch of places, so maybe we don't want that logic everywhere. Regardless, I think this logic needs a bit more organization to feel intuitive.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

How bad is the problem the regressing code fixed? I'd consider backing out the regressing code and finding a new way to fix it. This is not a good patch.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Adding maxli:
How bad is the problem that you fixed in Bug 906670? Shall we back that out or do something that doesn't involve setVisibility()?

Comment 11

[Max Li [:maxli]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> Adding maxli:
> How bad is the problem that you fixed in Bug 906670? Shall we back that out
> or do something that doesn't involve setVisibility()?

I'd say it's relatively major (e.g. see Marco's statements in Bug 906670 comment 3). 

I'm not too sure what else you could do to fix bug 906670 besides changing the visibility. As far as I know, the only other thing you could do would be to override the onInitializeAccessibilityNodeInfo method of the HomePager to say it's invisible when it's not shown, but to actually do that you'd probably still have to have essentially the same calls as the existing patch to maintain that state. So I don't think that's really a better solution.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I would be fine with the bandaid fix in this patch, as long as we file a followup bug to fix it better!

Comment 13

[Sriram Ramasubramanian [:sriram]]

Bandaided: https://hg.mozilla.org/integration/fx-team/rev/fd09414385c4

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fd09414385c4

Comment 15

[:Margaret Leibovic]

Friendly reminder to file the follow-up bug before we forget :)