Closed Bug 911830 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

26 Branch
ARM
Android
defect
Not set
major

Tracking

(firefox26+ verified, fennec26+)

VERIFIED FIXED
Firefox 26
Tracking Status
firefox26 + verified
fennec 26+ ---

People

(Reporter: aaronmt, Assigned: sriram)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files)

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)
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.
Severity: normal → major
Hardware: x86 → ARM
Attached patch PatchSplinter Review
Attachment #799106 - Flags: review?(margaret.leibovic)
Comment on attachment 799106 [details] [diff] [review]
Patch

This seems broken: show it! hide it!
(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.
(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?
Blocks: 906670
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 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.
Attachment #799106 - Flags: review?(margaret.leibovic) → review+
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.
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()?
(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.
I would be fine with the bandaid fix in this patch, as long as we file a followup bug to fix it better!
Assignee: nobody → sriram
tracking-fennec: ? → 26+
https://hg.mozilla.org/mozilla-central/rev/fd09414385c4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Friendly reminder to file the follow-up bug before we forget :)
Depends on: 915825
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.