Closed Bug 227951 Opened 21 years ago Closed 20 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: athakur, Assigned: p_ch)

References

(Depends on 1 open bug)

Details

Attachments

(1 obsolete file)

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
Confirmed

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031209 Firebird/0.7+
I can confirm this as well.

http://bugzilla.mozilla.org/show_bug.cgi?id=227951
As Pierre pointed out in bug 214527, this may be caused by bug 90276.
Depends on: 90276
OS: Windows 2000 → All
Hardware: PC → All
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.
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.
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.
Flags: blocking0.8?
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.
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.
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:.
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.
Blocks: 228128
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.
Attached patch remove the workaround (obsolete) — Splinter Review
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.
CCing mkaply because I think he looked at this for bug 123207.
Attachment #137415 - Flags: superreview?(dbaron)
Attachment #137415 - Flags: review?(danm-moz)
not sure if it is of value, but bug 80530 has been fixed after that the
workaround has been introduced.
Attachment #137415 - Flags: superreview?(dbaron) → superreview+
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.
approved for 0.8 with danm's review. 
Flags: blocking0.8? → blocking0.8+
Target Milestone: --- → Firebird0.8
brendan sez danm out of town. maybe bz can review. 
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.
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.
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?
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.
How about if for 0.8 we just remove the arrow and always have the tree view
visible and fix this for 0.9?
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.
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.
doesn't happen here at all here.
*** Bug 228995 has been marked as a duplicate of this bug. ***
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.
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.
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.
Target Milestone: Firebird0.8 → ---
Flags: blocking0.8+ → blocking0.8-
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.
Attachment #137415 - Flags: review?(danm-moz) → review+
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.
*** Bug 231139 has been marked as a duplicate of this bug. ***
WFM
Firefox 0.8 (20040209) on XP
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.
Attachment #137415 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.