Bookmark titles should be editable

RESOLVED DUPLICATE of bug 1243019

Status

()

Firefox for iOS
Home screen
RESOLVED DUPLICATE of bug 1243019
2 years ago
a year ago

People

(Reporter: alx91, Assigned: alx91)

Tracking

(Depends on: 1 bug)

unspecified
All
iOS

Firefox Tracking Flags

(fxios5.0+)

Details

(Whiteboard: [hold until bookmarks sync is done])

Attachments

(1 attachment)

48 bytes, text/x-github-pull-request
tecgirl
: ui-review+
fluffyemily
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

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

Comment 1

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

Comment 2

2 years ago
Created attachment 8684708 [details] [review]
Pull request
Attachment #8684708 - Flags: ui-review?
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.
Attachment #8684708 - Flags: ui-review?(randersen)
Attachment #8684708 - Flags: ui-review?
Attachment #8684708 - Flags: feedback?(rnewman)
(Assignee)

Comment 4

2 years ago
Would you suggest that we do anything in relation to modifying bookmarks? Or should it just be like it is?
I don't personally have a strong opinion either way. I value Robin's and Maria's opinions more.
(Assignee)

Comment 6

2 years ago
Ok, then let's wait for their replies
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!).
Attachment #8684708 - Flags: ui-review?(randersen) → ui-review+
(Assignee)

Comment 8

2 years ago
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.
(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
Assignee: nobody → alx91
Status: UNCONFIRMED → ASSIGNED
Depends on: 1196238
Ever confirmed: true
Hardware: Other → All
Whiteboard: [hold until bookmarks sync is done]
(Assignee)

Comment 10

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

Comment 12

2 years ago
Thank you, that is very interesting :)

Updated

2 years ago
Duplicate of this bug: 1224618

Comment 14

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

Comment 15

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

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

Comment 17

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

Comment 18

2 years ago
What do you think?

Updated

2 years ago
Flags: needinfo?(rnewman)
(Assignee)

Comment 19

2 years ago
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 on attachment 8684708 [details] [review]
Pull request

Emily, could you take a look at this, please?
Flags: needinfo?(rnewman)
Attachment #8684708 - Flags: feedback?(rnewman) → feedback?(etoop)
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.
Attachment #8684708 - Flags: feedback?(etoop) → feedback+

Updated

2 years ago
tracking-fxios: --- → ?
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1243019
tracking-fxios: ? → 5.0+
You need to log in before you can comment on or make changes to this bug.