162885 - scrolling shears on 10.2 in ppembed

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: Mac, defect)

Description

[Mike Pinkerton (not reading bugmail)]

- go to cnn.com in PPEmbed (8/15/02)
- scroll down, then up

notice the page "shears" and the left half doesn't scroll where the right half
does. seamonkey doesn't have this problem.

screenshot upcoming

Comment 1

[Mike Pinkerton (not reading bugmail)]

Attachment: shows shearing after scroll

Comment 2

[sairuh (rarely reading bugmail)]

fwiw, i couldn't repro this with two chimera builds: the 2002.08.15.05 nightly,
and bryner's 1.0.1-sync build from last night. (tested w/spacebar, arrow keys
and pageUp/Down --no scrollwheel attached to the jaguar machine at hand.) not
sure how/when/if chimera would be affected by this, tho'...

Comment 3

[Mike Pinkerton (not reading bugmail)]

i don't expect chimera to be affected. it's using different widget code entirely.

Comment 4

[moconnell]

A piece of the puzzle. This does not occur on CNN.com when you disable 
JavaScript. 

Perhaps CNN serves different content for javascript disabled browsers?

Comment 5

[Michael Dunn]

http://bugscape.netscape.com/show_bug.cgi?id=19249

Comment 6

[Mike Pinkerton (not reading bugmail)]

i'll take it

Comment 7

[Brian Nesse (gone)]

Sorry for the stupid question, but it's not specifically stated here... Does
this only occur on 10.2?

Comment 8

[Mike Pinkerton (not reading bugmail)]

can someone in qa get a reduced test-case. it's obvious (from turning off JS)
that it is content dependent. that might help us narrow it down. petersen? can
you jump on this? it would be greatly appreciated!

Comment 9

[Mazie Lobo]

I could not reproduce the bug with 10.1.5.

Comment 10

[Mike Pinkerton (not reading bugmail)]

i know why this is happening! oh happy day!

 in PPembed, we create a 32000 x 32000 widget as the toplevel. when we call
OffsetRgn() in nsWindow::CalcWindowRegion(), the call silently fails because for
some pages the region would get pushed over the 32767 limit. Then when we try to
restore the region's previous location, we end up ofsetting it and the shearing
occurs.

my best guess at a fix is two-fold:
- copy and restore the region rather than offset the region twice. this will
prevent us from ending up with a bad offset in the failure case
- don't have a toplevel widget of 32k x 32k.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Attachment: fixes for offset woes

patch consists of two parts:

1) copy/restore region so even if OffsetRgn silently fails, we're still ok
2) don't create such a monumental main widget in embedding. ppembed pins the
max size of windows to the grayRgn already, so this is a reasonable size and
gives us a lot of room offset-wise to play before we hit the 32k limit.

note that (1) by itself fixes the shearing on cnn.com, but i'm not sure what
other side effects it might cause that aren't immediately visible. Very long
pages with a widget near the bottom (such as an iframe or a plugin) will still
run into this problem of bad offsets, but with (1) any issues that may arise
won't show themselves as shearing.

Comment 12

[Conrad Carlen (not reading bugmail)]

Comment on attachment 96462 [details] [diff] [review]
fixes for offset woes

+    nsRect r(0, 0, abs(grayRect.right - grayRect.left), abs(grayRect.bottom -
grayRect.top));

I don't think the abs is needed here. If so, this would be an empty rect.
without the abs(), r=ccarlen

Comment 13

[Simon Fraser [no longer active]]

+      StRegionFromPool originalWindowRgn;
+      if ( !originalWindowRgn )
+        return;
+      ::CopyRgn(parent->mWindowRegion, originalWindowRgn); 
+      ::OffsetRgn(parent->mWindowRegion, origin.x, origin.y);
+      ::SectRgn(mWindowRegion, parent->mWindowRegion, mWindowRegion);
+      ::CopyRgn(originalWindowRgn, parent->mWindowRegion);

Why not:

      StRegionFromPool parentShiftedWindowRgn;
      if ( parentShiftedWindowRgn )
        return;
      ::CopyRgn(parent->mWindowRegion, parentShiftedWindowRgn); 
      ::OffsetRgn(parentShiftedWindowRgn, origin.x, origin.y);
      ::SectRgn(mWindowRegion, parentShiftedWindowRgn, mWindowRegion);

so you just shift a copy of the parent? Then, there is no problem shifting it back.

Also agree with the abs() removal.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Attachment: updated patch to simplify

updated for smfr's comments. ignore the odd spacing, the surrounding code has
tabs.

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 96483 [details] [diff] [review]
updated patch to simplify

sr=sfraser, tho maybe 
'shiftedParentWindowRgn' is a better name.

Comment 16

[Steve Dagley]

Comment on attachment 96483 [details] [diff] [review]
updated patch to simplify

r=sdagley

Comment 17

[Mike Pinkerton (not reading bugmail)]

landed on trunk. holding open for 101 branch

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

It's too late for 1.0.1; retargeting for 1.0.2.  Once 1.0.1 is kicked out the
door, we'll take this asap for 1.0.2.  If an embedder needs this, they can
install it in their tree for now.

Comment 19

[chris hofmann]

Comment on attachment 96483 [details] [diff] [review]
updated patch to simplify

a=chofmann for 1.0.2

Comment 20

[chris hofmann]

Comment on attachment 96483 [details] [diff] [review]
updated patch to simplify

a=chofmann for 1.0.2

Comment 21

[Mike Pinkerton (not reading bugmail)]

landed on the 1.0 branch.

Comment 22

[Jaime Rodriguez, Jr.]

mdunn: pls verify this as fixed on the 1.0 branch, then replace the "fixed1.0.2"
keyword, with "verified1.0.2". thanks!

Comment 23

[Gayatri Rimola]

Please verify the bug. Once verified, change the keyword fixed1.0.2 to
verified1.0.2 

Comment 24

[lchiang]

Michael - need verification of this.  Pls reassign.

Comment 25

[bmartin]

Marking Verified Fixed.

Unable to reproduce. Tested against today's PPEmbed 20020924 1.0 branch

Comment 26

[Michael Dunn]

Brent... To be clear you tested against OSX 10.2 (Jaguar)?
Petersen do you have an OSX 10.2 install?

Comment 27

[Chris Petersen]

Yes, I have 10.2.1 installed. I will pull the same PPembed build as Brent and
try it.

Comment 28

[Chris Petersen]

Confirming the problem has been resolved in the 2002-09-24-05 PPembed branch build.