Closed Bug 129350 Opened 23 years ago Closed 23 years ago

[PATCH][RR]USPS.com - Mouseover link does not become a pointer

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: susiew, Assigned: dbaron)

References

(
URL
)

Details

(4 keywords, Whiteboard: [adt2])

Attachments

(4 files, 6 obsolete files)

Simple testcase.
(deleted), text/html
Details
Another testcase
(deleted), text/html
Details
markup, frames and style dumps
(deleted), text/plain
Details
patch that also fixes bug 88154 and friends
(deleted), patch
dbaron
: review+
hyatt
: superreview+
asa
: approval+
Details | Diff | Splinter Review
Critical usability problem: 1. go to http://www.usps.com/shop 2. mouse over main categories at left like "Stamps by Rate" Then click. Expected: Cursor becomes a pointer so you know you're mousing over a link. Result: Cursor does not become a pointer, but clicking works as expected. Thus a new user does not know it's actually a link. (including me!! http://bugzilla.mozilla.org/show_bug.cgi?id=122010#c3 ) REGRESSION -- In 20011128 (Netscape 6.2.1) the cursor becomes a pointer. Problem occurs on Win2000 and Linux on recent commercial builds. (Note: Mozilla on Linux is really messed up so not a good test for this.)
added regression, topembed, nsbeta1 keywords
Keywords: nsbeta1, regression, topembed
Changing platform to all. Also confirmed with Linux.
OS: Windows 2000 → All
This could be a dup of bug 88688. I am investigating. We may need to mark dup and forward the keywords since it's very important site (in the top 100 sites).
"Mozilla on Linux is really messed up so not a good test for this" Is it ? Anyway, on Linux the regression occurred between nightlies 2001121808 and 2001121908, long after the bug mentioned in Comment #3 was filed.
Whoops - I didn't think how that read: I wasn't referring to mozilla...I meant, that page on usps.com is very messed up when you use mozilla on Linux. All of the drop down menus are fully expanded. However I realized that you can still mouseover section links to see the problem.
Attached file Simple testcase. (deleted) —
When the style element is after the link, problem. When the style element is not there, works. I talked with harishd and we conducted some tests - this could be layout.
-> layout. Ccying David Baron since it's pretty important style issue.
Component: Browser-General → Layout
And reassigning to the default component owner.
Assignee: asa → attinasi
QA Contact: doronr → petersen
Comment #5: "All of the drop down menus are fully expanded ..." Oh I see; I had to look the page with NS4XX to understand what it should look like. My Comment #4 refers only to cursor not becoming a "hand" since build 2001121908, the popup menues are shown expanded in all old Linux builds I have. (This looks serious, is it Linux only bug, or all platforms, or evangelism ?)
Changing Priority to P2.
Priority: -- → P2
Adding top100
Keywords: top100
Attached file Another testcase (deleted) —
Could it be an event bubbling problem?
Keywords: testcase
dbaron, any thoughts?
Keywords: topembed → topembed+
Keywords: nsbeta1 → nsbeta1+
Target Milestone: --- → mozilla1.0
Looks to me like there is a style problem. My minimal case is: <a href=""> <div>text</div> </a> <style></style> The content model shows the <style> content being pushed up into the <head> but it is still wiggin' out something. I'm investigating what is happening when the content sink handles that style tag - my guess is that it is disrupting the style system somehow...
Status: NEW → ASSIGNED
So, just in case anyone cares (or remembers something that may be relevent) this worked OK in the 11/28 build, but is broken in the 1/15 build. I did not test any in between there though - sorry, that is as much archaeology as I could stand.
Some more archaeology: Works on Linux nightly 2001121808, broken the next day, 2001121908.
Thanks R.B. - that made it easy! Hyatt broke this fixing bug 115787. I think the reasoning in that bug is flawed: while we may not need to rebuild the entire rule tree when a stylesheet is added, we do need to reframe. Forcing a reframe in this case fixes the bug, so I'm going to post a patch to do that and have hyatt pass his judgement on it.
When a stylesheet is added, you do not need to reframe, and if you put this back the way it was, you will spike the page load tests by 3% or so and cause the double-reframe CNN problem to come back. In other words, even though this is a regression, you'll cause another regression if you try to fix it by putting the old buggy reframe behavior back in. The problem here is probably that ReResolveStyleContext is buggy and isn't properly working in all cases.
Give this to me. I'll fix it.
>In other words, even though this is a regression, you'll cause another >regression if you try to fix it by putting the old buggy reframe behavior back >in. OK, I don't want to cause and perf regressions... >Give this to me. I'll fix it. If you want to fix it feel free. I would like to muddle through and see what I can learn about what ReResolveStyle is doing wrong as we appear to have a number or bugs related to problerms therein. If you have any hints or guesses please post them.
I think ReResolveStyle needs to be aware of the 'special' sibling used for block-in-inline situations. At least, I don't see us reresolving the style on the special sibling frames - maybe that's why the problems are mostly (always?) with block in inline cases.
The problem is that we are not correctly handling the special block in inline frames in ReResolveStyleContext. I spent a day trying to fix this by detecting special frames and then making sure that the style and pseudo style for those frames are resolved correctly, and that the frames' contexts are updated correctly. However, something else is causing the updated style on those frames to get lost (specifically the special anonymous block) to be retained. So, for the patch: In ReResolveStyleContext we check for special frames and, if one is found, we set the change hint to be a FRAMECHANGE on the _containing block_ of the special frame. Thus, we ultimately end up doing a ReframeContainingBlock for the special inline frames, just like we do in ContentAppended (and everywhere else we have to muck with special frames). This seems the safest approach since incremental management of the special frames this close to Moz 1.0 is somewhat risky. The good news is that this fixes at least one other problem with ReResolveStyleContext on block-in-inline frames (bug 111238), and likely fixes several other IB bugs. The bad news is, it begs off the really hard work until bug 109181 is addressed (ReframeContainingBlock is called too often). There is also a performance penalty as there will be *some* reframing happening, but at least it is constrained to the containing block of the special frames, and only if there _are_ special frames being ReResolved. Also, I found a problem where ReResolveStyleContext was not correctly updating the undisplayedNode with the new style context that it resolved for the node. Unrelated, but it seemed wrong to not set the new style context when the DISPLAY is still NONE.
Blocks: 111238
This patch has *copies* of two methods from nsCSSFrameConstructor.cpp (IsFrameSpecial and GetIBContainingBlockFor) - If this patch is deemed acceptable, I'd like to move those methods to nsIFrameManager and have the FrameConstructor use them from there.
Actually, was the undisplayedContext being leaked in ReResolveStyleContext before my patch? see http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrameManager.cpp#1768
scratch that - there was no leak, I am blind.
Could you remind me what the frame structure looks like for this stuff -- and for which frame we set NS_FRAME_IS_SPECIAL? I'm wondering if we could solve this by using nsIFrame::GetStyleContextProvider.
Summary: USPS.com - Mouseover link does not become a pointer → [RR]USPS.com - Mouseover link does not become a pointer
for markup like: <a><div>text</div></a> we create frames like: inline (a) block* (a) block (div) text the style of the anon block* is a pseudo on :-moz-anonymous-block with a parent conext that has the same values (but is a different instance) as the inline frame's context. The 'special' annotation is put on the inline and the block, with only the inline having a frame attribute for the special sibling that is the anonymous block. I'll attach a frame and style dump for the markup described, so you can see the details more clearly.
Attached file markup, frames and style dumps (deleted) —
Comment on attachment 75303 [details] markup, frames and style dumps NOTE: the frame and style dumps are for the markup with the <style> tag removed - actually, changed to a bogus tag <xstyle> Probably obvious, but there you go.
adt2 per adt triage
Whiteboard: [adt2]
reassign to priority bugmail account
Assignee: attinasi → attinasi_layout
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Have Patch, seeking reviews. If David comes up with a better approach, it should be easy enough to replace this behavior with something new as this approach has low code impact.
Keywords: patch
Summary: [RR]USPS.com - Mouseover link does not become a pointer → [PATCH][RR]USPS.com - Mouseover link does not become a pointer
Comment on attachment 75269 [details] [diff] [review] PATCH to handle special block-in-inline frames in ReResolveStyleContext The problem with this patch is that it could enqueue both a framechange hint for a node and another hint for a child of that node, either before or after the framechange hint, which would cause a crash. (Consider the ReResolve resulting from the addition of a style rule: b { color: green; } Isn't that going to crash on the testcase: <div> <b>1</b> <span>a<p>b</p>c</span> <b>3</b> </div> (assuming the content model corresponds to the invalid markup), since we enqueue a repaint on the both "b" elements and a framechange on the div (the IB containing block). (I can't remember in which order we process the change list, although I remember there's a good reason for it.)
Attachment #75269 - Flags: needs-work+
This fixes the testcase in this bug, although i haven't yet figured out the correct way to handle |list3| in the IB splits. This patch handles the children in list3 correctly, but it doesn't handle the list3 anonymous frames themselves. I need to think about how to do that best. (It probably also fixes some issues (and VerifyContextParent debug spew?) if a ReResolve was called directly on a table by simplifying ReResolveStyleContext a bit by removing |aParentContextIn|.)
The comment in nsTableFrame.cpp in attachment 75984 [details] [diff] [review] is incorrect, and should say: // We must return the grandparent since our parent, the table outer // frame, returned this frame. However, if the parent is the block // that split an inline, we must return the "special" inline parent // (which is retrieved through a frame property on the anonymous // parent, i.e., the grandparent).
Attached patch revised patch (obsolete) (deleted) — — Splinter Review
I realized that the list3 problem isn't a problem, since the "anonymous" inlines are created just like any other inlines for the same content (in particular, they have no pseudo-element), so nothing needs to be done to fix them. So I removed the MarkIBParentFrame for the inline anonymous frames from the patch. This version contains that change and the comment change I mentioned above.
Attachment #75984 - Attachment is obsolete: true
Oh, one more change. In SplitToContainingBlock (which I still haven't tested yet): MarkIBSpecialParent(aPresContext, aState.mFrameManager, blockFrame, parent); should be: MarkIBSpecialParent(aPresContext, aState.mFrameManager, blockFrame, firstInFlow);
Attached patch revised (again) patch (obsolete) (deleted) — — Splinter Review
See comment 37.
Attachment #76018 - Attachment is obsolete: true
David, I like the patch overall. I dislike the part about the nsTableFrame having to understand about SPECIAL frames, however, and I think that if we want to go this route we need to encapsulate that somehow. I think it is unwise to spread that knowledge to the child-frames. Any ideas on that? The brute-force method I had tried does not crash the testcase you proposed, or others similar to it that I tried. Could be lucky that the frames are ending up with the same address - I cannot turn off the arenas right now - we process the changes in order, no priority given to the hint level AFAICT. Anyway, I like your approach better, though it looks like it has a wider impact and will require much more testing. Specifically, table code that was making use of the old GetStyleContextProvider method (karnaze had a slew of bugs and dups that we can test).
*** Bug 133109 has been marked as a duplicate of this bug. ***
Attached patch revised (again) (obsolete) (deleted) — — Splinter Review
I was already compiling this change (consolidating the duplication between {nsFrame,nsTableFrame}::GetParentStyleContextFrame in nsFrame::GetIBSpecialParent, which I think should at least partly address your comment about spreading the knowledge of NS_FRAME_IS_SPECIAL too far. I assume the bugs you mentioned that karnaze has are the duplicates of bug 92868, right?
Attachment #76024 - Attachment is obsolete: true
GetIBSpecialParent is certainly an improvement, it removes the implementation coupling but unfortunately leaves the semantic coupling. I guess there are times when we need the 'real' parent and times we need the IB parent, and the frame subclasses need to know this, unfortunately. Yes, bug 92868 is the one that I was thinking of. David, what's you assessment of the regression risk of your patch, or of mine for that matter? I like yours better if we can mitigate the fact that it will impact ALL cases, not just ones that actually have special inline blocks. The more I look at it, however, the less I am concerned by the risks...
I'm not too worried about the table frame knowing about block-within-inline issues. After all, the outer table frame essentially has 'display-role: block' in the CSS3 terminology, but we don't split outer/inner frames, so it has to know about both block things and table things. There is of course some regression risk with my patch, but I don't think it's all that bad considering the problems that it fixes (I expect I'll be marking a bunch of bugs duplicates of this one shortly). A good bit of it is moving code around and making little changes to the way we do things rather than actually changing what it is that we do. Your patch makes me nervous because it reminds me too much of the crash described in bug 117141.
>I'm not too worried about the table frame knowing about block-within-inline >issues. My concern has more to do with the 'child' knowing about how its 'parent' contains it, not with table-block cross-contamination. More generally, should any frame subclass have to worry about being in a special block-in-inline arrangement? I am not yet convinced that this kind of knowledge can be elimiated from the child frames though. At least we don't have too many frames that have to override GetParentStyleContextFrame. I agree that this will probably fix a LOT of bugs, and that is a good thing. I also like the fact that it attempts to solve the root problem directly rather than masking it with a 'just rebuild this junk' approach.
Comment on attachment 75269 [details] [diff] [review] PATCH to handle special block-in-inline frames in ReResolveStyleContext rendered moot by dbaron's much better patch
Attachment #75269 - Attachment is obsolete: true
Attachment #75269 - Flags: needs-work+
Comment on attachment 76041 [details] [diff] [review] revised (again) Review Comments: nsFrame::GetParentStyleContextFrame should probably have a comment about why GetIBSpecialParent is used. In CSSFrameConstructor there are three methods for dealing with the special IB frames: IsFrameSpecial, GetSpecialSibling and SetFrameIsSpecial. Should these be moved to nsFrame instead, and used in nsFrame and FrameConstructor? Also, do you need to check for first-in-flow in GetIBSpecialParent? nsFrame::GetIBSpecialParent has if (NS_OK == rv), probably should be NS_SUCCEEDED nsTableFrame::GetParentStyleContextFrame should ASSERT or check mParent before the STATIC_CAST (do you really need the cast? what does it provide anyway?) Please replace the comment that was there for nsTableOuterFrame::GetParentStyleContextFrame
> In CSSFrameConstructor there are three methods for dealing with the special IB > frames: IsFrameSpecial, GetSpecialSibling and SetFrameIsSpecial. Should these > be moved to nsFrame instead, and used in nsFrame and FrameConstructor? I didn't see any particular places where they would be useful, and I'd rather not make this patch too big. (Well, IsFrameSpecial could be used, but it's trivial.) > Also, do you need to check for first-in-flow in GetIBSpecialParent? Yes, if we ever do a reresolve in print preview or if we support multicol. I'll add it. > nsFrame::GetIBSpecialParent has if (NS_OK == rv), probably should be > NS_SUCCEEDED It's correct as-is, since GetFrameProperty can return two different success return values (the other one is: #define NS_IFRAME_MGR_PROP_NOT_THERE \ NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_LAYOUT, 1) ), and I really do want to check that it's NS_OK. > (do you really need the cast? what does it provide anyway?) It's a cast from nsIFrame* to nsFrame*, since we haven't yet removed the distinction. > Please replace the comment that was there for > nsTableOuterFrame::GetParentStyleContextFrame I'll add a comment, although not in the style of the old one, which didn't say anything beyond what the code said. I'll attach a patch later that addresses the comments in comment 46 other than the ones I said I wouldn't in this comment.
Taking.
Assignee: attinasi_layout → dbaron
Status: ASSIGNED → NEW
The testcases on bug 92868 and both duplicates are still fine.
*** Bug 111238 has been marked as a duplicate of this bug. ***
*** Bug 119585 has been marked as a duplicate of this bug. ***
*** Bug 120878 has been marked as a duplicate of this bug. ***
*** Bug 125056 has been marked as a duplicate of this bug. ***
*** Bug 128559 has been marked as a duplicate of this bug. ***
Note that bug 125056 had 6 duplicates.
Attached patch patch addressing Marc's comments (obsolete) (deleted) — — Splinter Review
This patch addresses Marc's comments above. The one issue I'm a little concerned about is the changes I made to walk the prev-in-flow chain in nsFrame::GetIBSpecialParent. For performance, I think it's important to have the prev-in-flow walking inside the NS_FRAME_IS_SPECIAL check -- otherwise we'll have the O(N^2) walking for every part of every re-resolve, which can be bad for significant chunks of text broken over many lines. I think having NS_FRAME_IS_SPECIAL set on all of the next-in-flows of any frame with it set was the original idea, judging from SetFrameIsSpecial in nsCSSFrameConstructor.cpp. However, nsFrame::Init didn't copy the NS_FRAME_IS_SPECIAL bit from the prev-in-flow. So (and this is the change I'm a little worried about, at least now, since I haven't gone through the code that checks the bit), I changed nsFrame::Init to copy the bit from the prev-in-flow, as it does for a bunch of other bits that should be copied when splitting frames (or copied from the parent). I also cleaned up the bit-twiddling there and cleaned up some duplicated code in nsSplittableFrame.cpp that isn't needed since nsSplittableFrame::Init calls nsFrame::Init.
Attachment #76041 - Attachment is obsolete: true
Blocks: 116273
*** Bug 116273 has been marked as a duplicate of this bug. ***
Blocks: 122533
Blocks: 88154
I went through the uses of NS_FRAME_IS_SPECIAL and I'm comfortable with the change I made to nsFrame::Init. I did fix one case where DeletingFrameSubtree was doing too much work. I also noticed that a bunch of the other [RR] bugs had to do with placeholders, and I realized that what we were currently doing for placeholders in ReResolveStyleContext was completely wrong. So I fixed it -- this involved removing the method that I added to nsPlaceholderFrame and changing nsCSSFrameConstructor so that it initially parents the placeholder frame's style context differently (to its normal parent). This shouldn't make any difference since the placeholder frame is just an empty style-less inline anyway, except it simplifies the code and saves the need for us to worry about leaving placeholder frames with stale style contexts when we do a reresolve starting at the primary frame for an out-of-flow frame. I fixed nsFrame::GetParentStyleContextFrame and the same method on nsTableFrame by adding another non-virtual method to nsFrame to do the placeholder work so that we handle out-of-flow frames correctly, even if we start the style resolution on the out-of-flow frame. Finally, I added nsHTMLAtoms::mozNonElementPseudo to simplify (and make more accurate) the checking for what requires ResolveStyleContextForNonElement -- something I should have done when I created that method. I checked all the uses of nsIStyleContext::GetPseudoType to make sure this wouldn't cause any problems. I'm going to mark the bugs fixed by the placheldor work as duplicates of bug 88154.
Attachment #76136 - Attachment is obsolete: true
Comment on attachment 76230 [details] [diff] [review] patch that also fixes bug 88154 and friends Could the comment for DoGetParentStyleContextFrame mention that it takes care of dealing with the special IB parent? That would be useful to other frames that might want to use it. this assertion seems wrong: + if (pseudoTag == nsHTMLAtoms::mozNonElementPseudo) { + NS_ASSERTION(localContent, + "non pseudo-element frame without content node"); if the 'if' is entered, then it needs no content, so why assert? Other than that, this looks like a great patch! [s]r=attinasi
I'd like to leave that assertion, since it's asserting a reasonably important invariant (although it would probably be a bit of work for a caller to break it, but who knows...). Comment in nsFrame.h fixed to read: // Do the work for getting the parent style context frame so that // other frame's |GetParentStyleContextFrame| methods can call this // method on *another* frame. (This function handles out-of-flow // frames by using the frame manager's placeholder map and it handles // block-within-inline by calling |GetIBSpecialParent|.)
Comment on attachment 76230 [details] [diff] [review] patch that also fixes bug 88154 and friends sr-hyatt
Attachment #76230 - Flags: superreview+
Comment on attachment 76230 [details] [diff] [review] patch that also fixes bug 88154 and friends Noting Marc's review. Also, I realized while explaining this to hyatt on IRC that I should have called the SpecialParent the SpecialPrevSibling. (It started off as one, but soon became the other when I realized it was a lot more efficient.) Unless somebody objects I'm probably going to make this name change before I check it in.
Attachment #76230 - Flags: review+
That's fine.
Comment on attachment 76230 [details] [diff] [review] patch that also fixes bug 88154 and friends a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76230 - Flags: approval+
*** Bug 133271 has been marked as a duplicate of this bug. ***
Fix checked in 2002-03-26 18:38 PST. Did someone want this on the 0.9.9 branch? (Is that what topembed means?)
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
having embeddors pick this up at mozilla 1.0 branch should be good enough --thanks
*** Bug 122533 has been marked as a duplicate of this bug. ***
*** Bug 134519 has been marked as a duplicate of this bug. ***
*** Bug 132630 has been marked as a duplicate of this bug. ***
Marking verified in the April 10th (2002-04-10-03) Windows ME and OS X April 11th (2002-04-11-08) build.
Status: RESOLVED → VERIFIED
*** Bug 125274 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: