227951 - New "Add Bookmarks" window grows in size each time arrow button is hit

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[athakur]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209 Firebird/0.7+ (scragz)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209 Firebird/0.7+ (scragz)

Each time the arrow button is hit, the window grows in size.  Since the window
size is stored between session, the window will continue to get bigger and bigger.

The profile must be recreated or editted in order to shrink the window back down.

Reproducible: Always

Steps to Reproduce:
1. Bring up "Add Bookmark" dialog (CTRL-D)
2. Hit the arrow next to the bookmark name
3. Hit the arrow again
4. Continue hitting the arrow

Actual Results:  
The window grows in size each time the arrow is hit

Expected Results:  
The window should not grow in size or at least the user should be able to easily
resize it back to a normal size

Comment 1

[Sean Umphlet (nosebleed)]

Confirmed

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031209 Firebird/0.7+

Comment 2

[rnvarma]

I can confirm this as well.

http://bugzilla.mozilla.org/show_bug.cgi?id=227951

Comment 3

[Jon Henry]

As Pierre pointed out in bug 214527, this may be caused by bug 90276.

Comment 4

[Pierre Chanial]

some investigations lead me to suspect the "width+1" hack (bug 68352 comment 13)
in DocumentViewerImpl::SizeToContent around a pixel rounding error.

In a condensed form, this is what sizeToContent does:

presShell->ResizeReflow(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE)
presContext->GetVisibleArea(shellArea);
presContext->GetTwipsToPixels(&pixelScale);
width = PRInt32((float)shellArea.width*pixelScale);
height = PRInt32((float)shellArea.height*pixelScale);
treeOwner->SizeShellTo(docShellAsItem, width+1, height)

Maybe the rounding error has been corrected since?
I'll do more testing by just using "width" this evening, when I'll have a build
environment.

cc'ing a bunch of layout folks for their insigths.

Comment 5

[Pierre Chanial]

on my linux box, the window grows by one pixel everytime the expander button is
clicked.
I checked on windows ME. And the problem is really worse, sth like 6-9 pixels,
then the problem is not (only) the pixel rounding stuff.

Comment 6

[Andrew Keyser]

Windows XP here, same results as on your WinME test. I may also mention the
maximum width is 1036 pixels. When it reaches that point, it won't grow any larger.

Comment 7

[amano]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031210
Firebird/0.7+ (Nova: MNG,DOMi)

I want to add, that if reaches 1036 pixels and I close the dialog, it will jump
back to 1024 pixels again (I use 1024x768 resolution). If I click on the button
again it will jump to 1036 pixel width.

Now if I switch to 800x600 resolution and open the dialog, the dialog comes up
with 800x600 width. If I click on the arrow it is 812pixels and stays there.

Back to 1024x768: The dialog comes up at 800x600 and keeps growing again if I
hit the arrow. I hope that helps.

Comment 8

[Dan M]

WFM with WinXP Firebird trunk build using source from yesterday morning
(20031210). The bookmarks window expands once, the first time I click the arrow
button. But then it stays at that size. Note the build date, which means I'm
using the original, unsizable (bug 228040 not checked in) dialog this bug was
filed against.

Despite the window growing by more than one pixel on some builds (comment 5) I
think it'd still be worthwhile to try taking the +1 rounding compensation out
(comment 4) and see what happens. That was added to fix a pervasive problem in
dialog sizing. You should notice quickly whether its absence causes any harm. In
particular, the +1 was checked in to fix bug 68352 and bug 77639 ("and others").
On my build, the +1 doesn't seem to be necessary any longer.

Comment 9

[Bill Gianopoulos [:WG9s]]

re comment #8.  I see just the opposite.  The only time the window did not
expand when clicking on the arrow, was the very first time I clicked on it the
first time I started a browser with a build with the new bookmark manager. 
Every time after that it expands on clicking.  It also expands by more than 1
pixel.  I am running with 1024x768 resolution and it expands by approximately
1/2 the width of the browser window verticle scrollbar (not sure how wide that
is).  This is all with the default theme running the 20031211 offical win32 zip
build nightly on a WINXP system.

Just to make sure we are all on the same page on this, we are talking about the
arrow in a seperate box next to the Name:.  NOT the arrow for the pull-down on
the Create in:.

Comment 10

[Bill Gianopoulos [:WG9s]]

This looks more like it is drawing something too close to the window edge and
then the resize to fit widens the window to give a margin.

Comment 11

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031212 Firebird/0.7+
(daihard; XFT+GTK2; optimized for P4/SSE-2)

I can readily reproduce this bug by loading the Add Bookmarks window via the
context menu (Ctrl-D strangely doesn't work), and repeatedly clicking the arrow
six or seven times. This is with Aaron Spuler's Smoke theme.

Comment 12

[Pierre Chanial]

Attachment: remove the workaround

this patch removes the width+1 workaround.
I've run it for few days in my debug build, it removes the 1 pixed growth on
linux without apparently introducing problems in dialogs. My testing is however
limited to Mozilla Firebird, and I haven't tested it on Windows. Even if I
don't think it fixes the problem and the latter platform, I would be glad to
have some feedback if someone could build Firebird on Windows and testing the
suite with this patch.

Comment 13

[[e-mail]]

CCing mkaply because I think he looked at this for bug 123207.

Comment 14

[Pierre Chanial]

not sure if it is of value, but bug 80530 has been fixed after that the
workaround has been introduced.

Comment 15

[Bill Gianopoulos [:WG9s]]

Well, although this patch is probably corect, it does not solve the issue under
windows.  Even with this applied I still see the window growing by around 5 or
six pixels in width eash time I click on the arrow.

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

approved for 0.8 with danm's review. 

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

brendan sez danm out of town. maybe bz can review. 

Comment 18

[Boris Zbarsky [:bzbarsky]]

I've never seen this code before, so I'd like to know what situations the
original code was addressing (bug numbers?) and whether those have been tested.

Comment 19

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031217 Firebird/0.7+
(daihard; XFT+GTK2; optimized for P4/SSE-2)

This bug is still present.

Comment 20

[Koobs]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031216
Firebird/0.7+

Can confirm bug in Win XP SP1 + PxClassic Theme, although with two caveats. The
'arrow' in question is not an arrow in my 'Add Bookmark Dialog' but rather a
command button with no label/text in it whatsoever. Secondly, my 'Add Bookmark'
dialog is resizable, hence after clicking a few times, i can just resize it
down, and this size is saved when opening the dialog again.

Anyone else with the same behaviour?

Comment 21

[Jon Henry]

I think it's understood that the bug still exists, that's why it isn't marked
fixed yet ;). Pierre recently checked in code that made the box resizable, and
as for the no arrow issue, do you see that with the default theme too or just
PxClassic? I would guess the theme hasn't been updated to deal with the new
button yet.

Comment 22

[Bill Gianopoulos [:WG9s]]

How about if for 0.8 we just remove the arrow and always have the tree view
visible and fix this for 0.9?

Comment 23

[Michaël Arnauts]

i would suggest to just remove the button like William A. Gianopoulos says in
#22, but i also don't see why that small "create in" box is there? isn't it
waaay simpler for users to have only 1 box? i personally (but this is my
opinion) would like it better if the button is removed and the small "create in"
also, so you only have the name box and the large treeview-box.

Comment 24

[Mike Connor [:mconnor]]

bug 228852 has discussion on the treeview vs. the recent folders list.  We might
revisit tree vs. recent folders at some point, but theme issues are not going to
block getting some testing on this UI.

Treeview is less effective for a lot of users than the current UI, but we'll see
how user feedback is before changing anything.

Comment 25

[Mike Connor [:mconnor]]

sorry, bug 228582

Comment 26

[bluppfisk]

doesn't happen here at all here.

Comment 27

[Mike Connor [:mconnor]]

*** Bug 228995 has been marked as a duplicate of this bug. ***

Comment 28

[Bill Gianopoulos [:WG9s]]

Good news!

I just installed MozJF's build for 12/20 which includes this fix and no longer
have the problem.

So, I guess somehow when I created my own build with this fix in it (see comment
#15) I must have botched it somehow.  I think once an official branch build for
windows comes out with this and we can verify that it works we can mark this as
fixed.

Comment 29

[Bill Gianopoulos [:WG9s]]

Ah I see why it works now, but when I checked in the attached patch it didn;t
fix it.  Additional code was checked in as well.

Comment 30

[Pierre Chanial]

a workaround has been checked in in the trunk and in the 0.8 branch.
So, the growth is gone. We still need to figure out if the width+1 workaround is
needed or not so this bug should remain open but untargeting for 0.8.
Since it's hard to answer bz's legitimate question, because the dialogs that
were showing the problem solved by the width+1 workaround were not using
dialog.xml, I need to come up with a testcase.

Comment 31

[Dan M]

Comment on attachment 137415 [details] [diff] [review]
remove the workaround

See comment 8 for the reason for the +1 and the bugs it fixed. I've removed the
+1 and tested against the first of those bugs and noticed no problem. (Nor do I
see the this bug, 227951, for that matter.)

So the +1 is a hack, an old hack, and as far as I can tell it's no longer
necessary. Let's remove it and see what happens. r=danm.

Comment 32

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20031221 Firebird/0.7+
(daihard; XFT+GTK2; optimized for P4/SSE-2)

This is a ->0.8 build, and it no longer has the flaw.

Comment 33

[Jason Barnabe (np)]

*** Bug 231139 has been marked as a duplicate of this bug. ***

Comment 34

[Samir L. Boulema]

WFM
Firefox 0.8 (20040209) on XP

Comment 35

[Pierre Chanial]

Comment on attachment 137415 [details] [diff] [review]
remove the workaround

I forgot to obsolete this patch.
In fact I tested that the workaround is still needed, for the same reason it
was introduced. I will attach a working testcase hopefully this week.

Comment 36

[Pierre Chanial]

I think it's better to mark this bug as fixed rather than morph it, since a lot
of people cc'd themselves or have voted for a problem that doesn't show up
anymore. I'll open a new bug.

Comment 37

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Removing self from CC list.

Comment 38

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change