Topsites tab is showing an entry with blank content

VERIFIED FIXED in Firefox 23

Status

()

Firefox for Android
Awesomescreen
VERIFIED FIXED
4 years ago
11 months ago

People

(Reporter: tchung, Assigned: Margaret)

Tracking

Trunk
Firefox 23
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox20 affected, firefox21 affected, firefox22 affected, firefox23 verified, relnote-firefox 21+)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 732912 [details]
screenshot

See screenshot.   Tapping that empty entry goes to about:home.  Not sure why it came up blank.

Repro:
1) install 4/2 nightly build
2) launch homescreen and tap about:home
3) launch awesomescreen, and verify blank entry in the Topsites.

Expected:
- about:home should have content listed

Actual:
- white row
Long-tap hold and hit 'edit'; does it have a title?
(Assignee)

Comment 2

4 years ago
You can reproduce this by pinning an empty site. What you're seeing here is a fake pinned bookmark with no data in it. I think we shouldn't let users pin empty urls.
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → ?
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → affected
(Assignee)

Comment 3

4 years ago
Created attachment 737743 [details] [diff] [review]
patch
Attachment #737743 - Flags: review?(wjohnston)
Comment on attachment 737743 [details] [diff] [review]
patch

We should show an error the user (probably a toast?). Or alternatively, we could take this is an "unpin this" command. Also, I like brackets in java code.
Attachment #737743 - Flags: review?(wjohnston) → review-
(Assignee)

Comment 5

4 years ago
(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 737743 [details] [diff] [review]
> patch
> 
> We should show an error the user (probably a toast?). Or alternatively, we
> could take this is an "unpin this" command.

I wonder what a user is actually trying to do when submitting an emtpy serach/url. I interpreted it as cancelling, which is why I didn't add an error message or do anything to change the pinned site. Unpinning seems like the wrong thing to do if the user does think this is going to cancel their changes. 

Ian, what do you think we should do?

> Also, I like brackets in java code.

I was just copying the bracing style right above this, which was written by you :P
Flags: needinfo?(ibarlow)
(In reply to :Margaret Leibovic from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
> > Comment on attachment 737743 [details] [diff] [review]
> > patch
> > 
> > We should show an error the user (probably a toast?). Or alternatively, we
> > could take this is an "unpin this" command.
> 
> I wonder what a user is actually trying to do when submitting an emtpy
> serach/url. I interpreted it as cancelling, which is why I didn't add an
> error message or do anything to change the pinned site. Unpinning seems like
> the wrong thing to do if the user does think this is going to cancel their
> changes. 
> 
> Ian, what do you think we should do?

I agree with your comment 3, that we shouldn't even be letting people pin empty URLs to begin with.
Flags: needinfo?(ibarlow)
(Assignee)

Comment 7

4 years ago
(In reply to Ian Barlow (:ibarlow) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > (In reply to Wesley Johnston (:wesj) from comment #4)
> > > Comment on attachment 737743 [details] [diff] [review]
> > > patch
> > > 
> > > We should show an error the user (probably a toast?). Or alternatively, we
> > > could take this is an "unpin this" command.
> > 
> > I wonder what a user is actually trying to do when submitting an emtpy
> > serach/url. I interpreted it as cancelling, which is why I didn't add an
> > error message or do anything to change the pinned site. Unpinning seems like
> > the wrong thing to do if the user does think this is going to cancel their
> > changes. 
> > 
> > Ian, what do you think we should do?
> 
> I agree with your comment 3, that we shouldn't even be letting people pin
> empty URLs to begin with.

Yes, that's what my patch does, but Wes gave in an r- because he thinks that we should be showing an error message, and I was wondering what you think about that.
(Assignee)

Comment 8

4 years ago
Comment on attachment 737743 [details] [diff] [review]
patch

Re-requesting review, since ibarlow said he approved of this approach.
Attachment #737743 - Flags: review- → review?(wjohnston)
Attachment #737743 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/812da2a6d2f8
https://hg.mozilla.org/mozilla-central/rev/812da2a6d2f8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

4 years ago
status-firefox23: affected → fixed
Uplift?
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified

Updated

4 years ago
relnote-firefox: --- → ?
relnote-firefox: ? → 21+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.