361049 - Zooming to fit contents fails miserably

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Jonathan Thurlow]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20061115 Camino/1.1a1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20061115 Camino/1.1a1+

On www.pcworld.co.uk, clicking the green button to zoom to fit contents results in a normal-width but approx 10px-high content area.

Reproducible: Always

Steps to Reproduce:
1. Go to www.pcworld.co.uk
2. Click green button at top of window to fit window to contents (in branch build 20061115)

Actual Results:  
As described above

Expected Results:  
Resizes to fit contents like Safari does.

Comment 1

[Jonathan Thurlow]

Attachment: Reduced testcase

Attached testcase with fairly minimal content; hopefully someone can see what the problem is.

Comment 2

[philippe (part-time)]

Yeah, that is the same problem I was talking about in bug 360880.
Document in Quirksmode.
All content in an absolute positioned div.

(Thanks for the testcase, btw)

Comment 3

[Chris Lawson (gone)]

Let's dupe it, then.

*** This bug has been marked as a duplicate of 360880 ***

Comment 4

[Torben]

Reopening, this is not bug 360880, the symptoms are opposite.

Comment 5

[Torben]

Does anyone have more real world examples where this happens? I think the best short term solution is to err[*] if we get a height less than a reasonable minimum. A real fix depends on bug 360765.

[*] ATM this means maximizing to screen, I'm experiencing with other alternatives
    (for bug 360880)

Comment 6

[Torben]

Attachment: Min allowed height is now 20 px

Made us enter error state if we get a height that is less than 20. To not get problems with scrollbars, it uses innerWidth rather than clientWidth in these situations. This also changes the error-catching code to only maximize in the direction that fail. I've rearranged some of the code to make it somewhat cleaner therefor this patch also contains the changes suggested for bug 360878.

Comment 7

[Torben]

As Smokey said in the other bug since a2 will be the first time we ship with zoom, it'd be nice to get some of these zoom bugs fixed instead of shipping a partially busted impl.  

Comment 8

[Stuart Morgan]

Comment on attachment 248497 [details] [diff] [review]
Min allowed height is now 20 px

> I've rearranged some of the code to make it somewhat
> cleaner therefor this patch also contains the changes suggested for bug 360878.

If this replaces the patch in bug 360878, you should remove the review requests over there; we don't want two conflicting patches in the queue at the same time.

> +  // Get the current size of the window and the content and the new size of the content.
> +  NSRect stdFrame     = [[self window] frame];
> +  NSSize curFrameSize = [[mBrowserView getBrowserView] frame].size;

The comment is inaccurate (it's the size of the content area, not the window) and the name |curFrameSize| needs to be less vague (and ideally not abbreviated).

>+    // Change the window size, but don't let it be to narrow

s/to/too/

>+    stdFrame.size.width = MAX([[self window] minSize].width, stdFrame.size.width + widthChange);

I know this is not new code, but where is MAX defined (and why are we using it instead of something like fmaxf)?

>+  // Now the height.
>+  if (contentHeight <= 0) {
>+    // something went wrong, use the screen's maximal height and return.
>+    stdFrame.size.height = defaultFrame.size.height;
>+    stdFrame.origin.y    = defaultFrame.origin.y;
>+    [self setZoomState:stdFrame defaultFrame:defaultFrame];
>+    return stdFrame;
>+  }

Why does this case need to trigger an early return, rather than just setting the change to something that will maximize and then keep going?

>+  // Hackish workaround for bug 361049
>+  const PRInt32 kMinSaneHeight = 20;
>+  if (*outHeight < kMinSaneHeight) {
>+    domWindow->GetInnerWidth(outWidth);
>+    *outHeight = 0;
>+  }

I think this hack should be in the zoom code (checking the height against kMinSaneHeight instead of 0 when deciding whether to trust it), rather than making this function lie about the intrinsic size.

Comment 9

[Torben]

Attachment: Minimum fix, for this bug and bug 360878.

I'll be busy with RL for a week or two but IMHO this is what really needs to be done for a2, the behavior change for errors can probably wait.

Comment 10

[Torben]

(In reply to comment #8)
This might as well be done in bug 360880 but putting it here so it don't get lost.
> > +  // Get the current size of the window and the content and the new size 
> >       of the content.
> > +  NSRect stdFrame     = [[self window] frame];
> > +  NSSize curFrameSize = [[mBrowserView getBrowserView] frame].size;
> 
> The comment is inaccurate (it's the size of the content area, not the window)
> and the name |curFrameSize| needs to be less vague (and ideally not
> abbreviated).

The first line ( [[self window] frame] ) is the size of the window but, yeah, 
the comment could be clearer.

> >+    stdFrame.size.width = MAX([[self window] minSize].width, 
> >                               stdFrame.size.width + widthChange);
> 
> I know this is not new code, but where is MAX defined (and why are we using 
> it instead of something like fmaxf)?

I have no idea, I originally used PR_MAX but Håkan wanted MAX (bug 155956 
comment 96).

> >+  // Now the height.
> >+  if (contentHeight <= 0) {
> >+    // something went wrong, use the screen's maximal height and return.
> >+    stdFrame.size.height = defaultFrame.size.height;
> >+    stdFrame.origin.y    = defaultFrame.origin.y;
> >+    [self setZoomState:stdFrame defaultFrame:defaultFrame];
> >+    return stdFrame;
> >+  }
> 
> Why does this case need to trigger an early return, rather than just setting
> the change to something that will maximize and then keep going?

It doesn't need to return early but the rest of the code won't do anything 
either. However, an else-clause is probably cleaner though.
 
> >+  // Hackish workaround for bug 361049
> >+  const PRInt32 kMinSaneHeight = 20;
> >+  if (*outHeight < kMinSaneHeight) {
> >+    domWindow->GetInnerWidth(outWidth);
> >+    *outHeight = 0;
> >+  }
> 
> I think this hack should be in the zoom code (checking the height against
> kMinSaneHeight instead of 0 when deciding whether to trust it), rather than
> making this function lie about the intrinsic size.

The problem is that we need to change outWidth too and I didn't want to 
declare kMinSaneHeight twice. I'll see what I can do.

Comment 11

[Stuart Morgan]

Comment on attachment 248689 [details] [diff] [review]
Minimum fix, for this bug and bug 360878.

r=me, but we need to point  the previous patch in bug 360880 so it's not lost.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248689 [details] [diff] [review]
Minimum fix, for this bug and bug 360878.

rs=pink

Comment 13

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.