Text overlaps image (ESPN.com)

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
Layout
P3
normal
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Robert Rainwater, Assigned: kinmoz)

Tracking

({testcase, top100, topembed})

Trunk
mozilla1.2alpha
x86
All
testcase, top100, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(9 attachments, 2 obsolete attachments)

(Reporter)

Description

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

15 years ago
Confirming bug, 2002-07-25-08 (trunk) on Linux.
FWIW, the bug also occurs in 2002-07-10-21/Linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: top100
OS: Windows XP → All

Updated

15 years ago
Priority: -- → P3

Comment 2

15 years ago
Using 2002080204(trunk) on windows 2000, the refresh doesn't fix the problem,
the caption is still overlapping with the image.

Updated

15 years ago
Target Milestone: --- → Future

Comment 3

15 years ago
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. 
Assignee: attinasi → karnaze
Keywords: nsbeta1, topembed
Summary: Text Alignment Doesn't Work Until Refresh (ESPN.com) → Text overlaps image (ESPN.com)

Comment 4

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

Comment 5

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

Comment 6

15 years ago
I can't see the problem either using 9/5/11 trunk with Win2000.
(Assignee)

Comment 7

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

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

15 years ago
Created attachment 98036 [details]
Original page code

Comment 10

15 years ago
Created attachment 98037 [details]
Fixed page code

Comment 11

15 years ago
Created attachment 98039 [details]
Diff file

Comment 12

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

15 years ago
Created attachment 98072 [details]
another problem of this url

Comment 14

15 years ago
Created attachment 98096 [details]
reduced testcase from the url

Comment 15

15 years ago
I filed bug 167394 to deal with the problem in comment #12.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.2alpha

Updated

15 years ago
Attachment #98072 - Attachment is obsolete: true

Comment 16

15 years ago
Created attachment 98341 [details] [diff] [review]
patch to fix the bug
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

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

15 years ago
the patch works fine with the testcase.
but I have one question:
    what is the optimization?
Target Milestone: mozilla1.2alpha → Future

Updated

15 years ago
Target Milestone: Future → mozilla1.2alpha

Updated

15 years ago
Blocks: 166758

Comment 20

15 years ago
I mean where is the optimization, and how?

Comment 21

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

Updated

15 years ago
Keywords: edt1.0.2, mozilla1.0.2

Comment 22

15 years ago
*** Bug 146851 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 23

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

15 years ago
Yes.
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?
(Assignee)

Comment 26

15 years ago
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?
(Assignee)

Comment 27

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

15 years ago
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.
> 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.
(Assignee)

Comment 31

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

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

15 years ago
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.
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.
(Assignee)

Comment 36

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

15 years ago
->kin
Assignee: karnaze → kin
Status: ASSIGNED → NEW

Comment 38

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

r=karnaze on simpler patch in comment #36
Attachment #98601 - Flags: review+

Comment 39

15 years ago
But the var okToAddRectRegion should be removed too.
(Assignee)

Comment 40

15 years ago
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.
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")?
(Assignee)

Comment 42

15 years ago
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.
Status: NEW → ASSIGNED

Updated

15 years ago
Keywords: testcase
nsbeta1+
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 44

15 years ago
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?
Attachment #98601 - Attachment is obsolete: true
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.
Attachment #99589 - Flags: review+

Comment 46

15 years ago
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.
Attachment #99589 - Flags: superreview+
(Assignee)

Comment 47

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #99893 - Flags: needs-work+
(Assignee)

Comment 48

15 years ago
For anyone interested, I filed bug 169787 to track the sizing problems I saw
when dealing with right floaters during unconstrained reflows.
(Assignee)

Comment 49

15 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Whiteboard: FIXED_ON_TRUNK

Comment 50

15 years ago
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!
Keywords: edt1.0.2 → adt1.0.2

Comment 51

15 years ago
verified with 0922 trunk build.
Status: RESOLVED → VERIFIED
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.
Keywords: adt1.0.2 → adt1.0.2-
(Assignee)

Comment 53

15 years ago
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.
Keywords: adt1.0.2- → adt1.0.2

Comment 54

15 years ago
Minusing per comment 52
Keywords: adt1.0.2 → adt1.0.2-
(Assignee)

Comment 55

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

sr=dbaron
Attachment #101188 - Flags: superreview+

Comment 57

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

r=karnaze
Attachment #101188 - Flags: review+
(Assignee)

Comment 58

15 years ago
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.
Keywords: adt1.0.2- → adt1.0.2

Comment 59

15 years ago
cc'ing Buckland. OK for branch?

Comment 60

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

15 years ago
Petersen just moved to another group -- updated QA contact.  Amar, please verify 
fix on branch asap.
QA Contact: petersen → amar
(Assignee)

Comment 62

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

15 years ago
plussing for adt. please check in asap but make sure to get any additional
required approvals.
Keywords: adt1.0.2 → adt1.0.2+

Comment 64

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

a=chofmann for 1.0.2
Attachment #101188 - Flags: approval+
(Assignee)

Comment 65

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

Whiteboard: FIXED_ON_TRUNK

Updated

15 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+
(Assignee)

Updated

15 years ago
Keywords: mozilla1.0.2+ → fixed1.0.2

Comment 66

15 years ago
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.
Keywords: fixed1.0.2 → verified1.0.2

Comment 67

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