Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Places File Bookmark dialog should be resizeable.

RESOLVED FIXED in seamonkey2.10

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
seamonkey2.10
regression
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.9 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
Duplicate of this bug: 606151
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.
Keywords: regression
(Reporter)

Comment 3

6 years ago
> at the same time should be relatively easy to fix
Assigned => InvisibleSmiley then?
(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)...
Duplicate of this bug: 716878

Comment 6

6 years ago
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
(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;
}
(Assignee)

Comment 8

6 years ago
Created attachment 601758 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #601758 - Flags: review?(jh)
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 neil@parkwaycc.co.uk 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?
(Assignee)

Comment 10

6 years ago
(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!
(In reply to neil@parkwaycc.co.uk 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 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.
Attachment #601758 - Flags: review?(jh) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 602746 [details] [diff] [review]
Proposed patch
Attachment #601758 - Attachment is obsolete: true
Attachment #602746 - Flags: review?(jh)
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.
Attachment #602746 - Flags: review?(jh) → review+
(Assignee)

Comment 15

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

Comment 16

5 years ago
Created attachment 602759 [details] [diff] [review]
Addressed review comments

With jar.mn change.
Attachment #602746 - Attachment is obsolete: true
Attachment #602759 - Flags: review+
Comment on attachment 602759 [details] [diff] [review]
Addressed review comments

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

Comment 18

5 years ago
Created attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

I blame bug 724881 or any similar bugs that have previously been WONTFIXed...
Attachment #602759 - Attachment is obsolete: true
Attachment #602764 - Flags: review?(jh)
Attachment #602764 - Flags: review?(jh) → review+
Blocks: 733687
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.
(Assignee)

Comment 20

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

Comment 21

5 years ago
Pushed changeset cbfab6350b17 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
Attachment #602764 - Flags: approval-comm-aurora?

Updated

5 years ago
Attachment #602764 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 602764 [details] [diff] [review]
Addressed review comments [Checkin: Comments 21 and 23]

http://hg.mozilla.org/releases/comm-aurora/rev/da05932da851
Attachment #602764 - Attachment description: Addressed review comments → Addressed review comments [Checkin: Comments 21 and 23]
status-seamonkey2.9: --- → fixed
Target Milestone: --- → seamonkey2.10

Comment 24

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

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

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

Updated

5 years ago
Depends on: 739049
You need to log in before you can comment on or make changes to this bug.