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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Wong Huong Cheong]

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

[Alice0775 White]

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

Comment 2

[Marco Bonardo [:mak]]

this should be fixed in FF10, is pretty much annoying.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Marco Bonardo [:mak]]

And I think we should backout Bug 696159 from Aurora

Comment 5

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 6

[Dietrich Ayala (: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?

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1ec9ca46cd

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a1ec9ca46cd

Comment 10

[Alex Keybl [:akeybl]]

This is a regression in 10 - let's get this nominated for approval if deemed low risk.

Comment 11

[Marco Bonardo [:mak]]

(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.

Comment 12

[Alex Keybl [:akeybl]]

Thanks Marco, untracking for 10 in that case.