159363 - Text overlaps image (ESPN.com)

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Robert Rainwater]

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 (inactive)]

Confirming bug, 2002-07-25-08 (trunk) on Linux.
FWIW, the bug also occurs in 2002-07-10-21/Linux.

Comment 2

[david grogan]

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]

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)]

I can't duplicate the problem, after trying several times with a cleared cache. 
Did the site change?

Comment 5

[karnaze (gone)]

Or could this have been fixed by the patch in bug 96736?

Comment 6

[Christine Hoffman]

I can't see the problem either using 9/5/11 trunk with Win2000.

Comment 7

[kinmoz]

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]

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]

Attachment: Original page code

Comment 10

[Susie Wyshak]

Attachment: Fixed page code

Comment 11

[Susie Wyshak]

Attachment: Diff file

Comment 12

[Jerry]

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]

Attachment: another problem of this url

Comment 14

[Jerry]

Attachment: reduced testcase from the url

Comment 15

[karnaze (gone)]

I filed bug 167394 to deal with the problem in comment #12.

Comment 16

[karnaze (gone)]

Attachment: patch to fix the bug

Comment 17

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

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)]

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]

the patch works fine with the testcase.
but I have one question:
    what is the optimization?

Comment 20

[Jerry]

I mean where is the optimization, and how?

Comment 21

[karnaze (gone)]

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)]

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

Comment 23

[kinmoz]

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)]

Yes.

Comment 25

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

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]

Attachment: 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]

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-4, no longer working on Mozilla)]

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)]

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-4, no longer working on Mozilla)]

> 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]

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

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

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)]

>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)]

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-4, no longer working on Mozilla)]

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]

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)]

->kin

Comment 38

[karnaze (gone)]

Comment on attachment 98601 [details] [diff] [review]
Alternate approach?

r=karnaze on simpler patch in comment #36

Comment 39

[karnaze (gone)]

But the var okToAddRectRegion should be removed too.

Comment 40

[kinmoz]

Attachment: Test Case (table width doubled when attachment 98601 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-4, no longer working on Mozilla)]

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]

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)]

nsbeta1+

Comment 44

[kinmoz]

Attachment: 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-4, no longer working on Mozilla)]

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)]

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]

Attachment: 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]

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]

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.]

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]

verified with 0922 trunk build.

Comment 52

[Daniel Veditz [:dveditz]]

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]

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]

Minusing per comment 52

Comment 55

[kinmoz]

Attachment: Alternate approach 2 (MOZILLA_1_0_BRANCH version)

Comment 56

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

Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

sr=dbaron

Comment 57

[karnaze (gone)]

Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

r=karnaze

Comment 58

[kinmoz]

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]

cc'ing Buckland. OK for branch?

Comment 60

[Michael Buckland]

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]

Petersen just moved to another group -- updated QA contact.  Amar, please verify 
fix on branch asap.

Comment 62

[kinmoz]

The fix is *NOT* on the branch yet, I've been waiting for approval.
[e-mail] 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]

plussing for adt. please check in asap but make sure to get any additional
required approvals.

Comment 64

[chris hofmann]

Comment on attachment 101188 [details] [diff] [review]
Alternate approach 2 (MOZILLA_1_0_BRANCH version)

a=chofmann for 1.0.2

Comment 65

[kinmoz]

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]

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]

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.