48.79 KB, image/jpeg
6.94 KB, patch
|Details | Diff | Splinter Review|
7.08 KB, patch
|Details | Diff | Splinter Review|
7.76 KB, patch
|Details | Diff | Splinter Review|
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.
..accepting bug, adding rtm to keyword, adding Andre to cc....
Status: NEW → ASSIGNED
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.
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
Created attachment 16702 [details] screen capture of how plugins paint over chrome and scroll bars on the Mac
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.
Adding [rtm need info]. Peter, when you get a= and r=, change this to rtm+.
Whiteboard: [rtm need info]
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.
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.
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.
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 :-).
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+]
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
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. ***
okay, I just checked into the trunk, marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
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.
paint and positioning problem no longer occurs on trunk build 1127 on mac. VERIFIED,removing 'vtrunk' keyword
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.