Last Comment Bug 159363 - Text overlaps image (ESPN.com)
: Text overlaps image (ESPN.com)
Status: VERIFIED FIXED
: testcase, top100, topembed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: P3 normal (vote)
: mozilla1.2alpha
Assigned To: kinmoz
: Amarendra Hanumanula
Mentors:
http://sports.espn.go.com/ncb/index
Depends on:
Blocks: 166758
  Show dependency treegraph
 
Reported: 2002-07-25 07:25 PDT by Robert Rainwater
Modified: 2002-10-24 17:37 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Original page code (57.88 KB, text/html)
2002-09-05 15:58 PDT, Susie Wyshak
no flags Details
Fixed page code (57.86 KB, text/html)
2002-09-05 15:59 PDT, Susie Wyshak
no flags Details
Diff file (2.46 KB, text/plain)
2002-09-05 16:00 PDT, Susie Wyshak
no flags Details
another problem of this url (2.75 KB, text/html)
2002-09-05 19:36 PDT, Jerry
no flags Details
reduced testcase from the url (547 bytes, text/html)
2002-09-06 00:08 PDT, Jerry
no flags Details
patch to fix the bug (4.91 KB, patch)
2002-09-08 12:49 PDT, karnaze (gone)
no flags Details | Diff | Splinter Review
Alternate approach? (687 bytes, patch)
2002-09-10 10:46 PDT, kinmoz
karnaze: review+
Details | Diff | Splinter Review
Test Case (table width doubled when attachment 98601 is used) (232 bytes, text/html)
2002-09-10 14:50 PDT, kinmoz
no flags Details
Alternate approach 2 (938 bytes, patch)
2002-09-17 16:48 PDT, kinmoz
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Kin's attempt at using the space manager during unconstrained reflows. (4.17 KB, patch)
2002-09-19 16:36 PDT, kinmoz
no flags Details | Diff | Splinter Review
Alternate approach 2 (MOZILLA_1_0_BRANCH version) (960 bytes, patch)
2002-09-30 16:54 PDT, kinmoz
karnaze: review+
dbaron: superreview+
chofmann: approval+
Details | Diff | Splinter Review

Description Robert Rainwater 2002-07-25 07:25:27 PDT
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1b) Gecko/20020725
BuildID:    2002072504

On the ESPN NFL page (and others), the caption under the main image is aligned
over the image.  Refreshing the page aligns the text correctly.

Reproducible: Always

Steps to Reproduce:
1. Go to http://sports.espn.go.com/nfl/index
2. Notice the caption for the image aligned over the image
3. Hit refresh
4. Notice the caption aligned correctly
Comment 1 Mats Palmgren (vacation) 2002-07-25 21:36:25 PDT
Confirming bug, 2002-07-25-08 (trunk) on Linux.
FWIW, the bug also occurs in 2002-07-10-21/Linux.
Comment 2 david grogan 2002-08-02 08:59:28 PDT
Using 2002080204(trunk) on windows 2000, the refresh doesn't fix the problem,
the caption is still overlapping with the image.
Comment 3 Susie Wyshak 2002-09-03 14:26:48 PDT
This looks bad, and this site is very very highly trafficked.

Changed summary (was Text Alignment Doesn't Work Until Refresh (ESPN.com) )

According to bclary, the issue is the image is aligned right and the br
clear="all is supposed to make the text appear below the image but does not.
Removing the align="right" on the image 'fixes' the problem. 
Comment 4 karnaze (gone) 2002-09-05 14:55:16 PDT
I can't duplicate the problem, after trying several times with a cleared cache. 
Did the site change?
Comment 5 karnaze (gone) 2002-09-05 14:57:59 PDT
Or could this have been fixed by the patch in bug 96736?
Comment 6 Christine Hoffman 2002-09-05 15:01:42 PDT
I can't see the problem either using 9/5/11 trunk with Win2000.
Comment 7 kinmoz 2002-09-05 15:16:51 PDT
I'm not seeing the problem either, but we do seem to hit the code fixed by bug
96736 when loading the ESPN url in a TRUNK build.

I should also note that I can't reproduce this problem in a Netscape 7
(MOZILLA_1_0_BRANCH based) build either, which doesn't have the fix for bug 96736.
Comment 8 Susie Wyshak 2002-09-05 15:57:14 PDT
Actually that was the wrong URL. They use different code on different pages.
This one demonstrates the problem. I will attach the fixed code that bclary
created as the test case.

CORRECT URL: http://sports.espn.go.com/ncb/index
Comment 9 Susie Wyshak 2002-09-05 15:58:51 PDT
Created attachment 98036 [details]
Original page code
Comment 10 Susie Wyshak 2002-09-05 15:59:37 PDT
Created attachment 98037 [details]
Fixed page code
Comment 11 Susie Wyshak 2002-09-05 16:00:25 PDT
Created attachment 98039 [details]
Diff file
Comment 12 Jerry 2002-09-05 19:10:43 PDT
I find another problem about this url.
on the right side , below Search the web,
  there is some html code that shows in IE, but not mozilla.
Comment 13 Jerry 2002-09-05 19:36:02 PDT
Created attachment 98072 [details]
another problem of this url
Comment 14 Jerry 2002-09-06 00:08:19 PDT
Created attachment 98096 [details]
reduced testcase from the url
Comment 15 karnaze (gone) 2002-09-08 12:48:03 PDT
I filed bug 167394 to deal with the problem in comment #12.
Comment 16 karnaze (gone) 2002-09-08 12:49:31 PDT
Created attachment 98341 [details] [diff] [review]
patch to fix the bug
Comment 17 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-08 13:16:35 PDT
I don't understand how marking lines dirty on the first reflow (where they
should already be dirty) fixes the bug.  Could you explain what's going on here?
Comment 18 karnaze (gone) 2002-09-08 16:20:34 PDT
During the unconstrained reflow, the right floater is not added to the space 
manager, its rect gets meaningless x,y values, and the lines following the <BR 
clear=all> get positioned on top of where the floater eventually ends up (during 
the constrained reflow).

During the constrained reflow, the lines following the <BR clear=all> are not 
being marked dirty because the damage area is not being updated because of the 
optimization in FlowAndPlaceFloater:

  // If the floater's dimensions have changed, note the damage in the
  // space manager.
  if (region != oldRegion) {
    // XXXwaterson conservative: we could probably get away with noting
    // less damage; e.g., if only height has changed, then only note the
    // area into which the float has grown or from which the float has
    // shrunk.
    nscoord top = NS_MIN(region.y, oldRegion.y);
    nscoord bottom = NS_MAX(region.YMost(), oldRegion.YMost());
    mSpaceManager->IncludeInDamage(top, bottom);
  }

So, the optimization needs to exclude the case when the previous reflow on a 
right floater was unconstrained. 
Comment 19 Jerry 2002-09-08 22:52:39 PDT
the patch works fine with the testcase.
but I have one question:
    what is the optimization?
Comment 20 Jerry 2002-09-09 02:39:36 PDT
I mean where is the optimization, and how?
Comment 21 karnaze (gone) 2002-09-09 06:59:24 PDT
The optimization I was referring to is the code in comment #18 where the damage 
area is not added unless the floater's (region != oldRegion).
Comment 22 karnaze (gone) 2002-09-09 12:53:45 PDT
*** Bug 146851 has been marked as a duplicate of this bug. ***
Comment 23 kinmoz 2002-09-09 23:54:59 PDT
So I'm still curious why things render fine when you remove the <font> tags? Are
the (x,y,width,height) values for the floater, during the unconstrained reflow,
different when the <font> tags aren't there?
Comment 24 karnaze (gone) 2002-09-10 10:15:36 PDT
Yes.
Comment 25 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-10 10:20:16 PDT
So why is region == oldRegion if the floater has never been placed at a
reasonable position before?  Shouldn't oldRegion have some wacky x coordinate? 
Or is the origin (0,0) in both cases?
Comment 26 kinmoz 2002-09-10 10:46:21 PDT
Created attachment 98601 [details] [diff] [review]
Alternate approach?

To answer dbaron's question, oldRegions is (0,0,0,0) and region is
(0,0,width,height) during the unconstrained reflow.

I did some poking around, and it seems to me that there is an implicit
assumption made in nsBlockReflowState::FlowAndPlaceFloater() that if we're
doing a reflow with an unconstrained width, that we will be called again at
some point to properly place the right-aligned floater at the right edge. I
gathered that because during the unconstrained reflow, we actually place the
right-aligned floater at the left edge of the available space.

That said, I can't think of any reason why we wouldn't want to add the floater
to the space manager during the unconstrained reflow. This would allow any text
that comes after it to flow around/under it ... and when the 2nd reflow comes
around we will either move it, resulting in everything being damaged properly,
or leave it where it should be, in which case nothing should be done anyways.

I was able to fix the problem with this patch. Am I missing something?
Comments?
Comment 27 kinmoz 2002-09-10 10:49:28 PDT
Bah hit submit before I finished answering dbaron's question.

During the unconstrained reflow, oldRegion is empty (0,0,0,0) and region is
given dimensions but remains at (0,0). When we do the 2nd constrained reflow,
the width of the table cell is exactly the width of the floater, so oldRegion
and region are calculated to be the same value.
Comment 28 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-10 10:53:09 PDT
OK, so things at least make some sense.  Adding to the space manager no matter
what seems reasonable -- after all, hopefully we won't use the result of the
unconstrained reflow when right floaters are involved, but if we do, it will
probably be better if we've added to the space manager.  If we want to keep that
optimization (if it is one), it seems like we need to use a bogus X coordinate
(NS_UNCONSTRAINEDSIZE?) for the frame origin, although that might introduce
other problems.

That said, regarding attachment 98601 [details] [diff] [review] -- why not just add to the space manager
no matter what?  (What does the oldRegion == region check really mean, there?)
Comment 29 karnaze (gone) 2002-09-10 12:09:08 PDT
Besides a smaller patch, I'm not sure what is gained by adding the floater to 
the space manager during the unconstrained reflow since its (x,y) values are not 
meaningful. I would be interested in knowing of other advantages.

What may be lost is some efficiency, although I'm not sure how expensive it is 
to add things to the space manager. Kin's patch may not be sufficient as is, 
because there are probably test cases that will hit waterson's optimization 
(region != oldRegion), if we let the floater have x=0. So assumming that 
waterson's optimization has some value, a bogus x value probably needs to be 
given. 

>That said, I can't think of any reason why we wouldn't want to add the floater
>to the space manager during the unconstrained reflow. This would allow any text
>that comes after it to flow around/under it ... and when the 2nd reflow comes
>around we will either move it, resulting in everything being damaged properly,
>or leave it where it should be, in which case nothing should be done anyways.

I don't think this would be achieved because the floater has not been added in 
the right place during the unconstrained reflow.

I guess I'm not yet convinced this is the right approach. If we are going to add 
the floater to the space manager just to trip the optimization, it seems a 
roundabout, less efficient way to do it. If dbaron is proposing that we remove 
the optimization, then we don't need either patch, but I bet we will take a perf 
hit during some incremental reflow cases.
Comment 30 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-10 12:47:30 PDT
> Besides a smaller patch, I'm not sure what is gained by adding the floater to 
> the space manager during the unconstrained reflow since its (x,y) values are not 
> meaningful. I would be interested in knowing of other advantages.

The advantage is that we can correctly apply damage based on the idea that we
only need to damage lines next to floaters during incremental reflows when those
floaters move.  This seems like a pretty important optimization to keep for
pages that have floaters, and adding the floaters to the space manager no matter
what seems to make sense -- I'm surprised we weren't doing it already.
Comment 31 kinmoz 2002-09-10 12:59:35 PDT
Prior to buster's nsBlockReflowState.cpp rev 3.371 checkin, we were *always*
adding floaters to the space manager. Unfortunately he checked in fixes for 10
bugs in the same checkin, so I can't tell right away what this was supposed to
fix. I'm currently trying to find the test case in one of those bugs, if any,
that my fix could possibly regress.
Comment 32 [:jesup] on pto until 2016/8/1 Randell Jesup 2002-09-10 13:09:03 PDT
If there's a chance of this being ready for Thursday 9/12, please do so and
submit for approval for 1.0.2.
Comment 33 karnaze (gone) 2002-09-10 13:26:27 PDT
>The advantage is that we can correctly apply damage based on the idea that we
>only need to damage lines next to floaters during incremental reflows when 
those
>floaters move.  

This is probably the intent of waterson's optimization

>This seems like a pretty important optimization to keep for
>pages that have floaters, and adding the floaters to the space manager no 
>matter
>what seems to make sense -- I'm surprised we weren't doing it already.

But since it involves incremental reflows (and an incremental reflows never 
happens right after an unconstrained reflow) is this a valid point?
Comment 34 karnaze (gone) 2002-09-10 13:32:08 PDT
I ran the regression tests with kin's patch (and quite a few local printing 
chagnes) and got no failures that could be attributed to it. If buster had a 
test case, it's possible that he didn't add it to the tests. If he was concerned 
about perf, then maybe the tinderbox tests could catch it.
Comment 35 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-10 13:55:42 PDT
It seems better to me to go with the simple solution.  Saying that floats are
always added to the space manager when we pass them is a simple invariant. 
Saying that there's an exception for right floats when we do an unconstrained
reflow makes this more complicated.  Having to set a bit to record the existence
of such floats and testing whether that bit is still set is more complicated
still.  I don't see the value of the first complication, so I'd rather remove it
than add a second one.
Comment 36 kinmoz 2002-09-10 13:57:56 PDT
I just finished running through all of the testcases attached to all of the bugs
mentioned in buster's checkin:

  Bugs 588, 18545, 18827, 19579, 22327 24782, 26512, 30124, 31849, 32846

and didn't notice any difference in layout between a build with and without my
fix. Also, in the cases that did hit the code I modified, there was never a case
where |oldRegion| and |region| were both equal during the unconstrained reflow,
so we were always adding the floater to the space manager ... so perhaps it
would be sufficient to just remove or comment out this line to fix the bug:

-      okToAddRectRegion = PR_FALSE;

and *always* add the floater to the space manager.
Comment 37 karnaze (gone) 2002-09-10 14:16:11 PDT
->kin
Comment 38 karnaze (gone) 2002-09-10 14:17:07 PDT
Comment on attachment 98601 [details] [diff] [review]
Alternate approach?

r=karnaze on simpler patch in comment #36
Comment 39 karnaze (gone) 2002-09-10 14:19:08 PDT
But the var okToAddRectRegion should be removed too.
Comment 40 kinmoz 2002-09-10 14:50:51 PDT
Created attachment 98634 [details]
Test Case (table width doubled when attachment 98601 [details] [diff] [review] is used)

Ok I found a test case that would produce a wider than expected table with my
alternate approach (attachment 98601 [details] [diff] [review]).

It looks like something takes into account the width of the floaters *inside*
the space manager when calculating the max width during the unconstrained
reflow.

This test case shows a scenario where the right aligned image is the widest
thing in a table that doesn't specify a width on itself or any of it's cells.
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-11 17:36:19 PDT
So why don't we end up with double-the-width table without if you do that
testcase with a left float (replace align="right" with align="left")?
Comment 42 kinmoz 2002-09-12 09:08:44 PDT
It gets doubled because there is code in nsBlockFrame::PlaceLine() that
specifically looks for the case where we have right floaters during an
unconstrained reflow. The comment in PlaceLine() assumes that the right floater
is placed way out at the unconstrained width, which we know it isn't because
buster's change places it at mAvailSpace.x, so it moves the floater to the right
edge of the line and combines the new floater rect with the current line's
bounds ... essentially doubling the width of the test case.  Commenting out that
code fixes the Test Case (attachment 98634 [details]) for me. 

I'm currently tracking down a regression in

   mozilla/layout/html/tests/table/bugs/bug7113.html

where the buttons in the "Portfolio" table are shifted a *tiny* bit.
Comment 43 Kevin McCluskey (gone) 2002-09-12 11:58:19 PDT
nsbeta1+
Comment 44 kinmoz 2002-09-17 16:48:17 PDT
Created attachment 99589 [details] [diff] [review]
Alternate approach 2

Ok I think I've debugged this problem to death ... this patch passes the
regression tests and uses the approach of temporarily moving right floaters out
to the unconstrained right edge during an unconstrained reflow. This yields the
exact same layout results as leaving it at the mAvailSpaceRect.x since the x
positions of the right floaters are totally ignored until the line is placed,
at which time the combined area of all right floaters are simply added to the
existing line width to yield the width we use for the constrained reflow.

I've spent the last couple of days working on an approach that would allow us
to always use the space manager ... and have something that works, but it
yields slightly smaller widths which results in slight layout differences.
<sigh>

These differences can be attributed to the fact that the right floaters widths
become part of the lineCombined area when they are in the space manager so as
you can imagine line widths will differ from the algorithm used currently.

While debugging with different test cases, I noticed that this code was
incorrectly using XMost():

-    if (NS_UNCONSTRAINEDSIZE != mAvailSpaceRect.XMost()) {
+    if (NS_UNCONSTRAINEDSIZE != mAvailSpaceRect.width) {

I think it's wrong because that condition can only be true during an
unconstrained reflow if mAvailSpaceRect.x was zero since XMost() adds x+width.

Comments?
Comment 45 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-17 19:23:04 PDT
Comment on attachment 99589 [details] [diff] [review]
Alternate approach 2

r=dbaron, although it would be nice to see the patch that you couldn't get to
work attached to the bug in case someone wants to pick it up later.
Comment 46 karnaze (gone) 2002-09-19 12:21:01 PDT
r=karnaze (so dbaron can change his to sr). This is the first time in the code
we have done arithmetic on NS_UNCONSTRAINEDSIZE, but I can't think of a better
value.
Comment 47 kinmoz 2002-09-19 16:36:19 PDT
Created attachment 99893 [details] [diff] [review]
Kin's attempt at using the space manager during unconstrained reflows.

At dbaron's request ... here's the space manager patch I was playing with.
Comment 48 kinmoz 2002-09-19 16:39:26 PDT
For anyone interested, I filed bug 169787 to track the sizing problems I saw
when dealing with right floaters during unconstrained reflows.
Comment 49 kinmoz 2002-09-20 06:55:31 PDT
Fix (attachment 99589 [details] [diff] [review]) checked into the TRUNK:

    mozilla/layout/html/base/src/nsBlockReflowState.cpp  revision 3.457

Fix should appear in 8am 09/20/02 QA builds.
Comment 50 Jaime Rodriguez, Jr. 2002-09-22 18:08:30 PDT
Changing edt1.0.2 to an adt1.0.2 nomination, as the ADT is providing checkin
approvals for all Netscape contributions the 1.0 branch. 

petersen: can you pls verify this as fixed on the trunk? thanks!
Comment 51 Jerry 2002-09-22 18:41:30 PDT
verified with 0922 trunk build.
Comment 52 Daniel Veditz [:dveditz] 2002-09-30 10:48:12 PDT
Looks like latest patch is needs-work, adt-.  When there's a good, tested branch
patch re-nominate with an explanation of how this fits Blackbird goals.
Comment 53 kinmoz 2002-09-30 11:26:33 PDT
The patch that we want to try and land on the branch is "Alternate approach 2"
(attachment 99589 [details] [diff] [review]), which has review and super-review, *not* attachment 99893 [details] [diff] [review]
which is the one marked as "needs-work".

As for Blackbird criteria ... this patch fixes a problem that is visible on a
"top site" where text is rendered over right floaters.

Clearing the adt1.0.2- for reconsideration.
Comment 54 Michael Buckland 2002-09-30 14:09:54 PDT
Minusing per comment 52
Comment 55 kinmoz 2002-09-30 16:54:36 PDT
Created attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)
Comment 56 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-30 17:11:25 PDT
Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

sr=dbaron
Comment 57 karnaze (gone) 2002-10-01 06:53:09 PDT
Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

r=karnaze
Comment 58 kinmoz 2002-10-02 16:08:51 PDT
Renominating for adt1.0.2, I have a branch version of the patch that is r='d and
sr='d and have tested it in my branch build.
Comment 59 Susie Wyshak 2002-10-02 16:25:36 PDT
cc'ing Buckland. OK for branch?
Comment 60 Michael Buckland 2002-10-15 16:02:41 PDT
Discussed in bBird team meeting.  The comment was made that this could not be
reproduced.  QA contact, please try to reproduce with todays branch build. 
Please update this bug with results.
Comment 61 gerardok 2002-10-21 11:29:00 PDT
Petersen just moved to another group -- updated QA contact.  Amar, please verify 
fix on branch asap.
Comment 62 kinmoz 2002-10-21 11:41:45 PDT
The fix is *NOT* on the branch yet, I've been waiting for approval.
buckland@netscape.com mentioned that someone could not reproduce the problem
with the branch build ... was that going to the live ESPN url?

http://sports.espn.go.com/ncb/index

The problem is that they probably changed their site, you will only see the
problem if the image is the first thing in the table cell, you won't see it
otherwise. You can still see the problem using the testcases attached to this bug.
Comment 63 Michael Buckland 2002-10-23 11:18:47 PDT
plussing for adt. please check in asap but make sure to get any additional
required approvals.
Comment 64 chris hofmann 2002-10-23 12:55:20 PDT
Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

a=chofmann for 1.0.2
Comment 65 kinmoz 2002-10-23 13:32:47 PDT
Fix (attachment 101188 [details] [diff] [review]) checked into the MOZILLA_1_0_BRANCH:

    mozilla/layout/html/base/src/nsBlockReflowState.cpp  revision 3.447.4.5

Fix should appear in 10/24/02 branch QA builds.

Comment 66 Prashant Desale 2002-10-24 17:16:56 PDT
I was not able to reproduce this bug on branch builds or trunk builds. They
might have changed the code for their web site. I did some testing to check for
possible regressions. I dint find any regressions since the above checkin was
made. Verified this bug with today's branch build.
Comment 67 Amarendra Hanumanula 2002-10-24 17:37:09 PDT
oops I have submitted the above comments using prashant's login. I have checked
the given attached testcases. They look fine with todays branch build:
2002-10-24-08-1.0. I see the problem clearly with the attached testcases in
yesterdays build: 2002-10-23-08-1.0.

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