The default bug view has changed. See this FAQ.

Some bookmarks dialogs lost the folder picker, thus making hard to choose their location

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Wong Huong Cheong, Assigned: mak)

Tracking

(Blocks: 1 bug, {regression})

10 Branch
Firefox 11
x86
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111112 Firefox/10.0a2
Build ID: 20111112042038

Steps to reproduce:

Right click any hyperlink and choose "Bookmark this link"


Actual results:

Not able to choose different bookmark folder


Expected results:

Able to choose different bookmark folder

Comment 1

5 years ago
Regression window(m-i),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/32ab009026d7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111027 Firefox/10.0a1 ID:20111027020310
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1f8780bd07ae
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111027 Firefox/10.0a1 ID:20111027023108
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=32ab009026d7&tochange=1f8780bd07ae

Suspected:
1ed8be9fed39	Marco Bonardo — Bug 696159 - Remove some deprecated Places code. r=dietrich sr=rstrong
Blocks: 696159
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(Assignee)

Updated

5 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
this should be fixed in FF10, is pretty much annoying.
tracking-firefox10: --- → ?
(Assignee)

Comment 3

5 years ago
So, all of this has been caused by the misunderstoodment of minimalUI, my fault  caused by bad initial naming and lack of comments (plus hacks and workarounds on that code).
A dialog opened in minimalUI is supposed to be resizable, thus uses an alternative chrome url to be able to persist a different dialog size.
Which dialogs should be resizable? The ones who show a folder picker, since the hiearchy can easily go out of view in some cases.
I'm going to rename the aMinimalUI argument to aResizable, it's functionality will be the same, but the code will gain in clarity.
Finally I'll poll the dialogs to check we currectly show the folders picker where it's needed, should be easy at that point.
(Assignee)

Comment 4

5 years ago
And I think we should backout Bug 696159 from Aurora
(Assignee)

Updated

5 years ago
Summary: "Bookmark this link" fail to choose folder → Some bookmarks dialogs lost the folder picker, thus making hard to choose their location
(Assignee)

Comment 5

5 years ago
Created attachment 574291 [details] [diff] [review]
patch v1.0

The only case where we should hide the folderPicker is when we already know the destination, that is:
- when adding a new item to a view in PlacesController.newItem()
- when editing properties of an item in a view in PlacesController.showBookmarkPropertiesForSelection() 

In all the other cases we don't know where is the bookmark, nor the user, so it should be available.

In the "Add Keyword For this search" case, we should ask minimal information, right now we ask for tags description, when the user just wants to provide a keyword (and he already has to add a name and a location).

When adding a link="rel" bookmark (left click on a "rel=sidebar" link), we should not hide the "load in sidebar" checkbox, nor the location, since this a is possible scam source (load in sidebar should die, btw).

Finally, this clarifies what minimalUI means. Ideally we may make both cases resizeable and just persist the sizes separately, I don't completely understand why we decided to allow resizability only in one case. Dietrich?

PS: I'm not making a test since the matrix of possible cases here is pretty wide, it would likely always miss cases.
PPS: The right long-term fix for this is to completely stop using dialog windows.
Attachment #574291 - Flags: review?(dietrich)
Comment on attachment 574291 [details] [diff] [review]
patch v1.0

Review of attachment 574291 [details] [diff] [review]:
-----------------------------------------------------------------

about tests, should add in-litmus? maybe so that we can get coverage that way. in fact, we likely already do have some that way.

::: browser/components/sidebar/src/nsSidebar.js
@@ +109,5 @@
>      win.PlacesUIUtils.showBookmarkDialog({ action: "add"
>                                           , type: "bookmark"
>                                           , hiddenRows: [ "description"
>                                                         , "keyword"
> +                                                       , "location" ]

why making loadInSidebar be visible here?
Attachment #574291 - Flags: review?(dietrich) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Dietrich Ayala (:dietrich) from comment #6)
> about tests, should add in-litmus? maybe so that we can get coverage that
> way. in fact, we likely already do have some that way.

Probably yes, still the results matrix is complex to check, since we open the dialog from a lot different points. We may add a generic test with what I said above, so:
- if generated from a Bookmarks view, should not show the folder picker
- if generated from something that as as a panel should show loadInSidebar

> ::: browser/components/sidebar/src/nsSidebar.js
> @@ +109,5 @@
> >      win.PlacesUIUtils.showBookmarkDialog({ action: "add"
> >                                           , type: "bookmark"
> >                                           , hiddenRows: [ "description"
> >                                                         , "keyword"
> > +                                                       , "location" ]
> 
> why making loadInSidebar be visible here?

This is the internal implementation of .addPanel(), that is a dumb method we make available to content to allow a page to add a sidebar webpanel. loadInsidebar should be visible because often pages hijack addPanel() to force users to add a bookmark to them, and users are surprised by seeing their bookmark opened in the sidebar rather than in content. Really bug 560305 should be fixed and we should forget this crazy feature.
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1ec9ca46cd
Flags: in-litmus?
Target Milestone: --- → Firefox 11
https://hg.mozilla.org/mozilla-central/rev/2a1ec9ca46cd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 712533
This is a regression in 10 - let's get this nominated for approval if deemed low risk.
tracking-firefox10: ? → +
(Assignee)

Comment 11

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #10)
> This is a regression in 10 - let's get this nominated for approval if deemed
> low risk.

the bug causing this regression (bug 696159) has been backed out from 10, so I think there's nothing more to do here.
Thanks Marco, untracking for 10 in that case.
tracking-firefox10: + → -
(Assignee)

Updated

3 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.