Zooming to fit contents fails miserably

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
General
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jonathan Thurlow, Assigned: Torben)

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1
Bug Flags:
camino1.1a2 +

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

207 bytes, text/html
Details
3.46 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 245832 [details]
Reduced testcase

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

Comment 2

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

11 years ago
Let's dupe it, then.

*** This bug has been marked as a duplicate of 360880 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
(Assignee)

Comment 4

11 years ago
Reopening, this is not bug 360880, the symptoms are opposite.
Status: VERIFIED → UNCONFIRMED
Resolution: DUPLICATE → ---
(Assignee)

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac System 9.x → Mac OS X 10.2
Target Milestone: --- → Camino1.1
Component: Toolbars & Menus → General
QA Contact: toolbars → general
(Assignee)

Comment 5

11 years ago
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)
(Assignee)

Comment 6

11 years ago
Created attachment 248497 [details] [diff] [review]
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.
Assignee: nobody → torben
Status: NEW → ASSIGNED
Attachment #248497 - Flags: review?(stuart.morgan)
(Assignee)

Comment 7

11 years ago
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.  
Flags: camino1.1a2?

Comment 8

11 years ago
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.
Attachment #248497 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 9

11 years ago
Created attachment 248689 [details] [diff] [review]
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.
Attachment #248497 - Attachment is obsolete: true
Attachment #248689 - Flags: review?(stuart.morgan)
(Assignee)

Comment 10

11 years ago
(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.
Blocks: 360878

Comment 11

11 years ago
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.
Attachment #248689 - Flags: superreview?(mikepinkerton)
Attachment #248689 - Flags: review?(stuart.morgan)
Attachment #248689 - Flags: review+
Comment on attachment 248689 [details] [diff] [review]
Minimum fix, for this bug and bug 360878.

rs=pink
Attachment #248689 - Flags: superreview?(mikepinkerton) → superreview+

Comment 13

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Flags: camino1.1a2? → camino1.1a2+
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.