1249536 - Too small height of Folder Pane in menu 'Bookmarks → File Bookmark ...'

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Rainer Bielefeld]

Attachment: screenshot shows crppled vertical scrollbar problem

Steps how to reproduce  NOT reproducible REPRODUCIBLE with unofficial (from <http://seamonkey.callek.net/contrib/>)  en-US SeaMonkey 2.44a1  Mozilla/5.0 (Windows NT 6.1; x64; rv:47.0)  Gecko/20100101 Firefox/47.0 Build 20160208090255  (Default Classic Theme)  on German WIN7 64bit:

1. Launch Browser with arbitrary start page
2. Menu 'Bookmarks → File Bookmark ...' 
   » Dialog appears
3. if you want: Drag and drop bottom border of the dialog up as far as
   possible
   » Description pane height decreases; no problems, buttons stay
     visible.
4. Click [▼] Right from Folder Dropdown
   Expected: Folder Pane becomes visible with sufficient height to 
             see some folder hierarchy structure, additionally 
             [New Folder] button
   Actual (bugs): only grey horizontal line (or in best case 1 folder
                  name appears in folder name
                  Strange crippled verticla scrollbar at the right of
                  folder pane exceeds folder pane height
                  [Save] and [Cancel] disappear behind bottom dialog
                  border

Additional information
a) Click on bottom dialog border will increase dialog height so that
   [Save] and [Cancel]  will reappear
b) dragging down bottom border of dialog will increase folder pane
   height so that  folder hierarchy structure becomes visible
c) Already REPRODUCIBLE with  SeaMonkey 2.42a1 Mozilla/5.0 (Windows 
   NT 6.1; WOW64; rv:45.0 from public download area)  Gecko/20100101
   Firefox/ 45.0  Build 20151111021032, (Classic Theme)
   on German WIN7 64bit
d) Was still ok with  German SeaMonkey 2.41a1 Mozilla/5.0 (Windows 
   NT 6.1; Win64; x64; rv:44.0 from akalla download area) Gecko/20100101
   Firefox/ 44.0  Build 20150926114543,  (Classic Theme) 
   on German WIN7 64bit 
e) Something special with 2.42a1: Folder pane does not become visible
   when Drag and drop bottom border down
f) My further tests did not show dependency to UI language, add-ons,
   user profile

Comment 1

[Rainer Bielefeld]

g) NOT reproducible with  English SeaMonkey 2.44a1  (X11; Linux x86_64; rv:47.0)  
   Gecko/20100101 Firefox/47.0 Build 20160208023510   (Default Theme) 
   on VirtualBox Ubuntu 14.04 LTS
   WIN only?

Comment 2

[benl]

Attachment: seamonkey 2.39 bookmark dialog too small

Yes, the bookmark dialog box can be made too small to see the folders.

This even happens in Seamonkey 2.39.

I am using the Windows Classic theme in Windows 7, so it looks like Windows 95.

Another thing:

When there are three folders in the folder textbox but the vertical size is only large enough to show two folders the scrollbar appears and you can scroll up or down.

But when there are three folders in the folder textbox but the vertical size is only large enough for one folder, the scrollbar still is shown but won't scroll the textbox at all.

So the bookmark dialog box should never become too small vertically to show at least one full line of text in the folders textbox and the scrollbar for the folders textbox should scroll through all visible folders when the folders textbox is only one line high.

Comment 3

[bennaordures]

Same with 2.42a1 Win 10 64b.
User agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 SeaMonkey/2.42a1

Comment 4

[Rainer Bielefeld]

NEW due to various observations.

@benl:
I wonder why I did not see that with 2.41 (and not with SeaMonkey German 2.39 final Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0 from official download area)  Gecko/20100101  Firefox/42.0  Build 20151103191810  (Classic Theme) on German WIN7 64bit.
Additional dependencies?

Comment 6

[blue20]

I have not seen the problem with SM 2.39, just with 2.40, and I use Mac SeaMonkey.  One should not be forced to use a patch in userChrome.css in order to save a bookmark.

Comment 8

[Rainer Bielefeld]

Version due to comment 6, DUP and own research
OS due to Bug 1258141

Comment 9

[R. Suttie]

I did get my get my File Bookmark box to look right by dragging the borders down and extending the box however, I have to extend the box every time I file a book mark.  It will not stay extended.  Have I missed something written above or is it a bug and I can not keep the box long enough when I bring it back up.  I looked at "about config" and could not see any way of keeping the box extended when I go to file a bookmark

Comment 10

[Mark]

Attachment: Preferences Dialog - right edge cut off

This also seems to affect other dialogs. e.g. In SeaMonkey 2.40 on Windows Vista, Edit > Preferences has the right-hand side cut off (see attachment). Clicking the right edge of the dialog to start dragging it, it jumps to a size large enough to show the whole content.

In case it makes any difference, this is with Aero disabled, Windows Classic theme, and SeaMonkey Default Theme.

Comment 11

[David]

Noticed the same problem as Comment 9 since upgrading to seamonkey 2.40 from 2.26

Comment 13

[Frank-Rainer Grahl (:frg)]

Bug 1274014 contains another screenshot and a clear description how to reproduce this. Still valid in 2.44.

Comment 14

[Frank-Rainer Grahl (:frg)]

Attachment: 1249536-bookmarkproperties.patch

From looking at the recent changes I still haven't a clue why this broke in 2.39. 

Instead of setting a bigger default height the patch retains the height of the window between invocations. Previously only the width was saved.

Tested with a fresh profile en-US x64 build on Windows 7.

>> User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 
>> Firefox/49.0 SeaMonkey/2.46a1
>> Build identifier: 20160522174402


Applies clean to c-r c-b c-a and can be backported if accepted.

Comment 15

[Philip Chee]

Comment on attachment 8755212 [details] [diff] [review]
1249536-bookmarkproperties.patch

Does not work for me. As in the STR all I see is the thin grey line.

<tree id="editBMPanel_folderTree"> If I add rows="3" then the folder pane shows up but the Save and Cancel buttons get pushed down out of sight. Same for the Tags selector popdown.

Note: shrunk the height to minimum before testing.

Comment 16

[Frank-Rainer Grahl (:frg)]

>> Does not work for me. As in the STR all I see is the thin grey line.

I didn't change the initial height so initially you will not see a difference. Can do this too but what works now with the patch is that you can just resize the window to any size you desire and the size will stick between invocations and browser restarts. Previously only the width was saved ant the height was always 150 pixels. Could you retest this.

FRG

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: 1249536-SetDefaultHeight-V2.patch

I borrowed some parts from Bug 1199496 (and some other Firefox parts I found relevant) and the Folder: and Tags: parts in the dialog now seem to come up with the correct height. The dialog also does resize correctly and 2 prefs are no longer needed.

I also found an error in bm-props.js at line 415:

>>  if (this._itemType == BOOKMARK_ITEM)
>>    acceptButton.disabled = !this._inputIsValid();

acceptButton was undefined here and set previously so could be removed.

Compared it against FF 46 and the behaviour seems to be the same now.

Comment 18

[Philip Chee]

Comment on attachment 8757699 [details] [diff] [review]
1249536-SetDefaultHeight-V2.patch

suite/common/jar.mn
>     content/communicator/bookmarks/bm-props.xul                      (bookmarks/bm-props.xul)
> +   content/communicator/bookmarks/bm-props2.xul                     (bookmarks/bm-props.xul)
Don't do that. Instead do this:

      content/communicator/bookmarks/bm-props.xul                      (bookmarks/bm-props.xul)
  +# Provide another URI for the bookmark properties dialog so we can persist the attributes separately
  +% override chrome://communicator/content/bookmarks/bm-props2.xul chrome://communicator/content/bookmarks/bm-props.xul

See: https://hg.mozilla.org/comm-central/rev/291fee0046c7#l3.11

suite/modules/PlacesUIUtils.jsm

> +    let hasFolderPicker = !("hiddenRows" in aInfo) ||
> +                          !aInfo.hiddenRows.includes("folderPicker");
> +                          // Use a different chrome url to persist different sizes.
This comment is wrongly indented
> +    let dialogURL = hasFolderPicker ?

suite/common/bookmarks/bm-props.js

> +        let collapsed = target.getAttribute("collapsed") === "true";
> +        let wasCollapsed = mutation.oldValue === "true";
Since both sides of the equality test are strings I think we can use double equals == here.

> +        window.resizeTo(window.outerWidth, this._height);
I'm confused. Shouldn't the resizeTo() be outside the for loop?

> +    this._mutationObserver.observe(document,
> +                                   { subtree: true,
> +                                     attributeOldValue: true,
> +                                     attributeFilter: ["collapsed"] });
Technically /attributeOldValue/ implies /attributes/ however I feel we should explicitly specify |attributes: true| here.

> +  // gEditItemOverlay does not exist anymore here, so don't rely on it.
> +    this._mutationObserver.disconnect();
> +    delete this._mutationObserver;
> +
>      window.removeEventListener("resize", this, false);
>      // gEditItemOverlay does not exist anymore here, so don't rely on it.
This comment is redundant as you have added a copy a few lines up.

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 1249536-SetDefaultHeight-V3.patch

>>  +# Provide another URI for the bookmark properties dialog so we can persist the attributes separately
>>  +% override chrome://communicator/content/bookmarks/bm-props2.xul chrome://communicator/content/bookmarks/bm-props.xul

Hmm not sure if this is effective. Same for the previous version. Could you check if you see a difference or if I did something wrong? Width didn't seem to change with or without folder picker.

>> > +        window.resizeTo(window.outerWidth, this._height);
>> I'm confused. Shouldn't the resizeTo() be outside the for loop?

Straight copy from the Firefox patch but you are right. Previously the height was potentially adjusted multiple times in the loop but the end result was the ok. I moved it outside the loop and the height is obviously also set correctly. Maybe resulting in a small performance gain if we didn't miss something not so obvious.

Comment 21

[Philip Chee]

Comment on attachment 8762187 [details] [diff] [review]
1249536-SetDefaultHeight-V3.patch

r=me a=me

>>>  +# Provide another URI for the bookmark properties dialog so we can persist the attributes separately
>>>  +% override chrome://communicator/content/bookmarks/bm-props2.xul chrome://communicator/content/bookmarks/bm-props.xul
> 
> Hmm not sure if this is effective. Same for the previous version. Could you
> check if you see a difference or if I did something wrong? Width didn't seem
> to change with or without folder picker.
Lets get this patch checked in while we investigate this.

Comment 22

[Frank-Rainer Grahl (:frg)]

>> Lets get this patch checked in while we investigate this.

Oki. Reading my previous comment I noticed that it was ambiguous. The width can be changed and will be retained. It is just then the same for windows with and without a folder picker.

Comment 23

[Frank-Rainer Grahl (:frg)]

Attachment: 1249536-SetDefaultHeight-V3a.patch

I accidently left the line with the bm-pros2.xul in addition to the override in the jar.mn file and also added a blank line with trailing spaces. Corrected and retested.

r+ and a+ from Philip Chee carried forward.

Comment 24

[Frank-Rainer Grahl (:frg)]

 https://hg.mozilla.org/comm-central/rev/e5d210ca24bf

Comment 25

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8763134 [details] [diff] [review]
1249536-SetDefaultHeight-V3a.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Broken bookmark save feature. Major usability problem.
Testing completed (on m-c, etc.): c-r to c-c
Risk to taking this patch (and alternatives if risky): none. Already broken.
String changes made by this patch: none

Comment 26

[Frank-Rainer Grahl (:frg)]

Would also like to get approval for comm-esr45 in case someone builds from this tree.

Comment 27

[Philip Chee]

Comment on attachment 8763134 [details] [diff] [review]
1249536-SetDefaultHeight-V3a.patch

(In reply to Frank-Rainer Grahl from comment #26)
> Would also like to get approval for comm-esr45 in case someone builds from
> this tree.
a=me assuming it applies cleanly

Comment 28

[Frank-Rainer Grahl (:frg)]

All well which ends well:

https://hg.mozilla.org/releases/comm-aurora/rev/8892b33a201f
https://hg.mozilla.org/releases/comm-beta/rev/25c8845e04c9
https://hg.mozilla.org/releases/comm-release/rev/675872ac277f
https://hg.mozilla.org/releases/comm-esr45/rev/fec65a59c34c

Comment 29

[Rainer Bielefeld]

Please see Bug 1293618 comment #38