1222848 - Bookmark titles should be editable

Status: RESOLVED DUPLICATE of bug 1243019

(Firefox for iOS :: Home screen, defect)

Description

[alx91]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

Add any website as a bookmark. 


Actual results:

The title of the bookmark is inferred from the websites title and cannot be changed.


Expected results:

There should be a way to edit the title

Comment 1

[alx91]

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1164173 there should be no description text in the bookmark cell, because users might confuse it with the history items. But since you can have multiple bookmarks from the same host, it can happen that you have seemingly multiple identical items even though they differ. That's why I would suggest to allow a user to edit the title of a bookmark. An example implementation (without tests and localization) can be found in the attached PR and a short demonstration in this GIF : https://dl.dropboxusercontent.com/u/28177144/FirefoxEditBookmarkTitle.gif

Comment 2

[alx91]

Attachment: Pull request

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8684708 [details] [review]
Pull request

From an experience perspective, I don't know if we want to do this yet. We don't support any kind of editing other than deletion, and I would speculate that users want to organize or reorder bookmarks more than they want to rename them. 

I'll need to look at this from a data model perspective. I'm about to bitrot the heck out of anything that writes to bookmark storage.

Comment 4

[alx91]

Would you suggest that we do anything in relation to modifying bookmarks? Or should it just be like it is?

Comment 5

[Richard Newman [:rnewman]]

I don't personally have a strong opinion either way. I value Robin's and Maria's opinions more.

Comment 6

[alx91]

Ok, then let's wait for their replies

Comment 7

[Robin Andersen [:tecgirl]]

Comment on attachment 8684708 [details] [review]
Pull request

I can see where this could be helpful, especially with long sub-domain titles and the examples you've provided. Though currently this only works on locally saved Bookmarks—not synced. Not being able to edit all may cause confusion, and further confusion because they're unable to see mobile bookmarks on Desktop.

I think we should hold off on it until bi-directional sync is fully in place so as not to confuse the user as to what they can/can not edit. I will + for the local instance which works and looks good. You might want to lighten the color on URL in the modal and restrict the URL character length (with truncation) so it doesn't end up taking up the whole viewport (a use-case, but we've seen it happen!).

Comment 8

[alx91]

Hey Robin,

thanks a lot for the reply. I implemented your UI tips and here is what it would look like https://dl.dropboxusercontent.com/u/28177144/Screen%20Shot%202015-11-10%20at%2022.56.22.png
I used the already existing ellipsize() String extension out of convenience, if we want a trailing ... I would add another method to that extension. 
Should I write (add) tests to UITests/BookmarkingTests and ClientTests/TestBookmarks?
I would also really like to know how you handle the localization of all your Strings? Maybe you can give me a link where I can read something about how it works for this project.

Comment 9

[Richard Newman [:rnewman]]

(In reply to alx91 from comment #8)

> Should I write (add) tests to UITests/BookmarkingTests and
> ClientTests/TestBookmarks?

Tests are fantastic and always appreciated.

> I would also really like to know how you handle the localization of all your
> Strings? Maybe you can give me a link where I can read something about how
> it works for this project.

We use NSLocalizedString. See, e.g.,

https://github.com/mozilla/firefox-ios/blob/master/Client/Frontend/Settings/SearchEnginePicker.swift#L19

Comment 10

[alx91]

(In reply to Richard Newman [:rnewman] from comment #9)

> We use NSLocalizedString. See, e.g.,

I meant who is providing you with the Strings? And what is the workflow when you have new Strings to be translated.

Comment 11

[Richard Newman [:rnewman]]

(In reply to alx91 from comment #10)

> I meant who is providing you with the Strings? And what is the workflow when
> you have new Strings to be translated.

Sometimes engineers; after all, we communicate with words all day, so some of us are pretty good at writing strings (at least a first stab).

Often strings come from UX folk like Robin; they think about the whole experience, and typically have a good understanding of platform conventions.

Very often it's a combination of an engineer like me and a designer like Robin; some such combinations work very well indeed.

In particularly thorny cases, we ask Matej, our resident wordsmith, for his opinion. In all cases we get multiple eyes to make sure we come up with a good option!

Strings land in the repo when we're not string-frozen. Localizers translate using tools against exports from the repo, and we import current strings each time we do a build.

The only way this affects coding contributors, for the most part, is to delay the landing of some PRs until we're out of a pre-release string freeze.

Comment 12

[alx91]

Thank you, that is very interesting :)

Comment 14

[Dave Royal]

bug 1224618 is not an exact duplicate of this bug. I found this bug, as I mentioned.

I argue there that both title and URL should be editable.

Comment 15

[alx91]

Did you see my implementation approach? How would you add the feature of editing the url? We might have a problem when the page doesn't have a title and the url is inferred from the url. Would changing the url affect the title in the same way?

Comment 16

[Dave Royal]

(In reply to alx91 from comment #15)
> Did you see my implementation approach? How would you add the feature of
> editing the url? We might have a problem when the page doesn't have a title
> and the url is inferred from the url. Would changing the url affect the
> title in the same way?

I'm a user, not a developer. I just want to change title and/or URL of any bookmark I've created, using the keyboard. There is no reason to cross-check or validate either. It's essential functionality, IMO.

One reason - though not the main one - is to create bookmarklets. I use these a lot. So do not validate the scheme.

Comment 17

[alx91]

I added some UI Tests for editing a bookmark title alongside checks for empty/whitespace-only strings. https://github.com/mozilla/firefox-ios/pull/1223/commits

Comment 18

[alx91]

What do you think?

Comment 19

[alx91]

I could also try to help solving the other issues that my PR depends on, but it would be cool to get some feedback so I could implement suggestions and improvements first. Thanks!

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8684708 [details] [review]
Pull request

Emily, could you take a look at this, please?

Comment 21

[Emily Toop (:fluffyemily)]

Comment on attachment 8684708 [details] [review]
Pull request

The overall implementation is good. Just need to be a bit more careful with force unwrapping things and localizing all the strings. It works well and the test is nice and thorough.

Left a few nits in the PR.