General Mac plugin paint and positioning problems

VERIFIED FIXED

Status

()

Core
Plug-ins
P1
critical
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Peter Lubczynski, Assigned: Peter Lubczynski)

Tracking

({pp})

Trunk
PowerPC
Mac System 8.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++][checked in to branch])

Attachments

(4 attachments)

(Assignee)

Description

18 years ago
This bug has been spawned from bug 49743 (Mac plugins must not be double 
buffered) and the several e-mails discussing on how we should handle painting 
with plugins on the Mac.

Currently, plugins generally are visible on the Mac due to the fact we turn off 
double buffering for the view manager. This causes the content of a page with 
plugins to flicker. There needs to be a better solution to this. In addition to 
this, plugins aren't being clipped correctly, often causing painting over 
scrollbars. Also, after scrolling, the plugin often leaves garbage on the 
screen. 

I think I've cc:'ed everyone in the other bug and on the e-mail list. Please 
post your comments about this issue here.
(Assignee)

Comment 1

18 years ago
..accepting bug, adding rtm to keyword, adding Andre to cc....
Status: NEW → ASSIGNED
Keywords: rtm
Is there any way to tell a plugin to clip itself to a certain area on the Mac? 
If not, then there is no way to support plugins reasonably on the Mac with the 
current interface.

Updated

18 years ago
Priority: P3 → P2
(Assignee)

Comment 3

18 years ago
I have done some progress with this in my tree. Looks like positioning and
clipping is now almost being done correctly thanks to some help from Kevin. We
are no longer painting over scrollbars or the chrome.

However, there are still probelms with scrolling. Old bits are being left on the
screen after scrolling quickly. Slowly works better. Simon, pointed me to bug
31032 which is a similar problem.

Changing status to P1 because this is VERY important for Mac plugin usability.
Priority: P2 → P1
(Assignee)

Comment 4

18 years ago
Created attachment 16702 [details]
screen capture of how plugins paint over chrome and scroll bars on the Mac
(Assignee)

Comment 5

18 years ago
Created attachment 16725 [details] [diff] [review]
patch to prevent painting on top of chrome and scrollbars
Nice hack for the branch, but it doesn't look like the right fix for the trunk, 
IMHO.

Comment 7

18 years ago
Adding [rtm need info]. Peter, when you get a= and r=, change this to rtm+.
Whiteboard: [rtm need info]

Comment 8

18 years ago
I don't see any glaring errors in the patch, but am not qualified to review all 
the coordinate math. beard or kmcclusk may be better there.

Comment 9

18 years ago
Agreed, beard should do the review. Two note though:

1) I really don't like the following comment "// XXX for some reason 
aDamageRect.width is zero when doc margins are zero // the dirty rect is 
garbage!!!!". We should understand that better. Also, the 'badDirtyRect' bollean 
is not used anywhere.

2) There is an error in the current code which should be fixed independently of 
this patch. Open a separate bug report if needed. Line 2564 in version 1.183 
reads:
    clipRect.right = clipRect.right + rect.width;
instead of:
    clipRect.right = clipRect.left + rect.width;

Could it be the origin of what you noticed in point #1?
I saw this bug by chance, I haven't done a thorough code review. 
(Assignee)

Comment 10

18 years ago
Created attachment 16796 [details] [diff] [review]
revised patch from comments from reviewers
Since the damaged rect is never intersected with the child window's visible rect
we should remove the code from the patch which calculates the absolute dirty rect.





Robert: I am not sure I understand why you consider this patch to be hack? I
would describe it as a portion of the complete solution. The Mac plugin
interface requires that a clip rect is passed to the plugin expressed as
absolute pixel coodinates from the upper left hand corner of the top level
window. The GetWidgetPosAndClip calculates the resulting clip of the the virtual
plugin window by calculating the visible area assuming each of the child
window's is clipped by it's containing window.

The other portion of the patch converted the damaged rect passed to the
ObjectFrame into absolute pixel coordinates. This would be intersected with the
visible rect and passed to the plugin as it's absolute clip rect.
(Unfortunately, the the damaged rect passed to the ObjectFrame is not always
correct, so Peter removed the call to Intersect the damaged rect.) If he removes
the code to convert the damaged rect to absolute coordinates, we are left with a
patch that does the first part correctly, which is to determine what the clip
rect is in absolute coordinates. I would think this would be acceptable for both
the branch and trunk.

(Assignee)

Comment 13

18 years ago
Created attachment 16823 [details] [diff] [review]
yet a better patch with #ifdef
(Assignee)

Comment 14

18 years ago
The newest patch has changes made from comments by reviewers. There was a
problem with intersecting the dirty rect during paint so I #ifdef it out for now.

Clearning [rtm need info], got r=dcone,kmcclusk; a=buster, adding + per comments
by karnaze.
Whiteboard: [rtm need info] → rtm+
Sorry, I was mistaken. I thought you were adding a lot of Mac-specific code to 
nsObjectFrame, and I didn't realize it's already a complete mess of #ifdefs :-).

Comment 16

18 years ago
This is a pretty big patch, and if we'd had it a week ago, we might have been
able to test on the trunk before taking on the branch. As it is, we're about out
of time, and unless the flickering/clipping render the plugins unusable 100% of
the time, it doesn't sound like a ship stopper. Marking [rtm-] but please go
ahead on the trunk.
Whiteboard: rtm+ → [rtm-]
Phil: Plug-ins on the Mac are unusable without this fix. Worse, the 
plug-ins actually overwrite the chrome, interfering with the usability of the 
product UI itself. (Check out the screen shot attachment if you haven't had a 
chance to yet.) This affects multiple high-priority plug-ins and the display of 
plug-in content from high-profile sites. Hence P1. (All: Remind me which ones 
exactly we've seen this bug with?) This is a stop-ship bug. We must take this on 
the branch. I will show up with the team at 4:00 in Star Trek to make the case 
for this bug. In the meantime, restoring [rtm+] to trigger reconsideration for 
[rtm++].
Keywords: 4xp, pp
Whiteboard: [rtm-] → [rtm+]
(Assignee)

Comment 18

18 years ago
With the lastest patch, more than half of this code is #ifdef out anyway. This 
patch is really needed to clean up some of the code put in for PR3 for plugins. 
It's Mac specific and isolated only to pages with plugins. The screen shot 
example shows a Quicktime plugin painting over the chrome and scrollbars. This 
behavior is easily reproduced with ALL plugins on the Mac if the user scrolls. 

You can find the HTML diffs at: http://nikon/diffs
(Assignee)

Updated

18 years ago
Blocks: 53437

Comment 19

18 years ago
ekrock and clayton say we have to have this. rtm++
Whiteboard: [rtm+] → [rtm++]
The fix has been checked into the branch.
Could you check in on the trunk as well to keep them in synch and then mark
FIXED to trigger verification? Many thanks!
Whiteboard: [rtm++] → [rtm++][checked in to branch]
*** Bug 53437 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 23

18 years ago
okay, I just checked into the trunk, marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 24

18 years ago
This paint and positioning problem is fixed on the mac branch build 20001018. 
I have logged seperate bugs for other problem seen (bug 57186 and bug 57189). 
Adding keyword:VTRUNK to get this verified on trunk.
Keywords: vtrunk

Comment 25

18 years ago
paint and positioning problem no longer occurs on trunk build 1127 on mac. 
VERIFIED,removing 'vtrunk' keyword
Status: RESOLVED → VERIFIED
Keywords: vtrunk
You need to log in before you can comment on or make changes to this bug.