Closed Bug 96736 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: Layout, defect, P1)

All
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: hhwong, Assigned: kinmoz)

References

()

Details

(Keywords: testcase, top100, topembed+, Whiteboard: [adt2 RTM] [FIXED_ON_TRUNK] [ETA 09/12])

Attachments

(15 files, 2 obsolete files)

145.67 KB, image/jpeg
Details
56.35 KB, text/html
Details
32.05 KB, text/html
Details
324.31 KB, image/jpeg
Details
159.18 KB, image/jpeg
Details
160.12 KB, image/jpeg
Details
86.17 KB, image/jpeg
Details
24.76 KB, image/png
Details
1.91 KB, patch
Details | Diff | Splinter Review
54.76 KB, image/jpeg
Details
51.68 KB, image/jpeg
Details
51.68 KB, image/jpeg
Details
32.10 KB, text/html
Details
7.88 KB, patch
waterson
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
6.46 KB, patch
jesup
: review+
jesup
: approval+
Details | Diff | Splinter Review
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.
WFM 2001082203

Comment again in a few days if you still see it on new builds.
Status: UNCONFIRMED → RESOLVED
Closed: 20 years ago
Resolution: --- → WORKSFORME
Attached image Overwritten text
Still happening with latest build (2001082909). Attached a JPEG of a screenshot
showing the problem.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
ok, i see that too now (2001082703) on win98, over to parser. Might need a
reduced testcase
Assignee: asa → harishd
Status: UNCONFIRMED → NEW
Component: Browser-General → Parser
Ever confirmed: true
QA Contact: doronr → bsharma
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.
Attached file Saved Amazon File
Attached file reduced html
reduced html which shows bug
Resizing seems to correct the problem. Layout bug.
Assignee: harishd → attinasi
Component: Parser → Layout
QA Contact: bsharma → petersen
*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.

Keywords: qawanted, top100
Summary: The "Top 1000 Reviewer" is overwritten by text → [FLOATER]The "Top 1000 Reviewer" is overwritten by text
Target Milestone: --- → mozilla1.0.1
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.
Attached image No problem, right ?
no problem ...
Attached image No problem, right ?
changed to a smaller shot :)
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
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.
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.
Same position with the screenshot posted by Eric.
Shot on 2002011508/Linux
Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.
Priority: -- → P1
Target Milestone: mozilla1.0.1 → Future
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
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. 


bclary: is this still happening on the trunk? I can't reproduce with a build
from today.
Assignee: attinasi → waterson
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.
Gotcha. I see it now.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.0
*** Bug 141059 has been marked as a duplicate of this bug. ***
*** Bug 141483 has been marked as a duplicate of this bug. ***
*** Bug 141768 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.0 → mozilla1.0.1
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.
Attached patch proposed fix (obsolete) — Splinter Review
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.
cc'ing dbaron and kin for r= love.
Blocks: 76135
Keywords: review
Keywords: qawanted
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.
That sentence was just too long for me.
I'm saying you're not recurring into nested blocks, and you probably need to be.
(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.)
Attached patch brute forceSplinter Review
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.
Attachment #82580 - Attachment is obsolete: true
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")
after "confirming Text Zoom"
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? 
added topembed
Keywords: topembed
*** Bug 137948 has been marked as a duplicate of this bug. ***
Keywords: topembedtopembed+
Will it be possible to get this fixed for mach v's rtm?
Keywords: nsbeta1
I'm still seeing this with 2002-06-11 (1.1 alpha).  Also, shouldn't the OS=All ?
Keywords: testcase
Hardware: PC → All
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>
Basically, I noticed the reviewer image is contained in a anchor element which
is a child of the font element.
Target Milestone: mozilla1.0.1 → mozilla1.1beta
Target Milestone: mozilla1.1beta → Future
*** Bug 154688 has been marked as a duplicate of this bug. ***
Blocks: 157236
Did Amazon remove the problem code?  I can't find it on any of the sample URLs
anymore!
Nope, it's still there. Look carefully.
nsbeta1+.
Keywords: nsbeta1nsbeta1+
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.
Assignee: waterson → kin
Status: ASSIGNED → NEW
Whiteboard: [adt2 RTM] [ETA Needed]
Moving this to the top of the list of things to look at.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.1beta
Still occurs in the OS X 2002-07-31-05 NB
*** Bug 161860 has been marked as a duplicate of this bug. ***
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?
Target Milestone: mozilla1.1beta → mozilla1.2alpha
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.
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.
Attachment #95323 - Attachment is obsolete: true
Comment on attachment 95614 [details] [diff] [review]
Patch Rev 1.1 (Addresses dbaron's comments)

r/sr=dbaron
Attachment #95614 - Flags: superreview+
Comment on attachment 95614 [details] [diff] [review]
Patch Rev 1.1 (Addresses dbaron's comments)

r=waterson. great work!
Attachment #95614 - Flags: review+
Whiteboard: [adt2 RTM] [ETA Needed] → [adt2 RTM] [ETA 08/19][FIX IN HAND]
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.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago19 years ago
Resolution: --- → FIXED
Whiteboard: [adt2 RTM] [ETA 08/19][FIX IN HAND] → [adt2 RTM][FIXED_ON_TRUNK]
verified on trunk
Status: RESOLVED → VERIFIED
No longer blocks: 157236
*** Bug 157236 has been marked as a duplicate of this bug. ***
I still see this bug using build 2002-08-24 (1.1 beta) on WinNT.
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.
Just to note - this is also not fixed on Netscape 08_23_10_1.0.1 branch (tested
with Win2000)
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.
so when is the fix being landed on branch?
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.
Keywords: approval
Whiteboard: [adt2 RTM][FIXED_ON_TRUNK] → [adt2 RTM] [FIXED_ON_TRUNK] [ETA 09/12]
Comment on attachment 98159 [details] [diff] [review]
Patch Rev 1.1 Branch (Based on MOZILLA_1_0_BRANCH sources)

a=rjesup@wgate.com for 1.0 branch.  Carrying forward r/sr.  Please change
mozilla1.0.2+ to fixed1.0.2 when checked in (and please checkin ASAP).
Attachment #98159 - Flags: superreview+
Attachment #98159 - Flags: review+
Attachment #98159 - Flags: approval+
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!
Keywords: edt1.0.2edt1.0.2+
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
Verified in the 2002-09-12-08 1.0 (Windows ME) and 2002-09-12-05 1.0 (OS X ) Builds.
Keywords: verified1.0.2
Keywords: fixed1.0.2
You need to log in before you can comment on or make changes to this bug.