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.
110 bytes, text/html
Details
Another testcase
318 bytes, text/html
Details
markup, frames and style dumps
4.32 KB, text/plain
Details
patch that also fixes bug 88154 and friends
38.95 KB, 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. — 
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 — 
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.
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) — — 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) — — 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) — — 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. ***
Attached patch patch addressing Marc's comments (obsolete) — — 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
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: