Closed Bug 28553 Opened 20 years ago Closed 20 years ago
Table disappears when mouse over link (Unnecessary reframing for "special" frames)
303 bytes, text/html
551 bytes, text/html
1.41 KB, patch
|Details | Diff | Splinter Review|
reworked fix, based on Troy's feedback. Moved up a level, replaced recursion with iteration. Also can skip the fix based on a pref now.
1.59 KB, text/plain
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.
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
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?
*** 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
forgot to reassign...
Assignee: attinasi → karnaze
Status: ASSIGNED → NEW
happens on top100 site hotbot, so nominating for beta1. makes the page unusable.
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.
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
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
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.
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.
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 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.