Closed Bug 28553 Opened 20 years ago Closed 20 years ago

Table disappears when mouse over link (Unnecessary reframing for "special" frames)

Categories

(Core :: Layout, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: cesarb, Assigned: buster)

References

()

Details

(4 keywords, Whiteboard: [PDT+])

Attachments

(4 files)

From Bug Helper:
User-Agent: Mozilla/5.0 [en-US] (Linux; I)
BuildID:    1999122808
When you put your mouse over the first link in the 'library' section of
www.securityfocus.com, it changes color normally; when you put your mouse over
the title of the second article, the 'library' section turns empty.

Reproducible: Always
Steps to Reproduce:
1. Open the page
2. Go to the section titled 'library'
3. Make the mouse go over the title of the second article

Actual Results:  The whole 'library' section became empty.
Expected Results:  Color the link (like it does in the first article's title)
I've seen this bug for some time (and some milestones).

The Build ID is bogus -- it's in fact Debian's M13-4
You are using build 1999122808. It is about 2 months old... 
Please try with a new build.

PS I do not see any problems with win98 build 2000021908.
Confirmed on Linux build 2000.02.19.09.  Based on ezh's comments, marking pp.

Simplified testcase attached. Problem seems to occur because of unclosed <SPAN>
tag, the original page has a </CLASS> where there should be a </SPAN>.

Reassign to rickg/HTML Elements.
Component: Event Handling → HTML Element
Keywords: pp, testcase
QA Contact: janc → petersen
Summary: Part of page disappearing when mouse is over second item of the 'library' list of links → Table disappears when mouse over link following unclosed <SPAN>
Attached file testcase
Oops, forgot to reassign.
Assignee: joki → rickg
Status: UNCONFIRMED → NEW
Ever confirmed: true
Actually, the parser is working perfectly. The content model is fine too; this 
appears to be a bug in either the style system or in event handling. I'll start 
with style: markA, please take a look. (BTW: the testcase is pretty good).
Assignee: rickg → attinasi
Status: NEW → ASSIGNED
I actually digged into this somewhat and it seems like it's a incremetal reflow
problem, possibly in the table code (not sure). The content model is and remains
correct even after the table in the test case disappears, but the frame
hierarchy is changed.

Mousing over the link changes the state of the link to hover, this
(incorrectly?) causes an incremental reflow that reflows (part of) the document,
during this reflow mozilla loses part of the content of the table.

Actually, mozilla doesn't allways loose the content, sometimes it *adds* copies
of the content!

If you have a look at:

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=5562

you'll see tha same problem, but if you have a look at:

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=5564

which is the same file but with a <B> tag removed you'll see that every time the
mouse either enters or exits the link part of the content is copied!

I think the component should be "Layout" in stead of HTML element, does anyone
disagree?
OS: Linux → All
Hardware: PC → All
*** Bug 28705 has been marked as a duplicate of this bug. ***
May I make some comments?



First, to whoever said my build was old: I explicitly noted that the build ID

was bogus. Somehow, all builds I get as Debian packages seem to have bogus build

IDs.



Second, IMNSHO reflowing when a link state changes to hover is the correct

behavior, since the link size or even some wierder attributes for the link might

have changed (using CSS you can do pretty much anything).

Chris, The table says it wants to be 90 twips after a reflow. This looks like 
the table is reflowing on the hover, and further it looks like the Table Outer 
is changing the incremental into a resize... here is the dump of the reflow 
after the hover where the table asks to be 90 twips:

  TO::Rfl en 0166B390 rea=1 av=(8940,UC) comp=(8910,UC) count=10
    T::Rfl en 0166B3E4 rea=2 av=(8940,UC) comp=(8910,UC) count=11
      TRG::Rfl 0166B450 rea=2 av=(60,UC) comp=(60,UC) count=12
        TR::Rfl en 0166B490 rea=2 av=(60,UC) comp=(60,UC) count=13
        TR::Rfl ex 0166B490 des=(30,60)
      TRG::Rfl ex 0166B450 des=(60,60)
    T::Rfl ex 0166B3E4 des=(90,90)
  TO::Rfl ex 0166B390 des=(90,90)

I'm sure you can fix this faster than I can. 

Also, I removed the pp keyword since this is all platforms, and I changed to 
Layout
Component: HTML Element → Layout
Keywords: pp
forgot to reassign...
Assignee: attinasi → karnaze
Status: ASSIGNED → NEW
happens on top100 site hotbot, so nominating for beta1.  makes the page unusable.
Severity: normal → critical
Keywords: beta1, top100
Priority: P3 → P1
Whiteboard: [PDT+]
Status: NEW → ASSIGNED
Pierre, mousing over the link in the attachment is causing the table cell frame 
to be destroyed and then recreated. I'm not sure why a reframe reflow is needed 
or even a reflow for that matter. The table code eventually needs to be changed 
to handle this situation (which can still come up when the style problem is 
fixed), but the style problem should be fixed first.
Assignee: karnaze → pierre
Status: ASSIGNED → NEW
per PDT, this bug will become PDT- on Friday 2/25
Whiteboard: [PDT+] → [PDT+] Will become PDT- on 2/25
Whiteboard: [PDT+] Will become PDT- on 2/25 → [PDT+] w/b minus on 02/25
*** Bug 29068 has been marked as a duplicate of this bug. ***
Moving to PDT-.  Did not make the 02/25 deadline for beta1.  Putting on the 
relnote keyword.
Keywords: relnote
Whiteboard: [PDT+] w/b minus on 02/25 → [PDT-] w/b minus on 02/25
*** Bug 26015 has been marked as a duplicate of this bug. ***
*** Bug 26015 has been marked as a duplicate of this bug. ***
Taking over this one to help reduce Pierre's bug count.
Assignee: pierre → attinasi
Status: NEW → ASSIGNED
The SPAN does not have to be unclosed. I can recreate the problem with the 
following three criterion:
1) SPAN inside of a table cell
2) SPAN contains a <P>paragraph</P>
3) SPAN contains a link <A HREF-"foo">Link</A> after the <P>

This seems to cause a problem with the PrimaryFrame of the Anchor such that it 
cannot be found, causing a reframe when we really just want to see if the 
content stat has changed for the anchor:hover.

Here is the new testcase (the previous one is fine too, but is missing 
the </SPAN>):

<body>
<table border=1>
<tr><td>
<SPAN>
Here we open a SPAN tag and close it after the link
<P>This Paragraph is in the span, as is the following link</P>
<A HREF="http://www.yahoo.com">Mouse over this HREF tag to see entire table 
disappear</A>
</SPAN>
</td></tr></table>
</body>

I'm looking at why the PrimaryFrame is not found for the ANCHOR in 
nsCSSFrameConstructor::ContentStatesChanged
It looks like the primary frame is not found because of a side effect of a 
change Kipp made in how he handles blocks within inlines. I'll quote his 
explanation here so I don't get it wrong (I hope he don't mind):

"Rather than have the block frame be a child frame of the inline frame (which 
makes layout difficult), what he does is end the inline frame, create an
anonymous block to wrap the block frame, and then recontinue the inline frame. 

For example, for something like this: 
<SPAN>Some text <DIV>Nested div</DIV> more text</SPAN> 

We would have frames that look like this: 

inline < 
  text-frame: Some text 
> 
anonymous-block < 
  div < 
    text-frame: Nested div 
  > 
> 
inline < 
  text-frame: more text 
> 

That's what I seem to remember it would look like. 

The problem here is that from a content model perspective everything is a child 
of the SPAN element. However, if we have to search the tree (i.e.,
the frames are not in the map) we only end up finding what is in the first 
"inline" frame 

We don't know to check the anonymous-block or the inline frame below it because 
the frames aren't part of the same flow and the code currently
doesn't to do anything special for anonymous blocks. 

Basically if we're going to continue doing what Kipp did to the frame model then 
we need to make sure the code that searches for child frames knows
how to handle it." <troy chevalier>

I verified that by putting the inline frame created for the anchor into the 
primary frame map, so it can be found, the bug goes away. It looks like we need 
to modify the FindFrameWithContent routine in nsCSSFrameConstructor to take into 
account the case where an inline frame has created and anonymous block to wrap 
the block frame.
Adding Troy, Buster to CC list. 
*** Bug 30676 has been marked as a duplicate of this bug. ***
As we talked about, maybe the best solution is to put the primary frame in the 
map when we know we are creating an anonymous block to wrap the block frame that 
is in the inline frame.

A test case will be attached showing the same root bug in and out of a table. 
OUtside of a table the same problem finding the primary frame occurs, however 
the display is not as bad (it just flashes due to the reframe). 
Assignee: attinasi → buster
Status: ASSIGNED → NEW
No, adding the frames to the map when we're in that situation is not a very good 
solution. We would have to add all child frames recursively (which could be a 
lot), and we have to make sure that when incremental reflow changes happen we do 
the same.

That solution would be very fragile as well as unnecessarily increasing the 
size of the map
troy is right, we would have to recursively set all frames contained by the 
"special" frames into the primary frame map.  Just for fun, I tried the simple 
approach (3 lines of code) and while if fixes simple test cases, it doesn't fix 
the general problem at all.
Status: NEW → ASSIGNED
Just adding myself to the CC list and noting that as karnaze@ indicated on 
these 3, bug 29429 - bug 29511 - and bug 30408 , they appear to be dupes of this 
one ...
I've started a thread on n.p.m.layout.  I thought it was too involved to just 
dump into the bug report.  The thread is titled "Possible solution for 28553, 
need some feedback"  Please read the posting and respond there.  Thanks.
Whiteboard: [PDT-] w/b minus on 02/25 → [PDT-]
I have a candidate fix in hand for this bug.  It is not risk-free, and probably 
needs to go through a review/rewrite/test cycle before it's ready for prime 
time.  So I agree wiht the PDT- designation.  Fix will be ready when the M15 
tree opens for general development.
Whiteboard: [PDT-] → [PDT-] fix in hand
Target Milestone: M15
Augh, the same bug appeared in freenet.sourceforge.net (try the FAQ link in the

middle of the text). This is beginning to annoy me.

I talked to JAR and RickG separately about this today.  In each conversation, we 
waffled about how it would be great to get this in, since so many pages (some 
top100 pages) are effected visually, and because it's a huge performance win on 
pages that exhibit the pattern.  Of these effected pages, we believe most will 
be have the display problems (but not the performance problems) fixed by chris 
karnaze's fix for 29429.  However, Chris is only 50-50 on being able to get this 
in by tonight, and he is on vacation through the weekend after than.  If Chris 
does not check in his fix, then the fix for this bug would avoid his misbehaving 
code entirely, making those problem pages layout correctly, respond much 
faster, and use far less memory.  The problem he is working on would persist, 
but it would show up on fewer pages.
So, I propose we do this:
1. Wait and see if Chris gets the fix for 29429 in.  If so, this bug is PDT-.
2. If Chris can't get in the fix for 29429, then we check in my fix for this 
bug, pending review and approval by troy.

Your move, PDT team.
Whiteboard: [PDT-] fix in hand → fix in hand
The fix I just checked in for bug 29429 hides the symptoms of this bug (except 
for attachment #2 [details] [diff] [review]). The extra reflows are still happening, but the table code is 
handling it more gracefully.
The PDT+ plus status is provisional upon 4 things:
1. that the change go in under a pref
2. that the URL's affected (to your knowledge) are listed in this bug 
3. that the related bugs are cited here too
4. the usual pre-checkin rules.
Whiteboard: fix in hand → [PDT+]fix in hand
Ok:
1) The fix is ready, moved up a level as per Troy's request.  Troy, please 
review.
2) The fix is controlled by a pref, as per PDT's request.
3) Add dep bug 29429
4) urls are listed, there are more I'm sure be 3 should be sufficient.
FWIW, I believe this may go a long way towards fixing other bugs as well.
Depends on: 29429
Whiteboard: [PDT+]fix in hand → fix in hand
accidentally erased the [PDT+] rick had added
Whiteboard: fix in hand → [PDT+]fix in hand
Troy: I originally had this check the pref just once, but that doesn't work 
because we don't have a profile in mozilla the first time this is called, so we 
don't have a prefs impl to check against yet.  So until the PDT team is happy 
with the fix, we have to incur the cost of hitting the pref system every time we 
ask for a primary frame.  I suspect we can convince them to let us remove this 
very soon.
Keywords: perf
Summary: Table disappears when mouse over link following unclosed <SPAN> → Table disappears when mouse over link (Unnecessary reframing for "special" frames)
The PDT people are crazy. What do they want a pref for? The fix is fine, and 
it's very safe. Yes, I think you should check it in.

My only objection to the current code is that it looks like we're calling to get 
the pref service each time we enter FindPrimaryFrameFor().

That will be a performance problem. We either need to eliminate the silly pref 
or find a place to cache the pref service to avoid repeatedly loading it
*** Bug 31197 has been marked as a duplicate of this bug. ***
checked in fix, but won't mark this fixed because the code includes checking a 
pref every time through a frequently-called routine which is very inefficient.  
AFter a day or two of widespread testing, PDT team should allow me to remove the 
hacky pref.

Re-nominating on the basis of getting the pref the heck out of the code.
Whiteboard: [PDT+]fix in hand → fix checked in with pref
Are you totally you are convinced that pref is no longer needed? Will the 
overhead be noticible.  Can you cache the pref in a static variable?
Whiteboard: fix checked in with pref → [NEED INFO]fix checked in with pref
To add my two cents.
1. the pref was never needed in the first place. This fix is very low risk
2. yes it's a big performance problem
I am convinced the pref is unnecessary.  I am certainly willing to let folks 
bang on it for a day, and remove the code from the trunk and branch if 
necessary.
It is not trivial to cache the pref, because the code can be called before the 
pref file is loaded (the dialog that asks you to pick a profile uses core 
layout.) So I would need to detect when the profile has been loaded, and I don't 
know how to do that.  In any event, it's not worth spending time on for code 
that should be removed.
my two cents, this pref is showing up in my quantify work as well for loading a 
mail folder.
Putting on PDT+ radar for beta1 to .....Please remove the pref from the trunk 
this weekend or Monday, we can remove from the branch on Tues.
Whiteboard: [NEED INFO]fix checked in with pref → [PDT+]fix checked in with pref
*** Bug 31435 has been marked as a duplicate of this bug. ***
To land the "pref-removal," just page-chofmann and schedule a slot.
This seems like an easy change... so it would be nice to see it land RSN.
Whiteboard: [PDT+]fix checked in with pref → [PDT+]fix checked ; Now need to remove pref control
Whiteboard: [PDT+]fix checked ; Now need to remove pref control → [PDT+] fix (w/o pref) checked into trunk. waiting time slot for netscape beta branch
*** Bug 31644 has been marked as a duplicate of this bug. ***
fixed check in to both branch and trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] fix (w/o pref) checked into trunk. waiting time slot for netscape beta branch → [PDT+]
Verified on Linux build 2000.03.14.18.
Bug 30007, bug 30676, bug 31435, bug 31644, and bug 32052 reported mouseovers
recreating table cells etc.  Some of these have been marked duplicates of this
bug.  All of them, except bug 30676, still show the buggy behavior on Linux
build 2000.03.15.12-nb1b. So either these were not duplicates, or this bug
should be REOPENED.
any of those other bugs that are not fixed should be reopened
Looks like some of the bugs marked as dupes of this were neglected... bug 31137
looks more like a dupe of 29429 but it's still marked as dupe of this one (and
it's still an issue). Whoever checked and reopened the (wrong) dupes only
checked the bugs referenced by zach's posting but the posting missed half of the
dupes.
Bug 31137 is still open and assigned to buster.  I opened the ones where content
was duplicated (as opposed to where content disappeared).  These are now all
consolidated in bug 31644, and nisheeth is on it.  This leaves bug 28705, bug
29068, and bug 26015, all of which seem to be fixed now.
*** Bug 32421 has been marked as a duplicate of this bug. ***
Fixed in March 22 nd build for Mac, Win 98, and Linux.
Status: RESOLVED → VERIFIED
Bug 29511 which appears to be a dupe of this is not fixed. I'm using 2000040815.
You need to log in before you can comment on or make changes to this bug.