Closed
Bug 288873
Opened 19 years ago
Closed 19 years ago
content of iframe disappears after mouseover at print preview
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: linxspider, Assigned: roc)
References
Details
(Keywords: regression)
Attachments
(3 files)
390 bytes,
text/html
|
Details | |
12.96 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
10.11 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
asa
:
approval1.8b2+
|
Details | Diff | Splinter Review |
when moving the mouse over an "iframe" in print preview, the content of it disappears after returning to the page. this seems to be a regression from Bug 284664
Reporter | ||
Comment 1•19 years ago
|
||
an example of the problem
Blocks: 284664
Comment 2•19 years ago
|
||
In print preview, if I move the mouse outside the iframe I get a continuous stream of: ###!!! ASSERTION: Mouse move must have some target content: 'targetElement', file /home/bzbarsky/mozilla/xlib/mozilla/content/events/src/nsEventStateManager.cpp, line 2682 And when I open up print preview once the iframe has gone blank, I get: ###!!! ASSERTION: null window: 'mWindow', file /home/bzbarsky/mozilla/xlib/mozilla/layout/base/nsDocumentViewer.cpp, line 1671
Assignee | ||
Comment 3•19 years ago
|
||
The assertions about no target for the mouse move are fairly straightforward ... there is no content corresponding to the page frames and the viewport area outside the pages, so no target content for mouse moves over those frames. This should be fairly innocuous. I can patch that up so that GenerateMouseEnterExit treats the mouse as over the root element if it can't find any other target content. But how this manages to nuke the IFRAME at end of print preview, I'm not sure. I do get this assertion: ###!!! ASSERTION: no document: 'doc', file ../../../../../content/xul/content/src/nsXULElement.cpp, line 2376 Break: at file ../../../../../content/xul/content/src/nsXULElement.cpp, line 2376 On the stack, some script is trying to execute a "cmd_undo".
Assignee | ||
Comment 4•19 years ago
|
||
This is a better testcase: https://bugzilla.mozilla.org/attachment.cgi?id=179686 I've noticed that the IFRAME contents only get thrown away if we move the mouse out of it during print preview.
Assignee | ||
Comment 5•19 years ago
|
||
The problem is that the ESM code to propagate mouse events down into subdocuments was calling nsSubdocumentFrame::GetDocShell. This has the side effect of loading the subdocument if it hasn't already started loading. In particular it hooks up nsSubdocumentFrame::mFrameLoader to the frame loader. Then nsSubdocumentFrame::Destroy thinks it's responsible for telling mFrameLoader's document viewer to Hide() ... even though this document viewer is the docviewer for the main presentation, and we're in the print preview presentation. That would be OK because Hide() checks to see if it's in Print or PrintPreview and ignores the hide request if so. Unfortunately the comments lie and the check always returns PR_FALSE in subdocuments, because they don't have a pointer to the print engine. So we go ahead and tear down the subdocument's main presentation. The approach I'm taking here is to simply not call GetDocShell with that chain of nasty side effects ... instead we can get the subdocument's prescontext/ESM by walking the view tree. This doesn't work in print/print preview because they don't have the view trees hooked up, but who cares. This patch also includes a fix to stop those assertions firing. And it documents the lies in DocumentViewerImpl.
Assignee | ||
Updated•19 years ago
|
Attachment #179821 -
Flags: superreview?(bzbarsky)
Attachment #179821 -
Flags: review?(bzbarsky)
Comment 6•19 years ago
|
||
Comment on attachment 179821 [details] [diff] [review] fix I guess this is ok as wallpaper, but could you file a bug on fixing the nsDocumentViewer code? It should probably call internal methods that check its parents too, not the frozen api functions it's calling now. If you don't have time to fix that, let me know and I'll look for someone to do it, or do it myself. Once that's done, do we want to go back to using GetDocShell, or leave things as they are?
Attachment #179821 -
Flags: superreview?(bzbarsky)
Attachment #179821 -
Flags: superreview+
Attachment #179821 -
Flags: review?(bzbarsky)
Attachment #179821 -
Flags: review+
Assignee | ||
Comment 7•19 years ago
|
||
Comment on attachment 179821 [details] [diff] [review] fix fairly straightforward fix to a major regression
Attachment #179821 -
Flags: approval1.8b2?
Assignee | ||
Updated•19 years ago
|
Attachment #179821 -
Flags: approval1.8b2?
Assignee | ||
Comment 8•19 years ago
|
||
There are really a couple of bugs here: 1) DocumentViewerImpl doesn't ignore Hide() in print/print preview for subdocuments. 2) nsSubDocumentFrame::GetDocShell shouldn't have the side effect of triggering document load. It should be called ObtainDocShell or something like that, and if necessary, a non-side-effecting GetDocShell should be added. In particular nsSubDocumentFrame shouldn't be deciding whether to call Hide() based on whether GetDocShell was ever called or not. Honestly, I don't fully understand the lifetime issues and relationships between the docshell, the documentviewer, the frameloader, and how the print engine massages them. But you're right that this fix isn't great. Adding new API as a band-aid is not a good idea. I'll submit another patch which is less band-aid-y and at least doesn't add new API.
Assignee | ||
Comment 9•19 years ago
|
||
This approach just makes the subdocument frame not call Hide() if it didn't create the presentation in ShowDocShell. That seems reasonable. It certainly fixes the bug and doesn't require any new interface.
Attachment #179891 -
Flags: superreview?(bzbarsky)
Attachment #179891 -
Flags: review?(bzbarsky)
Comment 10•19 years ago
|
||
> Honestly, I don't fully understand the lifetime issues and relationships
> between the docshell, the documentviewer, the frameloader, and how the print
> engine massages them.
That's because the relationships are crappy. The document viewer is sort of a
per-presentation object, except there's one particular one that the docshell
holds a reference to... The rest of the mess is corollaries of this. :(
Comment 11•19 years ago
|
||
Comment on attachment 179891 [details] [diff] [review] fix #2 Yeah, this is better. Maybe s/ObtainDocShell/EnsureDocShell/ or something?
Attachment #179891 -
Flags: superreview?(bzbarsky)
Attachment #179891 -
Flags: superreview+
Attachment #179891 -
Flags: review?(bzbarsky)
Attachment #179891 -
Flags: review+
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 179891 [details] [diff] [review] fix #2 okay, this is the preferred fix for this unfortunate regression.
Attachment #179891 -
Flags: approval1.8b2?
Comment 13•19 years ago
|
||
Comment on attachment 179891 [details] [diff] [review] fix #2 a=asa for 1.8b2 checkin.
Attachment #179891 -
Flags: approval1.8b2? → approval1.8b2+
Assignee | ||
Comment 14•19 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 15•19 years ago
|
||
is this bug somehow related to Bug 176611 ?
Assignee | ||
Comment 16•19 years ago
|
||
Probably not.
You need to log in
before you can comment on or make changes to this bug.
Description
•