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

RESOLVED FIXED in seamonkey2.47

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RainerBielefeldNG, Assigned: frg)

Tracking

(Blocks 1 bug, {regression})

SeaMonkey 2.40 Branch
seamonkey2.47
Unspecified
All
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.43 wontfix, seamonkey2.44 fixed, seamonkey2.45 fixed, seamonkey2.46 fixed, seamonkey2.47 fixed)

Details

(Whiteboard: [easyconfirm])

User Story

Workaround: See Bug 1258141 "userChrome.css"

Attachments

(4 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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
Reporter

Comment 1

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

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

3 years ago
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
Reporter

Comment 4

3 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Updated

3 years ago
Duplicate of this bug: 1258141
Reporter

Updated

3 years ago
User Story: (updated)

Comment 6

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

Updated

3 years ago
Duplicate of this bug: 1258608
Reporter

Comment 8

3 years ago
Version due to comment 6, DUP and own research
OS due to Bug 1258141
OS: Windows 7 → All
Version: SeaMonkey 2.42 Branch → SeaMonkey 2.40 Branch

Comment 9

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

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

3 years ago
Noticed the same problem as Comment 9 since upgrading to seamonkey 2.40 from 2.26
Assignee

Updated

3 years ago
Duplicate of this bug: 1274014
Assignee

Comment 13

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

Comment 14

3 years ago
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.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8755212 - Flags: review?(philip.chee)

Comment 15

3 years ago
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.
Attachment #8755212 - Flags: review?(philip.chee) → review-
Assignee

Comment 16

3 years ago
>> 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
Assignee

Comment 17

3 years ago
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.
Attachment #8755212 - Attachment is obsolete: true
Attachment #8757699 - Flags: review?(philip.chee)

Comment 18

3 years ago
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.
Attachment #8757699 - Flags: review?(philip.chee) → review-
Assignee

Comment 19

3 years ago
>>  +# 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.
Attachment #8757699 - Attachment is obsolete: true
Attachment #8762187 - Flags: review?(philip.chee)
Assignee

Updated

3 years ago
Duplicate of this bug: 1279747

Comment 21

3 years ago
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.
Attachment #8762187 - Flags: review?(philip.chee) → review+
Assignee

Comment 22

3 years ago
>> 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.
Assignee

Comment 23

3 years ago
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.
Attachment #8762187 - Attachment is obsolete: true
Attachment #8763134 - Flags: review+
Assignee

Comment 25

3 years ago
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
Attachment #8763134 - Flags: approval-comm-release?
Attachment #8763134 - Flags: approval-comm-beta?
Attachment #8763134 - Flags: approval-comm-aurora?
Assignee

Comment 26

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

Comment 27

3 years ago
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
Attachment #8763134 - Flags: approval-comm-release?
Attachment #8763134 - Flags: approval-comm-release+
Attachment #8763134 - Flags: approval-comm-beta?
Attachment #8763134 - Flags: approval-comm-beta+
Attachment #8763134 - Flags: approval-comm-aurora?
Attachment #8763134 - Flags: approval-comm-aurora+
Assignee

Updated

3 years ago
Blocks: 1280522
Reporter

Updated

3 years ago
Target Milestone: --- → seamonkey2.44

Updated

3 years ago
Target Milestone: seamonkey2.44 → seamonkey2.47
Reporter

Comment 29

3 years ago
Please see Bug 1293618 comment #38
Target Milestone: seamonkey2.47 → seamonkey2.44

Updated

3 years ago
Target Milestone: seamonkey2.44 → seamonkey2.47
You need to log in before you can comment on or make changes to this bug.