1031084 - "Save to homescreen" is still displayed even though this app is already on the homescreen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Johan Lorenzo [:jlorenzo]]

STR
1. Open a collection
2. Add an e.me app to the homescreen
3. Long press this e.me again

Actual result
Context menu is displayed with that entry: "Save to homescreen"

Expected result
This entry should be hidden. Right now, tapping it leads you to the Edit bookmark page. If you modify it, changes are made on the homescreen but not inside the Collection, which is confusing.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Confusing UX.

Comment 2

[Jason Smith [:jsmith]]

I don't understand the bug here. Can you rephrase and/or provide a video?

Comment 3

[Johan Lorenzo [:jlorenzo]]

Video link: http://mzl.la/1mB9rST

Comment 4

[Johan Lorenzo [:jlorenzo]]

STR: 
1. Enter a Smart Collection
2. Long press an app suggestion from e.me
3. Save this app to the homescreen as bookmark
4. Back in the Smart Collection, long press the same app suggestion

Result: You'll see the same context menu as in step 2. This context menu contains an entry called "Save to homescreen" which is weird because the app has just been saved to the homescreen.

Moreover, if you tap "Save to homescreen" it actually leads you to the "Edit bookmark" page. And if you edit it (by remove some characters), you'll only edit the app on the homescreen and not the one you pressed in step 4.

Comment 5

[Kevin Grandon :kgrandon]

FWIW - we do this everywhere, and I'm assuming it's been this way for a long time. In the browser, or search app the link is always "Add to home screen", but if you've already added it, you're just taken to the edit screen.

It's certainly not the best UX, but I don't think we'd block on it given that it's been this way forever.

Comment 6

[Johan Lorenzo [:jlorenzo]]

Got it.

Comment 7

[Cristian Rodriguez (:crdlc)]

We could do it but I think that solutions are risky when the bug is not so bad . We would have two approaches:

1) We could check if the bookmark is already stored in the system or not by means of BookmarkDatabase.get method importing the library and adding the access to the datastore in collection app.

2) Or adding an IAC message to know if a bookmark exists or not in the bookmark app

Comment 8

[Jason Smith [:jsmith]]

Does this same problem occur on 1.4 w/the old e.me search implementation?

Comment 9

[Cristian Rodriguez (:crdlc)]

In 1.4 version, bookmarks and collections live in the homescreen app so when an evme-app was already bookmarked a message was displayed [1] because ev.e code asked to home, is this bookmark on the grid? If we have to support the same behavior we have to follow some approach explained in comment 7.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/locales/homescreen.en-US.properties#L70

Comment 10

[Eric Kramer [:Kramer]]

This issue does NOT repro as written above on Flame 1.4. When attempting to add an app a second time the user is shown a prompt stating: "XX was already added to Home Screen"

Device: Flame 1.4
BuildID: 20140630000201
Gaia: aa896d5db1b4929f3bf31a0f4bb7de50530222a8
Gecko: 8cba60bc12ef
Version: 30.0 (1.4) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 11

[Kevin Grandon :kgrandon]

But the issue did reproduce in the browser I assume? I don't think we actually know what the expected behavior of this should be. I assume we want to change the text to say something about editing the bookmark?
Adding a ni? on ux for expected behavior.

Comment 12

[Stephany Wilkes]

Flagging Rob while Jacqueline is out. I believe the expected behavior is "XX was already added to Home Screen".

Comment 13

[Kevin Grandon :kgrandon]

(In reply to Stephany Wilkes from comment #12)
> Flagging Rob while Jacqueline is out. I believe the expected behavior is "XX
> was already added to Home Screen".

As a dialog after you click the button, or as a replacement for the button? Also wanted to verify that this should work the same from rocketbar, and the browser.

Comment 14

[Cristian Rodriguez (:crdlc)]

I completely sure that when you tried to bookmark some web from browser app, the message was not displayed. Only from ev.me collections in 1.4

(In reply to Kevin Grandon :kgrandon from comment #13)
> (In reply to Stephany Wilkes from comment #12)
> > Flagging Rob while Jacqueline is out. I believe the expected behavior is "XX
> > was already added to Home Screen".
> 
> As a dialog after you click the button, or as a replacement for the button?
> Also wanted to verify that this should work the same from rocketbar, and the
> browser.

Comment 15

[Amir Nissim (:amirn)]

(In reply to Jason Smith [:jsmith] - PTO 7/14 - 7/18 from comment #8)
> Does this same problem occur on 1.4 w/the old e.me search implementation?

In v1.4 (and before)
--------------------
E.me displays a "Save to homescreen" contextmenu option for already-saved web results, but then shows an "Already added to homescreen" message.
The browser app also shows the "Save to homescreen" contextmenu entry but does not show the "Already added message"


In 2.0
------
Search, Browser and Collection apps are all using the 'save-bookmark' activity handled by the bookmark app. 

All apps show an 'Add to Homescreen' contextmenu entry to an already saved bookmark (Collection actually shows 'Save to Homescreen' which we may want to change for consistency).


Changing the summary since this issue applies also to the Browser and Search apps.

An easy "fix" can be to rephrase 'Add/Save to Homescreen' in all relevant apps.

Comment 16

[Amir Nissim (:amirn)]

Attachment: Pull Request

The patch changes 'Save to Homescreen' to 'Add to Homescreen' in the collection app to match the phrasing in other apps.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8460890 [details] [review]
Pull Request

Hey, it looks like you have some git troubles =(

Can you try rebasing? I think somehow you picked up another merge.

Comment 18

[Amir Nissim (:amirn)]

Comment on attachment 8460890 [details] [review]
Pull Request

(In reply to Amir Nissim (:amirn) from comment #16)
> Created attachment 8460890 [details] [review]

filed bug 1043216 for the text changes in the collection app

Comment 19

[Amir Nissim (:amirn)]

Changing the component per comment 15. 
This applies to Browser, Search and Collections apps.

Comment 20

[Rob MacDonald [:robmac]]

NI'ing Francis to consider as part of system browser spec.

Comment 21

[Francis Djabri [:djabber]]

(In reply to Cristian Rodriguez (:crdlc) PTO: 7/28 -> 8/18 from comment #14)
> I completely sure that when you tried to bookmark some web from browser app,
> the message was not displayed. Only from ev.me collections in 1.4
> 
> (In reply to Kevin Grandon :kgrandon from comment #13)
> > (In reply to Stephany Wilkes from comment #12)
> > > Flagging Rob while Jacqueline is out. I believe the expected behavior is "XX
> > > was already added to Home Screen".
> > 
> > As a dialog after you click the button, or as a replacement for the button?
> > Also wanted to verify that this should work the same from rocketbar, and the
> > browser.

My preferred solution would be not to show the "Add to Home Screen" option if the item is already on the Home Screen. This would apply across smart collections, search results and the browser.

Comment 22

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

thanks guys

Comment 23

[Amir Nissim (:amirn)]

Added comments on Github.
The patch does not cover the browser app, should we file another bug?

Comment 24

[Cristian Rodriguez (:crdlc)]

I think so, the patch implements the behavior that we had in 1.x (ev.me/homescreen now search/homescreen)

(In reply to Amir Nissim (:amirn) from comment #23)
> Added comments on Github.
> The patch does not cover the browser app, should we file another bug?

Comment 25

[Cristian Rodriguez (:crdlc)]

Addressed comments, thanks

(In reply to Amir Nissim (:amirn) from comment #23)
> Added comments on Github.
> The patch does not cover the browser app, should we file another bug?

Comment 26

[Cristian Rodriguez (:crdlc)]

Any update here from reviewers?

Comment 27

[Amir Nissim (:amirn)]

Comment on attachment 8481219 [details]
Github pull request

Looks good. Thanks Cristian :)

Comment 28

[Amir Nissim (:amirn)]

looks like the new tests are failing :(

TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js | contextmenu > clicking an icon not bookmarked - TypeError: BookmarksDatabase.get(...).then(...) is undefined (app://collection.gaiamobile.org/js/contextmenu.js?time=1409661771758:124)
TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js | contextmenu > clicking an icon not bookmarked - expected false to be truthy
TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js | contextmenu > "before each" hook - Attempted to wrap show which is already stubbed

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8481219 [details]
Github pull request

I'm fine with Amir's review here, but please update the tests. Thanks!

Comment 30

[Cristian Rodriguez (:crdlc)]

Thanks a lot, I gonna address it

(In reply to Amir Nissim (:amirn) from comment #28)
> looks like the new tests are failing :(
> 
> TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js |
> contextmenu > clicking an icon not bookmarked - TypeError:
> BookmarksDatabase.get(...).then(...) is undefined
> (app://collection.gaiamobile.org/js/contextmenu.js?time=1409661771758:124)
> TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js |
> contextmenu > clicking an icon not bookmarked - expected false to be truthy
> TEST-UNEXPECTED-FAIL | collection/test/unit/contextmenu_test.js |
> contextmenu > "before each" hook - Attempted to wrap show which is already
> stubbed

Comment 31

[Cristian Rodriguez (:crdlc)]

I have seen the problem, I added a mistake with the latest changes once addressed Amir's comment hehehe. Updating right now and waiting for green. Thanks guys

Comment 32

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/a29ac6b1222c8367c5fb3a15ac238d13b3bda696