96736 - [FLOATER]The "Top 1000 Reviewer" is overwritten by text

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[hhwong]

For some reason (don't know if it's Amazon's fault), the "Top 1000 Reviewer"
icon is overwritten by text. It should be indented over rather than overwritten,
if IE 5.5 is supposed to be trusted.

Seen on both 0.9.3 and 2001082203. Using Win2K SP2.

Comment 1

[Grey Hodge (jX)]

WFM 2001082203

Comment again in a few days if you still see it on new builds.

Comment 2

[hhwong]

Attachment: Overwritten text

Comment 3

[hhwong]

Still happening with latest build (2001082909). Attached a JPEG of a screenshot
showing the problem.

Comment 4

[Doron Rosenberg (IBM)]

ok, i see that too now (2001082703) on win98, over to parser. Might need a
reduced testcase

Comment 5

[harishd]

I don't see the problem.

FYI: When you notice a problem please attach a copy of the problem URL ( so that
a reduced test case can be deduced ). Pointing to a dynamic page is not going to
help much.

Comment 6

[hhwong]

Attachment: Saved Amazon File

Comment 7

[Doron Rosenberg (IBM)]

Attachment: reduced html

Comment 8

[Doron Rosenberg (IBM)]

reduced html which shows bug

Comment 9

[harishd]

Resizing seems to correct the problem. Layout bug.

Comment 10

[Kevin McCluskey (gone)]

*Reduced* test case doesn't show the problem, but attachment (id=47667) does. It
would be good to have a much more reduced test case for this.

Comment 11

[Chu Alan]

i can't see this problem on the location of the attached screenshot, but i saw
ANOTHER problem on another top 1000 reviewers ... oops ...

i'm on linux build 2001122108.

Comment 12

[Chu Alan]

Attachment: No problem, right ?

no problem ...

Comment 13

[Chu Alan]

Attachment: No problem, right ?

changed to a smaller shot :)

Comment 14

[Eric Krock]

Still happening in 0.9.7 on WinME: (look at "Top 500 Reviewer" in Moz vs. IE 5.5)
http://www.amazon.com/exec/obidos/ASIN/0805211357/ref%3Dpd%5Fgw%5Fqpt%5F2/002-7224494-9220061

Comment 15

[Eric Krock]

Argh! If I do a Save As in Moz (or also a View Source on the page, copy and
save) to my hard disk and then open from the saved copy, I don't see the
problem! But I consistently see if I open the URL above. Attaching screen shot.

Comment 16

[Eric Krock]

Attachment: screen shot of Amazon page showing problem

Note that there's another image of the same width just above the image with the
problem. Seems like the second (problem) image is becoming inline (or overlaid)
to content rather than floating.

Comment 17

[Chu Alan #2]

Attachment: Strange ... i don't have any problem ...

Same position with the screenshot posted by Eric.
Shot on 2002011508/Linux

Comment 18

[Marc Attinasi]

Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.

Comment 19

[hhwong]

I thought I'd take a look at this now that it's been quite a few builds since I
first posted it -- and it looks like the problem has been resolved. 

Using: Build 2002030508, Win2K

Comment 20

[Bob Clary [:bc] (inactive)]

This is still occuring. 

To reproduce, 

open a relatively narrow window, clear your cache

open
http://www.amazon.com/exec/obidos/ASIN/B000060P7L/qid=1019956695/sr=8-1/ref=sr_8_67_1/104-4472014-4421540

Notice that the Top 500 image for Larry Marks overlaps the text. If you resize
the window, the image/text will display properly. After you can loaded the page
once, if you open the page again without clearing your cache you will not see
the probelm again. 

Comment 21

[Chris Waterson]

bclary: is this still happening on the trunk? I can't reproduce with a build
from today.

Comment 22

[Bob Clary [:bc] (inactive)]

I can reproduce with a trunk build from 2002-04-29-03/win2k. Make sure your
cache is cleared and that your window is narrow enough before loading the page.
Make your window no more that 660 pixels wide to see the overlap.

Comment 23

[Chris Waterson]

Gotcha. I see it now.

Comment 24

[Chris Waterson]

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

Comment 25

[Matthias Versen [:Matti]]

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

Comment 26

[R.K.Aa.]

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

Comment 27

[Sean Harding]

Attachment: Text overlapping with cover image

Is this the same problem? It looks like the same kind of thing, but it's a
different image. I've never seen this specific problem before, and I use the
Amazon.com site constantly. Reloading the page fixed it.

The screenshot is from 1.0rc1 on FreeBSD.

Comment 28

[Chris Waterson]

Attachment: proposed fix

Okay, here's what's going on. There is a block frame inside the table cell
that's getting its initial reflow. The table cell is asking it to compute its
max-width, so (joy of joys) we do an extra reflow at an unconstrained width and
height before the `real' reflow. (This was the part where my eyes popped open
as I realized that we do 2**n reflows in this case, where n is the level of
nesting of block frames inside the table cell.)

But back to this problem. The rub is that this extra `pre-reflow' reflow leaves
the floats in the space manager. These floaters have to be removed before the
`real' reflow, or else the space manager gets upset: it can't handle arbitrary
re-positioning of regions it would seem. We do something similar in
nsBlockFrame::ReflowLine.

This is almost certainly what is happening in bug 76135, as well.

Comment 29

[Chris Waterson]

cc'ing dbaron and kin for r= love.

Comment 30

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

Hmmm.  So if you're doing 2^n reflows, then this will only fix the problem for
floaters not within another block, because they'll be associated with the
floater caches of the lines of closest ancestor block, and that line will have
hit the second reflow too, and the lines wouldn't have been removed yet.

Comment 31

[Chris Waterson]

That sentence was just too long for me.

Comment 32

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

I'm saying you're not recurring into nested blocks, and you probably need to be.

Comment 33

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

(Also, I'm not sure there's really something 2^n going on, since I'd hope we
don't try to do a computemaxsize again on the inner block while we're already
doing an unconstrained reflow on the outer block just to do it.  That said, if
the inner block goes through the normal codepath rather than the one that hits
your code, which I'd think it should, then you'd still need to recur into the
child blocks.  But I should probably try to understand this code a bit better.)

Comment 34

[Chris Waterson]

Attachment: brute force

As dbaron notes, we need to remove all the floaters in all the lines of all the
blocks below the current block. This is a brute force approach to do just that.


On IRC, dbaron suggested a more elegant approach; namely, checkpointing the
space manager's state and rolling back to that state after the reflow. I'll
look into that next.

Comment 35

[Rainer Bielefeld]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0.0+) Gecko/20020511
Same problem with page
http://bugzilla.mozilla.org/show_bug.cgi?id=141768
First loaded: pics at the right side are overwritten (att. "zdf_wrong")
"confirm" Text Zoom 100%: page shown correctly (att. "zdf_correct")

Comment 36

[Rainer Bielefeld]

Attachment: page shown in a rorrect way

after "confirming Text Zoom"

Comment 37

[Rainer Bielefeld]

Attachment: page shown in a wrong way

Comment 38

[Rainer Bielefeld]

Attachment: I test to create attatchment

Comment 39

[Rainer Bielefeld]

Please excuse for att.:
att.http://bugzilla.mozilla.org/attachment.cgi?id=83355&action=edit

I had some problems to create att. and made a test for reporting 
bug 144194  .

Can yo delete that att? 

Comment 40

[Susie Wyshak]

added topembed

Comment 41

[Chris Petersen]

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

Comment 42

[scottputterman]

Will it be possible to get this fixed for mach v's rtm?

Comment 43

[Tony Tovar]

I'm still seeing this with 2002-06-11 (1.1 alpha).  Also, shouldn't the OS=All ?

Comment 44

[Chris Petersen]

Attachment: A partially reduced test case from amazon's site

I tried to reduce amazon's site to simple test case. However, this wasn't easy
as I thought since attempting to removed elements would cause problem not to
occur. I noticed most of the tables use a width of 100% which may be causing
this problem. The code that references the image looks like this:

<font face=verdana,arial,helvetica size=-1>

<b>Absolutely Fabulous .......</b>, January 25, 2001
<br>
<a href=....>

<img border="1"
src="http://g-images.amazon.com/images/G/01/v9/communities/top-reviewer-500.gif"

 width=64 height=30 BORDER=0 alt="Top 500 Reviewer" align=left>

</a> Reviewer: <b> <a href="...."> 

dowadiddi (see more about me)</a> </b> from Weston, Florida United
States</font><br>

Comment 45

[Chris Petersen]

Basically, I noticed the reviewer image is contained in a anchor element which
is a child of the font element.

Comment 46

[R.K.Aa.]

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

Comment 47

[Tony Tovar]

Did Amazon remove the problem code?  I can't find it on any of the sample URLs
anymore!

Comment 48

[Miloslaw Smyk]

Nope, it's still there. Look carefully.

Comment 49

[Kevin McCluskey (gone)]

nsbeta1+.

Comment 50

[Chris Waterson]

Hey kin, could you take a look at this one? I've got a sloppy solution in
attachment 82734 [details] [diff] [review], but dbaron points the way to a more elegant solution, which
I've parroted in comment 34.

Comment 51

[kinmoz]

Moving this to the top of the list of things to look at.

Comment 52

[Chris Petersen]

Still occurs in the OS X 2002-07-31-05 NB

Comment 53

[Mats Palmgren (inactive)]

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

Comment 54

[kinmoz]

Attachment: Patch Rev 1 (Save/Restore SpaceManager approach)

I'm assuming this is something similar to what waterson and dbaron had in mind
for a solution. waterson and dbaron, care to comment and review this?

Comment 55

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

Comment on attachment 95323 [details] [diff] [review]
Patch Rev 1 (Save/Restore SpaceManager approach)

+  while (mSavedStates){
+    SpaceManagerState *state = mSavedStates;
+    mSavedStates = state->mNext;
+    delete state;
+  }

This is a few lines of code that we're guaranteed not to need.	How about
removing it and replacing it with

NS_ASSERTION(!mSavedState, "states remaining on state stack");

I also would slightly prefer the names PushState and PopState since it is a
stack rather than a single saved state, although it's fine with me if you
prefer the ones you gave.  The comments in nsSpaceManager.h are also incorrect
-- they don't reflect that it's a stack.

It's probably worth noting in your XXX (which seem a little too noisy since
they don't really point out a bug, I don't think -- perhaps a single XXX would
be sufficent? :-) comments that restoring the interval map could actually lead
to bugs since there could be missed damage (when we move from position A to
position B to position C, where in the typical case A and C are the same (but
not always, which is the problem) we can't use only the damage in going from B
to C, since we might miss some that was only in the move from A to B).

Other than that, sr=dbaron.

Comment 56

[kinmoz]

Attachment: Patch Rev 1.1 (Addresses dbaron's comments)

Addressed all of dbaron's comments. I spoke with dbaron on IRC regarding the
|while| loop in the destructor, I'd rather keep the code in to avoid any
possible leaks. I did add the suggested assertion though.

Comment 57

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

Comment on attachment 95614 [details] [diff] [review]
Patch Rev 1.1 (Addresses dbaron's comments)

r/sr=dbaron

Comment 58

[Chris Waterson]

Comment on attachment 95614 [details] [diff] [review]
Patch Rev 1.1 (Addresses dbaron's comments)

r=waterson. great work!

Comment 59

[kinmoz]

Fix checked into the TRUNK:

  mozilla/layout/base/src/nsSpaceManager.cpp             revision 3.46
  mozilla/layout/base/src/nsSpaceManager.h               revision 3.28
  mozilla/layout/html/base/src/nsBlockReflowContext.cpp  revision 1.92

Should appear in the 08/19/02 QA builds.

Comment 60

[kurros (Wade Majors)]

verified on trunk

Comment 61

[Boris Zbarsky [:bzbarsky]]

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

Comment 62

[Tony Tovar]

I still see this bug using build 2002-08-24 (1.1 beta) on WinNT.

Comment 63

[kinmoz]

Tony, the fix was checked into the TRUNK (moz1.2alpha) *after* 1.1 branched so
it won't appear in any of the moz1.1 branch daily builds unless one of the
drivers deems this bug a must-fix for moz1.1.

Comment 64

[Christine Hoffman]

Just to note - this is also not fixed on Netscape 08_23_10_1.0.1 branch (tested
with Win2000)

Comment 65

[Jaime Rodriguez, Jr.]

From email from Evangelist, this is "a visual issue", but would be very visible
to a highly trafficed site (www.amazon.com). "Kin says the risk is low to medium
...  fix is isolated to a particular circumstance which causes this to fail so
it should not affect other aspects of layout." 

The fix has been verified on the trunk and baked for 2 weeks, with no known
regressions on the trunk. Adding edt1.0.2 and Mozilla1.0.2 keywords to nominate
for 1.0 branch landing.

Comment 66

[Madhur Bhatia]

so when is the fix being landed on branch?

Comment 67

[kinmoz]

Attachment: Patch Rev 1.1 Branch (Based on MOZILLA_1_0_BRANCH sources)

I'm ready to land this on the MOZILLA_1_0_BRANCH as soon as I have a thumbs up
from whoever is in charge of the branch at the moment.

Comment 68

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

Comment on attachment 98159 [details] [diff] [review]
Patch Rev 1.1 Branch (Based on MOZILLA_1_0_BRANCH sources)

a=[e-mail] for 1.0 branch.  Carrying forward r/sr.  Please change
mozilla1.0.2+ to fixed1.0.2 when checked in (and please checkin ASAP).

Comment 69

[Jaime Rodriguez, Jr.]

Marking as edt1.0.2+ per email from Chofmann on behalf of the EDT. Please replace
"mozilla1.0.2+" with the "fixed1.0.2" keyword once you have checked in the fix.
thanks!

Comment 70

[kinmoz]

Fix (attachment 98159 [details] [diff] [review]) checked into the MOZILLA_1_0_BRANCH:

  mozilla/layout/base/src/nsSpaceManager.cpp             revision 3.43.32.3
  mozilla/layout/base/src/nsSpaceManager.h               revision 3.26.32.2
  mozilla/layout/html/base/src/nsBlockReflowContext.cpp  revision 1.86.20.3

Comment 71

[Chris Petersen]

Verified in the 2002-09-12-08 1.0 (Windows ME) and 2002-09-12-05 1.0 (OS X ) Builds.