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

VERIFIED FIXED in Firefox 26

Status

()

--
major
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: aaronmt, Assigned: sriram)

Tracking

(Depends on: 1 bug, {regression, reproducible})

26 Branch
Firefox 26
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26+ verified, fennec26+)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 798599 [details]
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)
(Reporter)

Comment 1

6 years ago
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
(Assignee)

Comment 3

6 years ago
Created attachment 799106 [details] [diff] [review]
Patch
Attachment #799106 - Flags: review?(margaret.leibovic)
Comment on attachment 799106 [details] [diff] [review]
Patch

This seems broken: show it! hide it!
(Assignee)

Comment 5

6 years ago
(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?

Updated

6 years ago
Blocks: 906670
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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

6 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Friendly reminder to file the follow-up bug before we forget :)
(Assignee)

Updated

5 years ago
Depends on: 915825
status-firefox26: affected → fixed
tracking-firefox26: ? → +
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
You need to log in before you can comment on or make changes to this bug.