Last Comment Bug 857661 - Topsites tab is showing an entry with blank content
: Topsites tab is showing an entry with blank content
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Awesomescreen (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 23
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-03 10:45 PDT by Tony Chung [:tchung]
Modified: 2013-10-18 16:19 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
verified
21+


Attachments
screenshot (80.76 KB, image/png)
2013-04-03 10:45 PDT, Tony Chung [:tchung]
no flags Details
patch (1.37 KB, patch)
2013-04-15 16:25 PDT, :Margaret Leibovic
wjohnston2000: review+
Details | Diff | Review

Description Tony Chung [:tchung] 2013-04-03 10:45:20 PDT
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
Comment 1 Aaron Train [:aaronmt] 2013-04-03 11:06:52 PDT
Long-tap hold and hit 'edit'; does it have a title?
Comment 2 :Margaret Leibovic 2013-04-15 16:16:16 PDT
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.
Comment 3 :Margaret Leibovic 2013-04-15 16:25:13 PDT
Created attachment 737743 [details] [diff] [review]
patch
Comment 4 Wesley Johnston (:wesj) 2013-04-15 16:32:25 PDT
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.
Comment 5 :Margaret Leibovic 2013-04-16 12:04:34 PDT
(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
Comment 6 Ian Barlow (:ibarlow) 2013-04-17 06:56:36 PDT
(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.
Comment 7 :Margaret Leibovic 2013-04-17 08:57:25 PDT
(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.
Comment 8 :Margaret Leibovic 2013-04-23 11:24:52 PDT
Comment on attachment 737743 [details] [diff] [review]
patch

Re-requesting review, since ibarlow said he approved of this approach.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-04-24 05:23:25 PDT
https://hg.mozilla.org/mozilla-central/rev/812da2a6d2f8
Comment 11 Aaron Train [:aaronmt] 2013-04-25 07:06:46 PDT
Uplift?

Note You need to log in before you can comment on or make changes to this bug.