[Browser][User Story] Bookmark editing in bookmark tab

RESOLVED WONTFIX

Status

P3
normal
RESOLVED WONTFIX
6 years ago
5 years ago

People

(Reporter: pdol, Unassigned)

Tracking

({feature, uiwanted})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: c=browser u=user)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
UCID: Browser-047

User Story:
As a user I want to be able to edit the name or url of a bookmarked page from my "Bookmarks" tab to make them easier to use.

Updated

6 years ago
Keywords: feature
Summary: [B2G][Browser][User Story] Bookmark editing → [Browser][User Story] Bookmark editing
Clearing tracking-b2g18 flag from User Story bugs. This flag is for bugs that we would take fixes for in the 1.x branch. Feature work should be officially slotted into a release instead with leo+. If this story is intended for v1.1, please nominate for leo? blocking.
tracking-b2g18: + → ---

Updated

6 years ago
Assignee: nobody → gasolin

Comment 2

6 years ago
Created attachment 720524 [details]
add Bookmark editing/removing support in Bookmarks tab

* add listener for contextmenu in bookmark tab.
* show remove and edit options.
* use bookmark_url property to save selected bookmark url
* rewrite removeBookmark and showBookmarkEntrySheet to allow bookmark_url property from bookmark tab.
* update bookmark tab if changed
Attachment #720524 - Flags: review?(bfrancis)
Comment on attachment 720524 [details]
add Bookmark editing/removing support in Bookmarks tab

Thanks for the patch, but I've noticed a few problems when doing some brief initial testing.

Firstly I keep seeing the following error in the console when using this feature:

E/GeckoConsole(  774): [JavaScript Error: "TypeError: Places.getBookmark(...) is undefined" {file: "app://browser.gaiamobile.org/js/browser.js" line: 970}]

Secondly, sometimes the "Add to home screen" option disappears from the bookmark menu and won't come back.

Thirdly, this context menu appears when long pressing on entries in the history and top sites tabs which gives confusing results.

Could you take a look at these issues before I test further or finish my review?

Also, might there be a way to pass the URL to the method that displays the menu rather than storing it in a temporary property on the Browser object?

Is this a 1.1 feature btw? It's not marked in any way to say that it is, how do we know?

Thanks
Attachment #720524 - Flags: review?(bfrancis) → review-
(In reply to Ben Francis [:benfrancis] from comment #3)
> Is this a 1.1 feature btw? It's not marked in any way to say that it is, how
> do we know?
> 

The related bug 838040 is a v1.1 feature. This is also a 1.1 story but for the most part only the P1 stories are leo+. We'd take this one if it also solves the blocking bug, but 838040 is the priority.

Comment 5

6 years ago
Thanks for review,

1st issue is because of miss-binding, fixed.

2nd, should "Add to home screen" option appear when hit a 'stared' bookmark? (I didn't find this option in current behavior when I hit the 'star' in browser tab). Maybe we could clearify it.

3rd, I add an 'current_tab' option for drawAwesomescreenListItem. current_tab is used for distinguish the current tab is bookmark. Now other tabs wont bind with long press menus.

Comment 6

6 years ago
Created attachment 722095 [details]
fix miss-binding and bound contextmenu only in bookmark tab
Attachment #722095 - Flags: review?(bfrancis)
Comment on attachment 722095 [details]
fix miss-binding and bound contextmenu only in bookmark tab

Please see comments on GitHub
Attachment #722095 - Flags: review?(bfrancis) → review-

Comment 8

6 years ago
Made a candidate pull request https://github.com/mozilla-b2g/gaia/pull/8606

I found after 'reset-gaia' then open browser.app immediately, there's a high chance to get default mcc/mnc settings as 0.

but navigator.mozMobileConnection.iccInfo.mcc/mnc can always get the correct value.

Comment 9

6 years ago
sorry Comment 8 is put in wrong issue # (should be reply to 835350)

Updated

6 years ago
Summary: [Browser][User Story] Bookmark editing → [Browser][User Story] Bookmark editing in bookmark tab

Comment 10

6 years ago
Created attachment 731762 [details]
Support Edit bookmark in bookmark tab

Follow new approach to implement bookmark edit in bookmark tab.
Need advice about updatebookMark behavior (see in github pullreqest comment).
Attachment #731762 - Flags: review?(bfrancis)
Comment on attachment 731762 [details]
Support Edit bookmark in bookmark tab

updateBookmark won't take effect synchronously, you'll need to wait for the callback to say it's been completed, then update the view accordingly.

I really don't like the use of "dataset.url" in this method and it's getting a little messy. I'd really like to refactor this bit of code myself if possible so don't worry if you don't get to it.
Attachment #731762 - Flags: review?(bfrancis) → review-

Comment 12

5 years ago
Enabled Test Case #1890: Able to edit web site bookmarks
For version 1.1.0 testing.
Duplicate of this bug: 835138
Whiteboard: c=browser u=user
Priority: P2 → P3
Flags: needinfo?(ibarlow)
Keywords: uiwanted

Comment 14

5 years ago
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548961

Comment 15

5 years ago
we need to fixed it on V1.1.
Thanks.
blocking-b2g: --- → leo?
Triage - not blocking for triaging partners as this is a feature request for 1.1.
blocking-b2g: leo? → ---

Comment 17

5 years ago
Any update on this bug? Are we going to fix it on v1.1, or not?
Ian, Karen, I think this is sprint ready because we have specs here http://people.mozilla.com/~lco/FX_B2G/Release_1_Specs/R1_Bookmarks_v2.pdf
Flags: needinfo?(krudnitski)
Ian, Francis - these specs look fine by me. Please confirm they're the latest (attached in C18) and I'll flag this for sprint-ready.
Flags: needinfo?(krudnitski) → needinfo?(fdjabri)

Updated

5 years ago
Assignee: gasolin → nobody
(Reporter)

Comment 20

5 years ago
Given the Browser OS work, moving to WONTFIX.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX

Updated

5 years ago
Flags: needinfo?(ibarlow)
Flags: needinfo?(fdjabri)
You need to log in before you can comment on or make changes to this bug.