Last Comment Bug 141901 - IFrame Rendering Asynchronously
: IFrame Rendering Asynchronously
Status: REOPENED
[adt2]DIGBug
: regression, testcase, topembed+
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: All All
: P1 normal with 3 votes (vote)
: mozilla1.4final
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: grouper
  Show dependency treegraph
 
Reported: 2002-05-02 18:11 PDT by Kirk Parsons
Modified: 2010-02-02 20:16 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simple test case fhat shows blinking IFrame bug. (1.36 KB, text/html)
2002-05-02 18:17 PDT, Kirk Parsons
no flags Details
*HACK* which makes the window show/hide asynchronous (2.98 KB, patch)
2003-02-07 11:43 PST, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
patch to defer the show/hide of a view's widget if the viewmanager is batching updates (3.29 KB, patch)
2003-02-07 17:23 PST, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
updated patch (3.91 KB, patch)
2003-04-08 13:59 PDT, Kevin McCluskey (gone)
roc: review+
kinmoz: superreview+
Details | Diff | Splinter Review
Updated trunk patch. (3.87 KB, patch)
2003-05-13 17:06 PDT, Kevin McCluskey (gone)
no flags Details | Diff | Splinter Review
patch with Kin's suggestions + small change to fix nsViewVisibility usage (3.73 KB, patch)
2003-05-15 15:56 PDT, Kevin McCluskey (gone)
asa: approval1.4+
Details | Diff | Splinter Review

Description Kirk Parsons 2002-05-02 18:11:41 PDT
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".
Comment 1 Kirk Parsons 2002-05-02 18:15:09 PDT
Nominating for nsbeta1+, since this is a regression.  
Comment 2 Kirk Parsons 2002-05-02 18:17:11 PDT
Created attachment 82146 [details]
Simple test case fhat shows blinking IFrame bug.

Test case for bug.
Comment 3 John Keiser (jkeiser) 2002-05-02 18:28:12 PDT
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*?)
Comment 4 Amarendra Hanumanula 2002-05-02 18:48:25 PDT
 Works fine with yesterdays branch build 2002050106 on WIN2K. Blue Iframe closes
when I click on the close link.
 
Comment 5 Kevin McCluskey (gone) 2002-05-03 09:27:09 PDT
I see the blinking with 2002050108 trunk build on WinXP. 
Comment 6 Kirk Parsons 2002-05-03 10:52:38 PDT
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.

 
Comment 7 Kevin McCluskey (gone) 2002-05-03 17:44:52 PDT
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?
Comment 8 Kevin McCluskey (gone) 2002-05-08 11:22:08 PDT
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.
Comment 9 John Keiser (jkeiser) 2002-05-13 17:25:31 PDT
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.
Comment 10 John Keiser (jkeiser) 2002-05-22 11:44:30 PDT
Strange, Netscape 2002052108 is not showing the blinking.
Comment 11 John Keiser (jkeiser) 2002-05-23 12:36:00 PDT
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).
Comment 12 John Keiser (jkeiser) 2002-05-23 15:33:01 PDT
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?
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2002-05-23 21:13:27 PDT
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.
Comment 14 Kevin McCluskey (gone) 2002-06-14 11:00:47 PDT
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.
Comment 15 John Keiser (jkeiser) 2002-06-14 13:27:35 PDT
I still see this on Netscape trunk WinXP 2002061408.
Comment 16 Kevin McCluskey (gone) 2002-08-12 10:54:14 PDT
nsbeta1+. 
Comment 17 Marek Z. Jeziorek 2002-09-13 09:36:03 PDT
batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Comment 18 Rick Ju 2002-09-27 19:24:34 PDT
i can see the blink on 20020826 cvs build for linux.
but not on w2k(20020910)(1.2a) and  mozilla1.0(gecko20020618) on linux.

:(
Comment 19 Kevin McCluskey (gone) 2002-10-15 15:14:12 PDT
I see the issue using 2002101508 trunk build on WinNT.
Comment 20 Amarendra Hanumanula 2002-11-19 15:17:15 PST
I see the problem with the latest build: 2002-11-19-08 on WINXP. Adding testcase
keyword.
Comment 21 Kevin McCluskey (gone) 2003-02-07 11:43:04 PST
Created attachment 113820 [details] [diff] [review]
*HACK* which makes the window show/hide asynchronous

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
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-02-07 12:32:26 PST
Yeah. But the docshell/documentviewer API really requires a widget to be
associated with each document.
Comment 23 Kevin McCluskey (gone) 2003-02-07 17:23:44 PST
Created attachment 113856 [details] [diff] [review]
patch to defer the show/hide of a view's widget if the viewmanager is batching updates

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
Comment 24 Kevin McCluskey (gone) 2003-04-07 18:27:48 PDT
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)
Comment 25 Kevin McCluskey (gone) 2003-04-07 18:53:16 PDT
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>

Comment 26 Kevin McCluskey (gone) 2003-04-08 13:59:35 PDT
Created attachment 119849 [details] [diff] [review]
updated patch
Comment 27 Kevin McCluskey (gone) 2003-04-11 14:46:05 PDT
taking
Comment 28 kinmoz 2003-04-11 16:33:03 PDT
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;
Comment 29 Michael Dunn 2003-05-05 11:04:16 PDT
Can we have and updated milestone? Who can review and when can the patch?
Comment 30 Samir Gehani 2003-05-09 11:16:55 PDT
adt: nsbeta1+/adt2
Comment 31 John Keiser (jkeiser) 2003-05-10 01:46:25 PDT
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.
Comment 32 Kevin McCluskey (gone) 2003-05-12 08:17:36 PDT
The patch needs to be tested against the current trunk. I'm not sure it still
works or applies. 
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2003-05-12 11:47:34 PDT
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.
Comment 34 Kevin McCluskey (gone) 2003-05-13 17:06:04 PDT
Created attachment 123210 [details] [diff] [review]
Updated trunk patch.
Comment 35 Kevin McCluskey (gone) 2003-05-15 15:22:35 PDT
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.
Comment 36 Kevin McCluskey (gone) 2003-05-15 15:56:55 PDT
Created attachment 123454 [details] [diff] [review]
patch with Kin's suggestions + small change to fix nsViewVisibility usage
Comment 37 Kevin McCluskey (gone) 2003-05-15 15:58:48 PDT
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.
Comment 38 Kevin McCluskey (gone) 2003-05-21 08:38:52 PDT
Latest patch has been tested on WIN32, Mac. No regressions have been detected.  
Comment 39 Kevin McCluskey (gone) 2003-05-21 08:40:07 PDT
Comment on attachment 123454 [details] [diff] [review]
patch with Kin's suggestions + small change to fix nsViewVisibility usage

Seeking approval for 1.4
Comment 40 Asa Dotzler [:asa] 2003-05-21 15:01:54 PDT
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.
Comment 41 Kevin McCluskey (gone) 2003-05-21 20:21:48 PDT
Fix checked into trunk.
Comment 42 Kevin McCluskey (gone) 2003-05-21 22:01:41 PDT
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.
Comment 43 Sergei Dolgov 2003-05-25 04:25:44 PDT
REGRESSION in BeOS.
i-Frames repaint/invalidation is dead.
Comment 44 Sergei Dolgov 2003-05-25 05:17:24 PDT
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
Comment 45 Madhur Bhatia 2003-06-05 14:45:36 PDT
This bug has the KW: nsbeta1+ and Status Whiteboard : [adt2]

any progress with this bug?


Comment 46 Kevin McCluskey (gone) 2003-06-05 17:02:03 PDT
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.  
Comment 47 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-03-05 07:36:29 PST
Is this still an issue in current trunk build? I can hardly imagine that.

Note You need to log in before you can comment on or make changes to this bug.