120821 - [viewpoint] Plugin paints in the wrong part of the screen when on a Div tag

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    .0.9.4.2

As a response to bug 114921, Andrei passes the drawing rectangle as the lparam 
in paint messages.
On windows XP only, the rect is wrong when the content is in a div tag.
for the content above, here is the correct(top, left) - this is what I get on 
Win2k or win9x:
(40,282)
On WinXP:
(80,564)
Note that the coordinates are doubled- are you adding the origin twice?
the height and width are correct.
I am setting up Mozilla source on an XP machine- that may give more information 
on the cause.

Reproducible: Always
Steps to Reproduce:
1.Install viewpoint plugin (http://cole.viewpoint.com/~aberger/readme.txt)
2.view above URL
3.The content is clearly in the wrong spot (compare to 2K or 9x).

Comment 1

[Ari Berger]

Update- QA is able to replace this on one or 2 Win2K machines.  Not usually, 
but on a few.

Comment 2

[Peter Lubczynski]

On my XP system, I am seeing we are adding to |origin| twice:

Once here (4320, 600):
    // parent view may be scrolled
    parentWithView->GetPosition(&correction.x, &correction.y);
    origin += correction;

Then again here (8460, 1200):
    // offset from parent frame
    nsIFrame* parentFrame;
    GetParentWithView(aPresContext, &parentFrame);
    if (parentFrame) {
      correction = nsPoint(0, 0);
      parentFrame->GetOffsetFromView(aPresContext, correction, &parentWithView);
      origin += correction;

Comment 3

[Ari Berger]

This seems to be a regression- I don't have exact dates, but it looks like it 
worked around 1/10 and broke by 1/15.

Comment 4

[Peter Lubczynski]

Ok, that's good to know. --->peterl I'm looking at this and see the problem on
my WinXP system. I'm trying to get W2K to compare. I'll also try backing out
fixes to the 10th.

Comment 5

[Ari Berger]

I updated my source yesterday, and the bug showed up even on my 2K machine.  I 
think it is probably broken on 2K also- I would say that it works rarely on 2K, 
never on XP.
It would probably be worth comparing to 9x more than 2K to see the correct 
behavior.

Comment 6

[Peter Lubczynski]

fixing url for testcase

Comment 7

[Peter Lubczynski]

Ari, 
Can you execute this command (in the parent of mozilla) to back out changes and
let me know if it fixes the coordinates for you:
cvs update -j1.248.2.21 -j1.248.2.20 mozilla/layout/html/base/src/nsObjectFrame.cpp
...after backing that out on my WinXP debug 0.9.4 branch build, I get the right
coordinates.

That patch was checked in on the 14th by Andrei from bug 110094 to fix scrolling
of windowless plugins.

Comment 8

[av (gone)]

You may just comment out the following lines of code:

  // parent view may be scrolled
  parentWithView->GetPosition(&correction.x, &correction.y);
  origin += correction;

That's what was essentially added. I tested this on NT4. I guess layout gives 
different values on one of the following |GetOffsets| on different OS. Wondering 
why.

Comment 9

[Michael Dunn]

Other notes:
http://bugscape.mcom.com/show_bug.cgi?id=11858

Comment 10

[Peter Lubczynski]

I am seeing this on NT 4, W2K, and WinXP. In all cases, the image is shifted
over to the right instead of on the yellow dot. I always get (80,564) from
SetWindow.

Ari, can you just comment out the lines as suggested by Andrei in comment #8?

Comment 11

[Peter Lubczynski]

cc:ing some layout folks...

The DIV in question in the testcase is absolutly positioned. This is probably
causing us to incorrectly calculate the offset from the view.

Comment 12

[Peter Lubczynski]

Attachment: possible patch

This patch attempts to fix the over correction for scrolling in the case of an
absolutely positioned parent. Please try out and review.

Comment 13

[Ari Berger]

No good. Patch fixes the reported problem, but re-breaks scrolling.
With the patch applied, view this:
http://cole.viewpoint.com/~ddavies/lego
Make your window small enough that a scrollbar appears.  No good. Tthis was the 
original bug (110094) that av fixed.

Comment 14

[Peter Lubczynski]

Attachment: better patch

I think this patch may do the trick. Both the "catalog" and "lego" viewpoint
testcases work nicely. Please try it out and review.

The problem was that we were assuming that our parent's view is always the
scrolling view. That's not always the case, as with absolute positioning.
Instead, I now walk up the view tree until I find the right kind of view and
then add its offset.

Comment 15

[Ari Berger]

Oh so close.
The patch does make us display correctly on the catalog (absolute div position) 
and on lego (no div).
However, while this time we scroll correctly on lego, we don't on div.
Try making the catalog patch small enough that vertical scrolls appear and try 
scrolling. clearly wrong.

Comment 16

[Michael Dunn]

Any ETA on this bug?

Comment 17

[Peter Lubczynski]

Attachment: patch, v.3

Here's another patch that may do the trick. Maybe 3rd time is the charm?

Kevin suggested simply walking up the views to sum up our absolute origin.
That's what I do in this patch and I can scroll in both testcases. Please try
this patch out.

Comment 18

[Ari Berger]

Confirmed.  Bug I saw is in our current (non-shipping) scene code.  Fix looks 
great! Peter- sorry for scaring you :)

Comment 19

[Kevin McCluskey (gone)]

Comment on attachment 66331 [details] [diff] [review]
patch, v.3

patch looks good
r=[e-mail]

Comment 20

[Michael Dunn]

Plussing for Checkin. 

Comment 21

[Michael Dunn]

Need super review on this and then I can land right away. I sent mail
yesterday to [e-mail] and Patrick but I have not heard back yet.
Maybe today?

Comment 22

[Patrick C. Beard]

Comment on attachment 66331 [details] [diff] [review]
patch, v.3

sr=beard

Comment 23

[Peter Lubczynski]

fixed0.9.4

Comment 24

[shrirang khanzode]

fix is working fine on win xP with build 01/28(0.9.4).  

Comment 25

[Peter Lubczynski]

patch in trunk

Comment 26

[shrirang khanzode]

works great now with latest viewpoint plugin on trunk (0812).Verif