Open Bug 141901 Opened 22 years ago Updated 2 years ago

IFrame Rendering Asynchronously

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

defect

Tracking

()

REOPENED
mozilla1.4final

People

(Reporter: kirkpp, Unassigned)

References

Details

(Keywords: regression, testcase, topembed+, Whiteboard: [adt2]DIGBug)

Attachments

(2 files, 4 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

An IFrame which is hidden, and then moved shows a brief rendering at the new
location, even though it should be hidden

Reproducible: Always
Steps to Reproduce:
1. Open up the test case
2. Click the "open" link
3. Click the "close" link
4. Note blue IFrame is rendered in the upper left hand corner (briefly), even
though it has been hidden previously.

Actual Results:  1. Open up the test case
2. Click the "open" link
3. Click the "close" link
4. Note blue IFrame is rendered in the upper left hand corner (briefly), even
though it has been hidden previously.

Expected Results:  1. Open up the test case
2. Click the "open" link
3. Click the "close" link
4. The blue IFrame is not rendered in the upper left hand corner, and there is
no "flash" or "blink".
Nominating for nsbeta1+, since this is a regression.  
Keywords: nsbeta1, regression
Whiteboard: DIGBug
Test case for bug.
WFM Linux 2002042707.  But it could be a Windows problem or something that was
fixed on branch since RC1.  When was the last time you saw this working?  (i.e.
what is this a regression *from*?)
 Works fine with yesterdays branch build 2002050106 on WIN2K. Blue Iframe closes
when I click on the close link.
 
I see the blinking with 2002050108 trunk build on WinXP. 
It's a regression from NS6.2.  We develop for NS6.2, and test on NS6.2, th
trunk, and now the branch.  I haven't gone looking yet to see when it first occurs.

 
amar: The blue IFRAME closes but the problem is that it shows the blue iframe
offset to the upper right hand corner for just a moment before it clears the
screen. On the branch build, do you see a flash of blue in the upper left corner
when the iframe is closed?
The blinking is caused by the resetting of the dialogStyle.top and
dialogStyle.left to (0,0) in testClose. If they are changed to dialogStyle.top =
40 and dialogStyle.left = 40 the blinking goes away

function testClose() {
    var dialogIFrame = document.getElementById("MyFrame");
    var dialogStyle = dialogIFrame.style;
    dialogStyle.visibility = 'hidden';
    dialogStyle.top = 0; <== Change to 40 to stop blinking
    dialogStyle.left = 0; <== Change to 40 to stop blinking
}

This example assumes that the visibility change will be processed *before* the
position change. It appears that the position change is processed before the
visibility change. My guess is the visibility change results in a reflow which
is  scheduled off a PL_event while the position change happens immediately.
Assigning to look into this.  If that's the problem we will probably be able to
fix it by flushing reflows after setting style, but that *will* have performance
implications.
Status: NEW → ASSIGNED
Strange, Netscape 2002052108 is not showing the blinking.
Debugging this, I don't get this at *all*.  The "set visibility" is processed
*after* the "set top and left" directives.  I have confirmed this as close to
the JS as possible (in CallSetProperty in nsCSSDOMDeclaration.cpp).
Roc, I have determined that the events do happen in order ... the style reflow
for visibility gets places in the queue before the style reflow for top and
left.  Kevin remembers a case where some elements were being displayed even when
they were hidden ... could this be a not-caught edge case of that?
I can't reproduce this (Linux), but I'll hazard a guess as to what's happening:

I suspect the first reflow that hides the widget is recomputing its position as
well using the new style data, so hiding and moving the widget is coalesced into
the first pass (note that this will happen even more frequently with the reflow
coalescing code now in the trunk, and this is generally a GOOD thing). The only
problem is that we are moving the widget first, before it gets hidden (this is
because reflow generally positions stuff before it updates the other properties
of views and widgets). Even this wouldn't be a problem except that Windows
updates the display synchronously in response to widget motion.

There are other bugs just like this. I think bugs like this are inevitable as
long as the underlying platform (Windows) doesn't allow us to change the state
of a whole set of widgets atomically without any intermediate painting. The
solution is to reduce or eliminate our use of native widgets.
WFM using 2002-06-13 WinXP trunk build. - No blinking
Blinks using 2002-6-13 WinXP branch build.

It's probably a difference in the reflow coalescing as Robert describes in
Comment #13.

Another solution may be to use the following methods in nsViewManager which
cache all of the widget changes:

nsViewManager::CacheWidgetChanges(PRBool aCache)
nsViewManager::IsCachingWidgetChanges(PRBool* aCaching)
nsViewManager::ProcessWidgetChanges(nsView* aView)

This code is currently turned off. see //#define CACHE_WIDGET_CHANGES in
nsViewManager.cpp.

evaughan put this code but ran into some problems. I can't recall exactly what
stopped working when this was enabled.

Marking nsbeta1-. Any fix for this on the branch is likely to be risky.
Keywords: nsbeta1nsbeta1-
I still see this on Netscape trunk WinXP 2002061408.
nsbeta1+. 
Keywords: nsbeta1-nsbeta1+
batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed
i can see the blink on 20020826 cvs build for linux.
but not on w2k(20020910)(1.2a) and  mozilla1.0(gecko20020618) on linux.

:(
Keywords: topembedtopembed+
I see the issue using 2002101508 trunk build on WinNT.
Blocks: grouper
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta
I see the problem with the latest build: 2002-11-19-08 on WINXP. Adding testcase
keyword.
Keywords: testcase
Target Milestone: mozilla1.3beta → mozilla1.4alpha
The hack I attached shows that this bug is caused by the synchronous painting
triggered by the show/hide method in nsWindow. The hack makes the show/hide
asynchronous and the blinking bug goes away. The hack has other bad side
effects however. Dropdown's are not repainted when they are rolled up.

I think a better approach would be to suppress the creation of the widget
except when the iframe contains an applet or plugin
Attachment #113820 - Attachment is obsolete: true
Yeah. But the docshell/documentviewer API really requires a widget to be
associated with each document.
Looking at this closer the show/hide is *not* doing a synchronous paint.  The
problem is that show/hide was not honoring the view managers batching of
updates. Showing/Hiding the widget will cause an invalidate. If the view
manager is batching updates these invalidates also need to be batched.

I added code to check to see if the view manager was currently batching
updates. If so, I defer the hide/show. This is very similar to what was
happening in bug 36849. The solution "piggy backs" on top of the solution for
bug 36849
If I modify the test case to use onMouseOver to open and close the iframe and
run it within mfcembed it does not flicker. (Note: I have to use mfcembed to get
around bug 129844 which causes flicker due to XUL status bar updates)
Also doesn't blink if OnClick is used (run within mfcembed).

Problem is specific to JS/href:

<a href="javascript:testOpen()">open</a> 
<a href="javascript:testClose()">close</a>

Attached patch updated patch (obsolete) — Splinter Review
Attachment #113856 - Attachment is obsolete: true
taking
Assignee: jkeiser → kmcclusk
Status: ASSIGNED → NEW
Comment on attachment 119849 [details] [diff] [review]
updated patch

sr=kin@netscape.com


==== I'd lose "being" in this sentence, but it's up to you.  :-)

+    // If they dont match then showing/hiding the widget was being deferred.


==== Use PR_TRUE/PR_FALSE, but can't the following be simplified to just
|widget->Show(viewVisibility);|?


+	 // Process the deferred show/hide of the view's widget
+	 if (viewVisibility) {
+	   widget->Show(TRUE);
+	 } else {
+	   widget->Show(FALSE);
+	 }


==== Also can this be simplified to |return mUpdateBatchCnt > 0;|?


+PRBool nsViewManager::IsBatchingUpdates(void)
+{ 
+  if (mUpdateBatchCnt > 0) {
+    return PR_TRUE;
+  }
+    
+  return PR_FALSE;
Attachment #119849 - Flags: superreview+
Can we have and updated milestone? Who can review and when can the patch?
Target Milestone: mozilla1.4alpha → mozilla1.5beta
adt: nsbeta1+/adt2
Whiteboard: DIGBug → [adt2]DIGBug
Comment on attachment 119849 [details] [diff] [review]
updated patch

The patch looks good to me and I'll r= if we have to put it in right away, but
roc's module owner here.
Attachment #119849 - Flags: review?(roc+moz)
The patch needs to be tested against the current trunk. I'm not sure it still
works or applies. 
Comment on attachment 119849 [details] [diff] [review]
updated patch

It does need to be tested but I can't think of anything that's changed that
would prevent this from working.

I think this approach could be extended to defer widget sizing and positioning
too, which would be fabulous.
Attachment #119849 - Flags: review?(roc+moz) → review+
Attached patch Updated trunk patch. (obsolete) — Splinter Review
Testing the latest patch I found a problem when clicking on the URL-bar dropdown
list. It does not reliably close when clicking on the menubar.
Ignore comment #35. I am able to reproduce the problem with a trunk build that
does not include the patch, so it is an unrelated issue.
Latest patch has been tested on WIN32, Mac. No regressions have been detected.  
Target Milestone: mozilla1.5beta → mozilla1.4final
Comment on attachment 123454 [details] [diff] [review]
patch with Kin's suggestions + small change to fix nsViewVisibility usage

Seeking approval for 1.4
Attachment #123454 - Flags: approval1.4?
Comment on attachment 123454 [details] [diff] [review]
patch with Kin's suggestions + small change to fix nsViewVisibility usage

a=asa (on behalf of drivers) for checkin to 1.4.
Attachment #123454 - Flags: approval1.4? → approval1.4+
Fix checked into trunk.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Reopening for now. I noticed the MacOSX Tp: and Ts: jumped up after my checkin.
So I backed out my change to see if it was from my checkin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Windows 2000 → All
Hardware: PC → All
REGRESSION in BeOS.
i-Frames repaint/invalidation is dead.
Sorry people.
comment http://bugzilla.mozilla.org/show_bug.cgi?id=141901#c43
is about results of bug #201442 "fix" which killed children (and so iFrames)
repaint in BeOS port
This bug has the KW: nsbeta1+ and Status Whiteboard : [adt2]

any progress with this bug?


QA Contact: amar → madhur
The patch causes a 20% page load slow down on the Mac.
The Mac must paint more as the result of the patch, but I have not determined why.  
Is this still an issue in current trunk build? I can hardly imagine that.
Assignee: kmcclusk → nobody
QA Contact: madhur → layout.html-frames
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core

Moving to p2 because no activity for at least 24 weeks.
See How Do You Triage for more information

Priority: P1 → P2
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: