Closed Bug 118721 Opened 23 years ago Closed 22 years ago

windowless plugins render in the wrong place when in div tag

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows 2000
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: mozilla, Assigned: peterl-bugs)

Details

(Keywords: regression, testcase)

Attachments

(4 files)

Zip file containing test case and plugin
315.33 KB, application/x-zip-compressed
Details
Improved test case
333.34 KB, application/x-zip-compressed
Details
First attempt at a patch
1.04 KB, patch
peterlubczynski-bugs
: review+
roc
: superreview+
asa
: approval+
Details | Diff | Splinter Review
Updated tests
33.85 KB, application/x-zip-compressed
Details 
I think that this is due to the div tag, but I haven't verified this. This may 
be a regresssion introduced by one of the recent set of checkins for bug 
114921, bug 116093 and bug 116109 .

I'll attach a test case. To see the correct behaviour try the test case in 
0.9.7. In the current builds the plugin is offset down the page. SetWindow is 
getting the proper values, in this case x = 50, y = 50, width = 200, and height 
= 200 . I can't debug this further at the moment as my local build crashes on 
start-up.
Attached file Improved test case — 
Improved test case, this shows the bug better. When the plugin is initially
drawn it is in the wrong place. If the window is resized then it jumps to the
correct place.
Given that the x and y values that are passed to SetWindow never change (they
are always 50, 50) this implies that the DC we are initially given is for the
plugin area (so we should draw at 0,0 to get the correct plugin position),
after the resize we are instead given the DC for the div's layer and so should
then draw at (50, 50). I would have thought that the DC should always be for
the div's layer, and that worked in 0.9.7, any ideas as to what might have
changed to break this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
David, is this still happening?
Peter, I've just tested it in build 2002022003 and the problem still exists. I
think we might need someone from layout to help us on this one, can you cc
someone suitable.

Adding regression keyword as this problem didn't occur in 0.9.7.
Keywords: regression
adding keywords and I'll see if I can take a look. I'll also ask the layout
folks about any changes.

Maybe something changed in nsObjectFrame:Paint()?
Assignee: av → peterl
Keywords: mozilla1.0, nsbeta1
The nsObjectFrame:Paint()method is the same as in the 0.9.4 branch, so I don't
think the problem is there. I think the problem is with the DC that we get from
aRenderingContext. It is changing from initally being one that just represents
the plugin area, to after the resize representing the whole div tag.
Is anything changing with the views?
I have had a go at debugging this further. What I have tried so far is to find
why the device context changes after a resize. I put a breakpoint on: 

aRenderingContext.RetrieveCurrentNativeGraphicData(&hdc);

in nsObjectFrame::Paint and stepped into this method then noted the this pointer.

During the plugins first load and paint we get passed rendering contexts with
four different addresses (in this order) in my case they were: 0x03dd8750,
0x03c9e9e8, 0x02beccd8, 0x03cf8678, so the final one that we draw to is 0x03cf8678.

After the resize the rendering context we are passed is 0x03c9e9e8, which is the
second one in the list above. I can't work out where this difference comes from
though and why we get passed all those different contexts as we are being
loaded. I'll try again when I have some more time.
-->mozilla1.0

Strange behavior with the RenderingContext. Kevin, do you know why it changes
after a resize?
Priority: -- → P2
Target Milestone: --- → mozilla1.0
Okay, I've got a bit further in understanding this. It's down to Invalidation.
During the load process, our plugin is calling InvalidateRect and passing in the
whole of its area. So, the first DC we get is the whole of the background and
the plugin draws in the right place. The subsequent DCs are just for the
invalidated regions, however the plugin still draws at the same offset into the
DC. So, it gets drawn in the wrong place. You get equally bad behaviour if you
partially invalidate the screen by dragging a window over part of it. After a
resize, we get the DC for the whole window again and draw in the right place.
nominating to nsbeta1+ as per ADT triage.
Keywords: nsbeta1 → nsbeta1+
I'm not sure what the correct behaviour should be here. This is something that
never happened in 4.x, I guess Mozilla's drawing code is better optimised.

There is no way of telling within the plugin what the origin of the DC is. I
guess the only way of doing this is using the window.x, and window.y parameters
that are passed through SetWindow. This would mean that these parameters could
be negative. Here is an example of how things might work if this happened.

E.g. the plugin window in the example fills the area of the screen (50, 50, 250,
250):

- on the first draw the DC corresponds to the region (0, 0, 250, 250) and the
plugin should draw at (50, 50, 250, 250).

- if you then invalidate the whole plugin area, then the DC corresponds to (50,
50, 250, 250), how do you tell the plugin this? I would guess that you have to
pass (0, 0, 200, 200) through SetWindow.

- if you then invalidate just the bottom right area of the plugin say (150, 150,
200, 200) in plugin co-ords. This means that a DC corresponding to (200, 200,
250, 250) will be passed to draw. And (-150, -150, 50, 50) must then be passed
through SetWindow. And the plugin must make sure that it only paints into the
positive part of the DC.

If we adopt this approach the downside is that it will probably break existing
windowless 4.x plugins, our plugin as it stands is not designed to deal with
negative values for window.x and window.y. Although it's not very much work to
fix it. Is there an alternative, can we switch off the optimised drawing for the
widget that contains us?
I just found bug 49743 which is for the Mac. It switches double-buffering off
for Mac plugins. I guess we could do that for windowless plugins and that might
fix this problem.
Does the 0.9.4 branch show this problem?

Does disabling double buffering help?
Keywords: 4xp, testcase
The 0.9.4 branch doesn't show this problem.

I haven't tried disabling double-buffering yet. I don't have time today, and am
out of office until Wednesday. I'll give it a go then.
David, you are right. There in an optimization in the trunk to create a new
context for just the area to be painted. Some of that translation code is here:
http://lxr.mozilla.org/mozilla/source/view/src/nsViewManager.cpp#698

Kevin, is there a way to deactivate the optimization? A hack in plugin code may
break backwards compatibility, see comment #12.
Looks like we may have to call SetWindow on every paint. The origin will need to
be fixed up (with negative values) so that the plugin basically paints at 0,0. 

Ari, does this bug pose any problems for Viewpoint?
Yes- I see this problem.  Haven't been able to play with it though- jury duty :(
The view manager should be applying a translation to the rendering context so
that (0,0) is at the origin of the frame whenever you paint. You should never
have to apply any sort of fixup. Always start drawing at (0,0). Do not apply the
offset you got from SetWindow.

If, for backwards compatibility reasons, you must apply your last SetWindow
offset  when drawing, then we need to modify nsObjectFrame::Paint to subtract
that offset before calling into the plugin.
Upon the first paint, the rendering context is the size of the entire window and
painting at dirty.x, dirty.y is in the corner.  Adding window->x, window->y
paints in the right place.


    +-----------+  --
    |           |   |  <-- G
    |   +---+   |   |
+--------+  |   |  --
|        |--+   |
|        |------+
+--------+

Upon invalidating the lower left corner of the plugin, the plugin draws in the
correct horizontal direction but is offset by G in the veritcal.

Is there some kind of translation or other calculation needed to tell the plugin
where the right place to paint from the new origin is? 

Another testcase:
http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/
(add /I "..\..\..\base\public")
Ahhhhhh

What's happening is that when the view manager applies a translation to
nsRenderingContextWin, the translation is stored in the context's mTranMatrix.
But the plugin is using the underlying DC directly, ignoring any accumulated
translations. Thus the setup that the view manager is doing for you is just
being ignored.

You need a way to extract the current translation offset from an
nsRenderingContextWin and apply it to the DC before asking the plugin to paint.
You'll also need to subtract whatever SetWindow offset the plugin is going to
add. (Don't forget to undo your changes before returning.)
Really what you want is a method on nsRenderingContextWin that lets you sync up
a DC with whatever logical transformations have been specified for the rendering
context. Better talk to whowever owns that code.
This patch improves things but doesn't fix everything. Invalidation works well
with this patch. If you drag a window over the top of the plugin then it
redraws perfectly. However, sometimes the plugin doesn't appear at all on first
load, I need to investigate whether this is related to the patch or not.

This is also quite a big change as it knocks out the call to
GetWindowOriginInPixels which was introduced to fix a lot of problems viewpoint
were having. I suspect though that this might be the 'right' way of doing
things.
Looks good! You're definitely on the right track.
The transformation also holds a scale, does anyone know in what circumstances
the DC will get scaled? I guess that the scale could be used to modify the
height and width passed into SetWindow.

Another benefit of the above patch is that it fixes a bug I have been meaning to
file for ages. style.mozOpacity never worked with windowless plugins, the plugin
always drew in the wrong place. With this patch it works.

Also, it's worth noting that I didn't have to change our plugin code at all.
The scale is mainly used for twips->pix conversion. You don't need to worry
about it. We don't currently support arbitrary scaling.
Attachment #73005 - Flags: review+
Comment on attachment 73005 [details] [diff] [review]
First attempt at a patch

This patch is pretty good, I'm not seeing any weird problems. Maybe we should
try to get this into 0.9.9? r=peterl
I wouldn't bother with this for 0.9.9.
Comment on attachment 73005 [details] [diff] [review]
First attempt at a patch

sr=roc+moz

Looks good for 1.0
Attachment #73005 - Flags: superreview+
Without this patch, windowless plugins are not usable at all.  This patch is
also isolated to windowless plugins.
Feel free to ask the other drivers for 0.9.9 approval.
Comment on attachment 73005 [details] [diff] [review]
First attempt at a patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73005 - Flags: approval+
Attached file Updated tests — 
This test doesn't exhibit the problem of sometimes not drawing. I have also
included a demo of the mozOpacity working. It's a bit slow in debug builds but
it works.

The problem of the plugin sometimes not drawing, was due to our plugin. There
was a bug in NS4.x which meant that sometimes it set window coords of (0,0) and
actually drew the plugin at this position even if it was meant to be offset
into the window. So, we ignore calls to draw at (0,0) unless an extra parameter
is set in the embed tag.

I don't have cvs permission to check-in the patch so could you do it Peter?
Patch in trunk, marking FIXED.

David, thanks for the patch! You can get CVS write access by following the
guidlines here:
http://www.mozilla.org/hacking/getting-cvs-write-access.html
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
verified with build 2002031803
Status: RESOLVED → VERIFIED
Nominating for the 0.9.9 branch.
Keywords: edt0.9.9
a=chofmann and edt for the 0.9.9 branch.
Keywords: edt0.9.9 → edt0.9.9+
landed on 0.9.9 branch
Keywords: fixed0.9.9
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: