605786 - Places File Bookmark dialog should be resizeable.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Philip Chee]

Splitting off resizeability issues to a separate bug.

From Bug 586947 Comment 46:

>>-    var features;
>>-    if (aMinimalUI)
>>-      features = "centerscreen,chrome,modal,resizable=yes";
>>-    else
>>-      features = "centerscreen,chrome,modal,resizable=no";
>>-    this._getCurrentActiveWin().openDialog(dialogURL, "",  features, aInfo);
>>+    var features = "centerscreen,chrome,modal,resizable=yes";
> Sorry, but I think the resizability issue is too complicated to include in this
> bug, so I'd prefer to go with this patch, but without this resizable change.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

I think it should not only be resizable but must also persist its size across sessions.

Furthermore, the area below the Folder drop-down must IMO have a flex (take any space that is gained by resizing the window) and its visibility (expanded state) must be made persistent. If this needs to go to its own bug, fine. But I think it's a vital part to make this dialog usable again.

I'm adding the regression keyword because all of this is a huge UX regression compared to SM 2.0, and at the same time should be relatively easy to fix.

Comment 3

[Philip Chee]

> at the same time should be relatively easy to fix
Assigned => InvisibleSmiley then?

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> > at the same time should be relatively easy to fix
> Assigned => InvisibleSmiley then?

If only I knew what I was doing. It's easy to describe what I want, but hard to make it work.

I succeeded in making the window resizable (not taking different dialog types into account yet), persist width and height, and to make the area below the Folder drop-down flexible (mostly by adding flex="1" to <vbox id="editBookmarkPanelContent">), but failed to make the folder selection <tree> flexible (it currently has a fixed height). Maybe the <grid>/<vbox> combination is preventing it (until SM 2.0, the <tree> was inside a <vbox> but not a <grid>) from working the way I would expect it. Adding flex="1" to various elements using DOMI didn't help much, either. :-/

Furthermore I failed to make the expanded state persistent (tried persist="collapsed" in bm-props.xul). Either I don't get it or this needs a JS workaround.

I begin to wonder whether it's me or XUL that is too dumb. If trouble continues I might just create an add-on that reverts the UI back to what SM 2.0 had (plus tags support of course)...

Comment 6

[Phoenix]

Can't find this bug with search...
Anyway, this regression is one of the reasons, holding part of users on 2.0.x, see http://forums.mozillazine.org/viewtopic.php?f=3&t=2364173 for example

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Phoenix from comment #6)
> Anyway, this regression is one of the reasons, holding part of users on
> 2.0.x, see http://forums.mozillazine.org/viewtopic.php?f=3&t=2364173 for
> example

I know... I asked Ratty whether he can port the OpenBook add-on which should help in the mean time.

I use the following Stylish user style to at least get a taller list (when expanded):

#editBMPanel_folderTree {
min-width: 340px ! important;
min-height: 400px ! important;
}
#editBookmarkPanelContent {
min-width: 300px ! important;
}

Comment 8

[[e-mail]]

Attachment: Proof of concept

This is just to make the toggles interact with resizing; I didn't try to persist the height but that's easy to add if it is wanted. Presumably in the .jsm we would just unconditionally make the dialog resizable?

The ugly outerWidth hack exists because I couldn't come up with a sane way of predicting what sizeToContent() would decide to size the width to.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 601758 [details] [diff] [review]
Proof of concept

[Sorry, no time for an actual review yet. :-( But thanks for giving this a try, it surely is appreciated!]

(In reply to [e-mail] from comment #8)
> I didn't try to persist the height but that's easy to add if it is wanted.

It is! :-) Super bonus points for persisting the expanded state, too (otherwise it'll be a follow-up issue).

I only did very few testing now (on Linux) and it happened to me once that the Description field resized instead of the tree area. Need to try and reproduce when that happens.

> Presumably in the .jsm we would just unconditionally make the dialog resizable?

Err, which .jsm?

I noticed, though, that even with the patch the dialog is not resizable at all on Windows; I guess we need to adjust the calling code for that. Could you check/do that please?

Comment 10

[[e-mail]]

(In reply to Jens Hatlak from comment #9)
> (In reply to comment #8)
> > I didn't try to persist the height but that's easy to add if it is wanted.
> It is! :-) Super bonus points for persisting the expanded state, too
> (otherwise it'll be a follow-up issue).
I guess that's possible, but I didn't look at that part of the code...

> I only did very few testing now (on Linux) and it happened to me once that
> the Description field resized instead of the tree area. Need to try and
> reproduce when that happens.
Well, I did make the Description field resizable too, in case you had collapsed the tree and tags.

> > Presumably in the .jsm we would just unconditionally make the dialog resizable?
> Err, which .jsm?
PlacesUIUtils.jsm

> I noticed, though, that even with the patch the dialog is not resizable at
> all on Windows; I guess we need to adjust the calling code for that. Could
> you check/do that please?
The calling code? Oh, you mean the .jsm ;-) See my question above!

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #10)
> Well, I did make the Description field resizable too, in case you had
> collapsed the tree and tags.

I doubt many people use the Description field, and it is scroll bars enabled, so I'd be OK with keeping it fixed-height (say, three lines). Having the possibility to resize the dialog to a custom width should already take care of most lengthy descriptions (assuming they are rather continuous text than multi-line).

> > > Presumably in the .jsm we would just unconditionally make the dialog resizable?
> > Err, which .jsm?
> PlacesUIUtils.jsm

Then yes, always resizable (unless Stefan objects for possible Mac-specific reasons, but for now let's assume he agrees).

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 601758 [details] [diff] [review]
Proof of concept

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

r=me with the nit addressed and an unconditional resizable=yes flag in the .jsm (I'll leave it to you whether to have a single "var features =" line or inline it and kill the variable altogether).

Thanks a ton!

If you choose to go one step further and try to come up with a patch (replacing this one or an extra one) that also remembers the tree expanded state (including the twisty) that would be very much appreciated! :-) Otherwise, if you choose to close this bug, please open a follow-up to take care of that aspect. Thanks!

Note: The Bookmark This Group of Tabs option seems to not remember the dialog size, or reset it. As long as the tree expanded state is not saved, that doesn't really matter, though.

Once this has baked a bit on trunk we might consider backporting to branches (well, right now probably not to Beta anymore).

::: suite/common/bookmarks/bm-props.js
@@ +401,5 @@
>          }
>          break;
>  
> +      case "resize":
> +        ["editBMPanel_folderTreeRow", "editBMPanel_tagsSelectorRow", "editBMPanel_descriptionRow"].forEach(function(e) {

Please wrap (and indent accordingly) after the second array element.

Comment 13

[[e-mail]]

Attachment: Proposed patch

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 602746 [details] [diff] [review]
Proposed patch

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

r=me with the extra removals.

Please file a follow-up bug for remembering the tree expanded state as explained in comment 12. Thanks!

::: suite/common/src/PlacesUIUtils.jsm
@@ -643,5 @@
>     * @return true if any transaction has been performed, false otherwise.
>     */
> -  _showBookmarkDialog: function PUIU__showBookmarkDialog(aInfo, aMinimalUI) {
> -    var dialogURL = aMinimalUI ?
> -                    "chrome://communicator/content/bookmarks/bm-props2.xul" :

You're removing the last (and only) reference to bm-props2.xul here. I'm fine with that (FF removed the minimal UI in bug 696159, which we're planning to do in our bug 712533), but please remove the obsolete file here (including the jar.mn change) before it gets lost.

BTW the method signature change is OK, too. I considered add-on compat for a second, but current FF code has two other (incompatible) parameters so this change doesn't make it worse.

Comment 15

[[e-mail]]

(In reply to Jens Hatlak from comment #14)
> You're removing the last (and only) reference to bm-props2.xul here. I'm
> fine with that (FF removed the minimal UI in bug 696159, which we're
> planning to do in our bug 712533), but please remove the obsolete file here
> (including the jar.mn change) before it gets lost.
Actually it's an override, not an actual file, but I do need the jar.mn change.

Comment 16

[[e-mail]]

Attachment: Addressed review comments

With jar.mn change.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 602759 [details] [diff] [review]
Addressed review comments

Wrong upload? Well, as long as you push the right thing... ;-)

Comment 18

[[e-mail]]

Attachment: Addressed review comments [Checkin: Comments 21 and 23]

I blame bug 724881 or any similar bugs that have previously been WONTFIXed...

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Neil, I filed that follow-up for you. Anything else keeping you from checking in and closing this one?

Mind you, next Tuesday is Aurora uplift day. Ideally this one should land before that. Then we can either go for Aurora approval before or Beta approval after that day.

Comment 20

[[e-mail]]

(In reply to Jens Hatlak from comment #19)
> Anything else keeping you from checking in and closing this one?
I often don't have enough free time on Mondays or Tuesdays.

Comment 21

[[e-mail]]

Pushed changeset cbfab6350b17 to comm-central.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

[Approval Request Comment]
Regression caused by (bug #):                          Bug 498596
User impact if declined:                               Continued bad UX
Testing completed (on m-c, etc.):                      Works with latest nightly
Risk to taking this patch (and alternatives if risky): None I know
String changes made by this patch:                     None

Comment 23

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

http://hg.mozilla.org/releases/comm-aurora/rev/da05932da851

Comment 24

[therube]

To note:  Width & location persist, but height does not.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120316 Firefox/13.0a2 SeaMonkey/2.10a2

Comment 25

[therube]

> <NeilAway>	therube: ideally bug 733687 should fix that too

So I'll leave it at that :-).