128133 - [PATCH]Screen flicker and ghost of flash/iframe content with dhtml

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Ray Semiraglio]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002022703 (Win2k)

A ghost image of flash or iframe content appears ~50-100 pixels above the actual
content and flickers in time to dhtml activity. In the above example, while the
image is moved across the page. I have another example url (coming soon) of the
same behavior involving dhtml menus and an iframe. I have encountered this
behavior in Linux and Windows 2000, using the nightly build above, Mozilla 0.9.8
and Netscape 6.2. This has also been seen in Netscape 6.2 on WinNT 4.0.

I have verified that this behavior is not in Netscape 6.1, so it is a fairly
recent addition to the layout engine.

Reproducible: Always
Steps to Reproduce:
1.Load this page and wait for the moving dhtml image to start across the page.

Actual Results:  The flash ghost image flickers 150 pixels above the actual flash

Expected Results:  Not show the flash ghost image

Comment 1

[Chris Petersen]

Confirming on the Feb 27th build (2002-02-27-03) under Windows ME. Doesn't occur
on OS X build (2002-03-01-03).

Comment 2

[Ray Semiraglio]

Iframe example: http://www.sportsline.com/

Same as the flash example, the iframes ghost a few hundred pixels above their
actual location when you mouse over the dhtml menus. The iframe content is
labeled Brackets Contests and Fantasy/My Leagues.

Comment 3

[Chris Petersen]

Attachment: Reduces test case of flicker problem

Test case includes two left aligned tables. The second table contains the embed
element which references the flash file.

Comment 4

[Marc Attinasi]

Well, the problem is related to the embed in a floated table. If you remove the
align-"left" from the table that contains the <embed> then the flickering /
ghosting cruft goes away.

To fix this in the code is going to be at least a day's work, possibly more. I
need to put in some probes to figure out why the floated table is getting moved
around so much - it should not be. If we cannot fix this in the markup (eg.
evangelism) then I'll take a crack at it - pease advise.

Comment 5

[Marc Attinasi]

I have a fix for this. The problem is that the plugin is in a floated element
(in my testcase, a div, could be a table as well). The floated element is
reflowed whenever the blimp moves. During the reflow, we are first repositioning
the view for the floated element's frame to a bogus location (the top of the
page, basically) and then after reflowing and placing the floater where we want
it, we set the view position again, this time to the correct location.

I cannot think of any reason to do the initial view placement - it is going to
be a wrong location, and it will have to be moved again later anyway. Stopping
the first view placement stops this bug. Now I just need to figure out if that
breaks something else...

Comment 6

[Marc Attinasi]

Attachment: PATCH: removes unnecessary call to PlaceFrameView from ReflowDirtyLines

This patch removes a call to PlaceFrameView that was setting the view position
incorrectly. Later, after the frame was reflowed, the view would be moved back
to the correct location, but the movement from position A to position B and
then back to position A caused the flicker.

From the comments in the code it looked like there was some question as to
whether or not that call to ::PlaceFrameView should have been made at all - I'm
querying dbaron on that since he wrote (rewrote) that code. However, from my
tests the position of that view was never correct as a result of that call, so
I cannot see what good it was. Also, by moving the view(s) to an intermediate
position and then later back to the correct position we subcert the
optimizations of the view manager that minimize the work required if a view is
moved to the same position that it is already in. Thus we get better
performance and no flicker.

Now, to figure out what might have broken...

Comment 7

[Marc Attinasi]

Attachment: Simplified testcase: no tables, animates on mouse-click

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What I needed that for is what I mentioned in bug 86947 comment 31,
that is, the bugzilla query page.  I forget whether it was the textboxes
or the selects, but I remember that some things weren't moving when I
resized the bugzilla query page and/or bugzilla bug pages.  (At least I
think it was resizing rather than incremental loading.)

Comment 9

[Marc Attinasi]

Thanks David. I tested Bugzilla Query and some bug pages, resizing the window
and messing with the listboxes and combo-boxes - no problems. I'll concoct some
tests that float those controls and see what I get too, just to be safe.

The block regression tests passed (though there are two testcases that crash due
to reading free'd memory, but that is in both the baseline and modified build).
Admitedly, the block regression tests will not detect the issues with resizing...

Comment 10

[Marc Attinasi]

Attachment: Additional patch: tables need to stop repositioning views during reflow for floated tables

I noticed that there is a problem (with testcase and www.sportsline.com) with
flickering when resizing the window horizontally as well. That problem is due
to the table code erroneously repositioning the views for the floated table
before the table has actually been placed, so again the view are positioned to
an incorrect intermediate position before being correctly positioned, causing
the flicker.

This patch, used in addition to the first one (76649), fixes the flickering in
the DHTML case and the resizing case, and should make www.sportsline.com happy.


It might be a good idea to revisit the whole notion of repositioning views
during reflow - I think there is a mixture of assumptions and approaches in the
layout code...

Comment 11

[Marc Attinasi]

CC'ing karnaze since I have now entered Table Land (tm).

Also, nominating for NSBETA1 since this is a pretty ugly problem on a pretty
popular site.

Comment 12

[Bob Clary [:bc] (inactive)]

nominating for topembed as sportlines is *the* AOL sports partner.

Comment 13

[Kevin McCluskey (gone)]

nsbeta1+.

Comment 14

[Marc Attinasi]

BTW: this problem of positioning a floated element's views before it is
positioned, and then again after it is positioned, is very general and will
impact a lot of sites since left-side navigation areas are very common (though
it needs an IFRAME, a plugin, or some other widget-containing element to exhibit
the flicker). 

I'm pretty sure it is a regression from 0.9.4 but I have not narrowed down the
milestone where it broke.

Comment 15

[Kevin McCluskey (gone)]

adt2

Comment 16

[karnaze (gone)]

Comment on attachment 76760 [details] [diff] [review]
Additional patch: tables need to stop repositioning views during reflow for floated tables

r=karnaze

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 76649 [details] [diff] [review]
PATCH: removes unnecessary call to PlaceFrameView from ReflowDirtyLines

r=dbaron if you've tested that resizing the bugzilla query page and bugzilla
bug pages works as expected

Comment 18

[Marc Attinasi]

Yes, I tested the bugzilla case, and a whole bunch of other pages using floated
elements, floated plugins, non-floated plugins, and form controls.

Comment 19

[Chris Waterson]

Comment on attachment 76649 [details] [diff] [review]
PATCH: removes unnecessary call to PlaceFrameView from ReflowDirtyLines

sr=waterson

Comment 20

[Chris Waterson]

Comment on attachment 76760 [details] [diff] [review]
Additional patch: tables need to stop repositioning views during reflow for floated tables

sr=waterson

Comment 21

[Marc Attinasi]

-->adt1.0.0

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 76649 [details] [diff] [review]
PATCH: removes unnecessary call to PlaceFrameView from ReflowDirtyLines

a=roc+moz

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 76760 [details] [diff] [review]
Additional patch: tables need to stop repositioning views during reflow for floated tables

a=roc+moz

Comment 24

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval for checkin.

Comment 25

[Marc Attinasi]

Fixes checked into trunk.

/cvsroot/mozilla/layout/html/table/src/nsTableOuterFrame.cpp,v  <--  nsTableOute
rFrame.cpp
new revision: 3.220; previous revision: 3.219

/cvsroot/mozilla/layout/html/table/src/nsTableRowFrame.cpp,v  <--  nsTableRowFra
me.cpp
new revision: 3.305; previous revision: 3.304
done

/cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.495; previous revision: 3.494

Comment 26

[R.K.Aa.]

can this have caused bug 136078?

Comment 27

[Matthias Versen [:Matti]]

it caused bug 136080.
I backed out this fix and the problem goes away.

Comment 28

[Mrten]

will this patch fix (still unconfirmed) bug 119163 as well? i might dup that one
then to this...

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 136080 was basically the problem I mentioned in comment 8 and comment 17.  I
find it most noticeable with the cc: field listbox on the bugzilla bug pages
(like this page), which often gets misplaced for me on the initial load of the
page, and after than "drags" one resize behind when resizing the page.

Comment 30

[karnaze (gone)]

In addition to causing bug 136080, it (actually on the 1st patch) has caused a 
problem on http://bugzilla.mozilla.org/show_bug.cgi?id=126160 where the <select> 
is badly placed. I'm seeing it on initial load, but alexsavulov writes: 

I can see this in build 2002040803
I also discovered that is reproductible only when refreshing the
window (SHIFT + click on refresh) and the window has the width
seen in the the jpg image (aprox. 962 px including the scrollbar).
when resizing the window, the layout gets fixed.

Comment 31

[karnaze (gone)]

Well, maybe what I just reported is bug 136080.

Comment 32

[Alexandru Savulov]

Attachment: regression testcase

i also found out that if you get the right window width (about 962px) and you
slowly resize around that width you'll get the select to be  placed wrongly too
although another place than when refreshing.

Comment 33

[Alexandru Savulov]

Attachment: screenshot

Comment 34

[Marc Attinasi]

Well I see the problem on bugzilla now :)  

Backing out until I can figure out why we are sometimes placing the views
correctly and sometimes not.

Comment 35

[Marc Attinasi]

Backing out the first patch fixes the bugzilla page (bug 136080) and also the
original URL. I'm going to back out that patch and leave the table patch in.

bug 136078 has nothing to do with this bug (I tested it with my changes backed
out and it made no difference). 

Comment 36

[Marc Attinasi]

BlockFrame patch backed out:

/cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.496; previous revision: 3.495

I left the table patche in because it fixes this bug, and removing just the
block frame patch fixes all of the reported regressions.

Marking FIXED again.

Comment 37

[Chris Petersen]

Verified on the April 09th build (2002-04-09-03).

Comment 38

[Jacob Kjome]

Well, the page doesn't flicker at the URL anymore, but btestcase #2
( http://bugzilla.mozilla.org/attachment.cgi?id=76654&action=view ) still
displays a "ghost" flicker every time the the text is clicked (to move the
image).  Also, check out the menu bar at brainjar.com (
http://www.brainjar.com/dhtml/menubar/demo.html ).  After the menu is clicked on
to  drop down a menu, all mouseovers produce a flicker in the upper left hand
corner of the window.

does this bug cover these cases?

If so, it should be reopened.

Jake

Comment 39

[Marc Attinasi]

The testcase that is still broken
(http://bugzilla.mozilla.org/attachment.cgi?id=76654&action=view) needed the
block frame patch that was backed out. I'd prefer a new bug on that specific
issue rather than expanding this one. Also, if there are any real sites that
show this, that would be good to note.

Comment 40

[Jacob Kjome]

I reported the bug about fixing the block frame flickering as bug 136549.  
Please fix this one especially since you already have an existing patch.  It 
really makes Mozilla's dhtml look shoddy!

Jake

Comment 41

[Chris Petersen]

Verified on Windows Me May 19th branch (2002-05-19-05).

Comment 42

[Bill Mason]

*** Bug 56517 has been marked as a duplicate of this bug. ***

Comment 43

[Adam D. Moss]

Looks like the sort of thing that roc would have an opinion on.  CC:ing.

Comment 44

[Adam D. Moss]

Whoops.  Meant to draw attention to bug 136549 instead
of this closed one.