IFrame Rendering Asynchronously

REOPENED
Unassigned

Status

()

Core
Layout: HTML Frames
P1
normal
REOPENED
16 years ago
8 years ago

People

(Reporter: Kirk Parsons, Unassigned)

Tracking

({regression, testcase, topembed+})

Trunk
mozilla1.4final
regression, testcase, topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2]DIGBug)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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".
(Reporter)

Comment 1

16 years ago
Nominating for nsbeta1+, since this is a regression.  
Keywords: nsbeta1, regression
Whiteboard: DIGBug
(Reporter)

Comment 2

16 years ago
Created attachment 82146 [details]
Simple test case fhat shows blinking IFrame bug.

Test case for bug.

Comment 3

16 years ago
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

16 years ago
 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. 
(Reporter)

Comment 6

16 years ago
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.

Comment 9

16 years ago
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: nsbeta1 → nsbeta1-
I still see this on Netscape trunk WinXP 2002061408.
nsbeta1+. 
Keywords: nsbeta1- → nsbeta1+

Comment 17

15 years ago
batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed

Comment 18

15 years ago
i can see the blink on 20020826 cvs build for linux.
but not on w2k(20020910)(1.2a) and  mozilla1.0(gecko20020618) on linux.

:(

Updated

15 years ago
Keywords: topembed → topembed+
I see the issue using 2002101508 trunk build on WinNT.

Updated

15 years ago
Blocks: 176349

Updated

15 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta

Comment 20

15 years ago
I see the problem with the latest build: 2002-11-19-08 on WINXP. Adding testcase
keyword.
Keywords: testcase

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.4alpha
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

Updated

15 years ago
Attachment #113820 - Attachment is obsolete: true
Yeah. But the docshell/documentviewer API really requires a widget to be
associated with each document.
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
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>

Created attachment 119849 [details] [diff] [review]
updated patch
Attachment #113856 - Attachment is obsolete: true
taking
Assignee: jkeiser → kmcclusk
Status: ASSIGNED → NEW

Comment 28

15 years ago
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+

Comment 29

15 years ago
Can we have and updated milestone? Who can review and when can the patch?

Updated

15 years ago
Target Milestone: mozilla1.4alpha → mozilla1.5beta

Comment 30

15 years ago
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+
Created attachment 123210 [details] [diff] [review]
Updated trunk patch.
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.
Created attachment 123454 [details] [diff] [review]
patch with Kin's suggestions + small change to fix nsViewVisibility usage
Attachment #119849 - Attachment is obsolete: true
Attachment #123210 - Attachment is obsolete: true
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 40

15 years ago
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
Last Resolved: 15 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 → ---

Updated

15 years ago
OS: Windows 2000 → All
Hardware: PC → All

Comment 43

15 years ago
REGRESSION in BeOS.
i-Frames repaint/invalidation is dead.

Comment 44

15 years ago
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

14 years ago
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
You need to log in before you can comment on or make changes to this bug.