157055 - Text doesn't show up on Lycos mail front page (layout broken by focus)

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Lawrence Teo]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1a+) Gecko/20020711
BuildID:    2002071119

The content on this page does not show up properly. There's supposed to be text
under the "Communicate with your family and friends" banner.

It used to show in previous versions of Mozilla and other browsers. I'll create
screenshot attachments to show what I'm talking about.

I'm not sure if this bug is related to Bug 150232.

Reproducible: Always
Steps to Reproduce:
Go to http://mail.lycos.com/

Actual Results:  Nothing shows up under the "Communicate with your family and
friends" banner.

Expected Results:  Page content shows up under the "Communicate with your family
and friends" banner. You should see:

"Lycos Mail is a full featured web-based email solution. Lycos Mail now offers..."
(and a whole bunch of text)

Comment 1

[Lawrence Teo]

Attachment: Actual Results: Mozilla - does not show content

This screenshot shows the http://login.mail.lycos.com/ page on Mozilla Build
2002071119. The text does not show up under the "Communicate with your family
and friends" banner.

Comment 2

[Lawrence Teo]

Attachment: Expected Results: Opera - shows content

This screenshot shows the http://login.mail.lycos.com/ page under Opera 6.01.
The text *shows* up under the "Communicate with your family and friends"
banner.

I tried using Netscape 4.78 to create this screenshot but it looks weird on
Netscape too. So I decided to use Opera.. hope that's ok with you.

Comment 3

[Andrew Schultz]

it looked ok on the first load for me.  reloading made it look as you have
described.  clearing the cache did not help.

tested with build 20020713.

DOM Inspector showed the relevant text positioned on the left side (near the
password box).  however, the "blinking box" appeared to have 0 width.

Comment 4

[Andrew Schultz]

Attachment: testcase

looks like table layout can't handle a focus event while it's doing the table. 
Just moving the Javascript below the table fixes the problem.

Comment 5

[Andrew Schultz]

marking NEW

regression between linux trunk builds 2002061108 and 2002061304.  1.0 branch
also exhibits this bug.

==> tables

Comment 6

[Andrew Schultz]

backing out bug 146831 from current CVS fixes the problem

Comment 7

[Chris Waterson]

Not on the branch, it doesn't.

Comment 8

[Chris Waterson]

I should be more specific. The attached testcase appears to be broken on the
branch both with and without dbaron's patch. (The original URL, mail.lycos.com,
seems to now _work_ both with and without dbaron's patch...Lawrence, do you know
if the content changed?)

Comment 9

[Andrew Schultz]

the URL still doesn't work for me with trunk and branch builds from 20020721...

Comment 10

[Andrew Schultz]

backing out 146831 from the trunk still fixes it for me.
backing it out of the branch also fixes it there (tested testcase and URL)

note that it sometimes shows up ok on the first try.  load the page and then hit
the "go" button or hit enter in the URL bar after the page has loaded.

Comment 11

[Lawrence Teo]

Chris, AFAIK nothing has changed. I did not save the HTML though, so I can't be
100% sure.

Still doesn't work on Build 2002072208.

Comment 12

[kinmoz]

Hmmm like Andrew, backing out dbaron's changes *seem* to fix the problem for me
in both the http://mail.lycos.com page and the testcase (attachment 91244 [details]) ...
BUT ... saving both the lycos page and the testcase locally and opening them up,
even *with* dbaron's changes in my tree, things render correctly.

This leads me to believe that this bug is influenced by something during frame
construction.

We've had bugs in the past (like bug 106489) where timing of the parser's
flushes affected the frame hierarchy and subsequent reflows because there was a
difference between the frame hierarchy constructed when all of the content was
complete, and the hierachy generated when the content trickled in and was
constructed via ContentAppended().

Changing Platform/OS to all since I see this on windows too.

Comment 13

[kinmoz]

ok, it looks like saving the test case out of the browser yields different html
<sigh>. That is it's prematurely closing the the <form> tag in the test case
saved to my local drive. Using view source on attachment 91244 [details] I was able to
restore the file back to what it was and now I see the bug ocassionally with my
local copy.

That is, I can only see the problem when going from a page to the test case, and
not when i reload or shift-reload the test case. I also don't see the problem in
viewer at all.

Comment 14

[kinmoz]

Ok, I think the scenario is this ...

The test case consists of an absolutely positioned div containing a table within
a table. The inner table contains both an input element and some script that
sets focus on the table.

When the script is processed, it tries to set focus on the input element, this
triggers some focus related global window callbacks registered only in mozilla
(note this bug is not reproducible in viewer) that call FlushPendingEvents(),
forces a reflow before the outer table's last cell is added.

After this reflow  happens, the last cell is added, so an incremental reflow is
targeted at it's table row. In the meantime an incremental reflow is also
targeted at the area frame for the canvas. When the reflow pl event is fired,
the  reflow path now contains both an incremental reflow for the area frame, and
one for the table row inside the absolutely positioned div.

When we go to reflow the area frame, it doesn't call incremental reflow for any
of it's absolutely positioned elements, because dbaron's modifications for bug
146831 only call incremental reflow for it's absolutely positioned elements if
the area frame itself isn't the target of a reflow command.

Comment 15

[kinmoz]

I think the solution here is to allow the incremental reflow to happen if the
area frame is a target, and there is a reflow path underneath that target frame
to one of it's children.

Comment 16

[kinmoz]

taking bug.

Comment 17

[kinmoz]

Attachment: Possible Fix

It turns out that only the 2nd set of expression changes in both
nsBlockFrame.cpp and nsInlineFrame.cpp (attachment 86443 [details] [diff] [review]) are needed to reduce
the CPU load for bug 146831.

waterson sat down with me and poked around and said that the 1st set of
expression changes should be unnecessary since
nsAbsoluteContainingBlock::IncrementalReflow() does all the necessary checks.

Applying this patch confirms that the CPU load remains roughly the same on
http://home.netscape.com with the 1st set of expressions restored back to what
they were and fixes the load problems with attachment 91244 [details] and the login page
at http://mail.lycos.com.

Comment 18

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

Comment on attachment 92779 [details] [diff] [review]
Possible Fix

I'd rather leave the HasAbsoluteFrames check since the 99.9% case is that it
doesn't.  Other than that this seems fine.

Comment 19

[kinmoz]

Attachment: Patch rev 1.1 (Leaves in the HasAbsoluteFrames() check)

Seeking r= and sr= for this patch now? dbaron? waterson?

Comment 20

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

Comment on attachment 92858 [details] [diff] [review]
Patch rev 1.1 (Leaves in the HasAbsoluteFrames() check)

sr=dbaron

Comment 21

[karnaze (gone)]

Comment on attachment 92858 [details] [diff] [review]
Patch rev 1.1 (Leaves in the HasAbsoluteFrames() check)

r=karnaze

Comment 22

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Drivers' approval. pls check this in asap, then replace "mozilla1.0.1+" with
"fixed1.0.1"

Comment 23

[Paul Wyskoczka]

Let's land this on the trunk today.  Please get drivers approval.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 92858 [details] [diff] [review]
Patch rev 1.1 (Leaves in the HasAbsoluteFrames() check)

a=asa (on behalf of drivers) for checkin to 1.1

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Approval noted for 1.0.1.  Please do some serious regression testing (including
CPU usage) of sites using absolute positioning after this is committed, since
there's been no baking on trunk, and back out if new regressions occur.  When
checked in, change mozilla1.0.1+ to fixed1.0.1.

Please also check DHTML test pages, since absolute positioning is common in those.

Check testcases/dups of bug 146831 as well.

Comment 26

[kinmoz]

Fix checked into the TRUNK:

    mozilla/layout/html/base/src/nsBlockFrame.cpp   revision 3.525
    mozilla/layout/html/base/src/nsInlineFrame.cpp  revision 3.182

Fix checked into the MOZILLA_1_0_BRANCH:

    mozilla/layout/html/base/src/nsBlockFrame.cpp   revision 3.496.2.15
    mozilla/layout/html/base/src/nsInlineFrame.cpp  revision 3.173.2.6

Fixes should appear in 07/29/02 QA builds.

Comment 27

[Jaime Rodriguez, Jr.]

amar: can you pls verify this as fixed on the 1.0 branch, and do very good
regression testing (including CPU usage, and Pageloading Perf).

Comment 28

[John Morrison]

I've run pageload tests on win32 and linux, comparing the performance for 
branch 07/28 and 07/29 builds -- no significant difference noted for either
platform. I've tried some tests with abs-pos animations, and didn't note any
change in performance (but if someone has further tests to consider, please
note them here (and/or test them)). I'll check Mac 9 / OSX tomorrow.

Comment 29

[Jaime Rodriguez, Jr.]

amar: can you pls verify this as fixed on the 1.0 branch, then replace
"fixed1.0.1" with "verified1.0.1"? thanks!

Comment 30

[Amarendra Hanumanula]

Given URL and the testcase works fine with the branch builds. I have done some 
regression testing on urls and testcases. which uses "position: absolute" from 
bugzilla and found that performace is same with the builds 07/27 and 07/28. Some 
of these URLS which have DHTML with abosulte positioning also looks fine. The 
URLs are

http://glish.com/css/noah.asp
http://smaug.java.utoronto.ca/NS5-bugs/posn-bug.html
http://www.psa.neu.edu/error/
http://www.world-direct.com/mozilla/dhtml/75121/anim-test.htm
http://www.calstatela.edu/library/bi/ssotton/catalog2ns6.html
http://members.blackplanet.com/blackwithindian/
http://www.eflorence.com/eflorence/test_position.htm
http://www.ideaverona.com
http://www.bamdad.org/
http://www.geocities.com/drozenbaum/div_test2.html
http://www.fas.harvard.edu/~dbaron/css/test/sec1704b
http://www.vbug.co.uk/magazine/i6/tricks_of_the_trade.asp

http://bugzilla.mozilla.org/attachment.cgi?id=82387&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=49940&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=10680&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=13970&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=35294&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=10567&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=90002&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=90003&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=92206&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=92209&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=92210&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=85074&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=44245&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=65739&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=49477&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=10970&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=76049&action=view 

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Madhur Bhatia]

v