5693 - [ESM/CSS] :hover should be hierarchical

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now :active and :hover don't quite work right on non-link elements.  The
problems I see are as follows:

* They don't work if the click or the hover is over an inline element (i.e.,
any text).
* They don't work on multiple elements at once.  That is, a parent and its
child can both have :hover rules.  If the mouse is in the child's area (and in
the parent's, which is usually implied), then both should have their :hover
styles applied.

That is, they should be triggered roughly be events.

:active = between onMouseDown and onMouseUp (except that the onMouseUp doesn't
have to be in the element's area)
:hover = between onMouseOver and onMouseOut
:focus = between onFocus and onBlur

The above URL demonstrates the problem:

http://www.fas.harvard.edu/~dbaron/csstest/sec051103b

and I have a demonstration in javascript that shows how I think it should work
(except for the above comment about :active):

http://www.fas.harvard.edu/~dbaron/tests/nglayout/sec051103b_js

Comment 1

[joki (gone)]

Okay, maybe I'm just missing something here but its not at all clear to me that
the CSS spec says anything about what you should do with hover on a parent and
hover on a child.  If anyone has a URL to point me to I'd appreciate it.

Peter, you're more involved with this than I.  Any thoughts?

I do agree with the inline elements bit, though.  For DOM purposes text is its
own content and can have its own event handlers so the pseudo class 'events'
are getting grabbed by it.  I'll have to fix it but depending on what we decide
to do for parent/child relationships it might fix itself.

Comment 2

[Hixie (not reading bugmail)]

It wouldn't make sense if pseudo-classes only affected the top most element and
not any of their parents.

For example, imagine the following:
   <A ...> Hello <strong> Wonderful </strong> World! </A>

If you applied the rules
   A:hover { font-weight: bold; }
   STRONG:hover { color: red; }

... and your cursor hovers over the link, you expect the whole link to get bold. If you
hover over the strong bit, the link should still be bold, otherwise it looks like the
middle word is not part of the link!

IMHO, the :active and :hover pseudo-classes should apply to all elements that are
_at that point_. That is, it might not necessarily apply straight up the tree.

For example:

   <DIV style="position: fixed; top: 10px; left: 10px; width: 10px; height: 10px;">
   ONE
   </DIV>
   <DIV style="position: fixed; top: 15px; left: 10px; width: 10px; height: 10px;">
   TWO
   </DIV>

The two DIV's overlap, and so both could have ":hover" applied to them at the same
time. If the styles had the rule ":hover { color: red; }", then if you positioned your
cursor carefully, you should be able to make _both_ DIVs go red.

The editor team probably have some code already written to work out what
elements are at a point on the canvas, should that be required.

Comment 3

[Peter Linss]

I have to agree with David regarding :hover. I've raised this issue with the CSS
w/g and have received very little response, but what I have heard concurs with
this opinion (ie: :hover applies to the entire frame geometric nesting under
the mouse, not the content tree, and not just the top most frame or the frame
that accepted the mouse events).

(I think that the way :hover is implemented today is also useful, but not what
was indented for :hover. I think :hover is being overloaded a bit to do too many
things. I'll take that up with the w/g, perhaps we'll invent a new pseudo class
or two.)

:active should work essentially like it does now. ie: simply mousing down on
anything doesn't make it active. The content has to be able to become "active"
in some way that makes sense for the content. Most content can not become
active.

CSS3 has a UI proposal whereby you can stylistically describe what content can
become active, and how (ie: is it a button, a pulldown, a editable field,
etc...). Until then, the current system works.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I disagree about :active.  I think anything that can have an
onClick/onMouseDown/onMouseUp/etc. handler can be active.  That is, some
element could be like a link, but handled through javascript event handlers.
(I think that's a better technique than creating a dummy link element that
doesn't actually have a real HREF, and putting handlers on that.)

Anyway, right now, active *does* work on block level elements, but only the
topmost one.  That should definitely be changed, either to Peter's solution or
to mine, or something else logical.

So should :active work on:

 - only links?
 - only elements where clicking does something, and only be set on the
   element that handles the click?
 - anything, and the way hover works?

Comment 5

[Hixie (not reading bugmail)]

I strongly support the "anything, and the way hover works" point of view.

This allows all the previous possibilities, does not restrict what can be done,
and is the most generic option (implying that less hard coding will be
involved i.e. less time will be spent implementing it).

If someone does not want _all_ their documents' elements to be affected by an
:active rule, then it is simply a matter of making it specific to what is
actually wanted.

In other words, instead of

   :active { color: red; }

...one would use

   a:active { color: red; }

This means that links containing other elements can still be made active. For
example, imagine the following (which is intentionally similar to the :hover
argument I used above):

   <A ...> Hello <strong> Wonderful </strong> World! </A>

If you applied the rules

   A:active { color: red; }
   STRONG:active { font-size: 150%; }

...and you click on the link, you expect the whole link to go red. If you
click over the strong bit, the link should still go red, otherwise it looks
like the middle word is not part of the link!

Comment 6

[Hixie (not reading bugmail)]

The following test page should also come in useful:
   http://www.bath.ac.uk/%7epy8ieh/internet/eviltests/pseudoclasses1.html
It also demonstrates some problems you have with :hover and :active on links.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've slightly changed my opinion on the meaning of :hover and :active states.  I
think that:

* An element matches :hover when the mouse pointer is within it *or any of its
descendents*.
* An element matches :active when the mouse pointer is depressed within it *or
any of its descendents*.

Think about the following cases and I think you can see why:
* a link with a floating image in it
* a link with a tall image in it (since the height of the link's inline box is
unchanged)
* a link with an absolutely positioned child
etc.

I don't think this applies to :focus, though.

Do you have a Target Milestone for this?  I think this is reasonably important
for DOM applications that use onclick="", etc.

Comment 8

[Hixie (not reading bugmail)]

Good point - really, both the geometrics and the ancestry need to be taken into
account. That is, the list of elements matching :hover (:active) should be all
the elements over which the cursor is floating (and clicked) _and_ all their
parents. So if two images-in-links overlap (due to positioning or negative
margins, for example), then both the links will light up.

A simple example:

   IMG { position: absolute; top: 0; left: 0; width: 10px; height: 10px; }
   A { color: blue; }
   A:hover { color: red; }

...with this markup:

   <P><A> Link 1 <IMG SRC="a.gif"> </A></P>
   <P><A> Link 2 <IMG SRC="b.gif"> </A></P>

Whenever the mouse is over the images (the square area 10px by 10px at the top
left of the viewport), both links should turn red.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I disagree with what Ian said about overlap.  Assuming that, in cases of
overlap, only the element on top gets the events (which are then bubbled...), I
think only the element on top (i.e., receiving the events) should be the one
that matches :active or :hover (along with its ancestors, of course).

Comment 10

[Hixie (not reading bugmail)]

What does that do to the following?

   DIV.overlay {
      position: fixed; left: 0; right: 0; top: 0; bottom: 0;
      margin: auto; padding: 0; border: none; z-index: 100;
      background-image: url(animatedbutterflies-lazy.gif);
   }
   DIV.overlay:hover {
      background-image: url(animatedbutterflies-busy.gif);
   }
   DIV.overlay:active {
      background-image: url(animatedbutterflies-frantic.gif);
   }
   :link { color: blue; }
   :link:hover { color: red; }
   :link:active { color: lime; }

...on a document such as:

   <TITLE> Butterfly Haven </TITLE>
   <P> Go to the <A href="next.html"> next page </A> . </P>
   <DIV class="overlay"></DIV>

Are you saying that the <A> link is unclickable, and will not give :hover and
:active feedback?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The basic problem here is that you're only allowing one element at a time to
match :active or :hover.  (It's reasonable for focus, though.)

I will attach a test case that shows that img:hover doesn't work when it's
inside a link.  Fixing this would probably allow you to get rid of some of the
element-specific code for these pseudo-classes.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: test case for img:hover inside anchors

Comment 13

[Hixie (not reading bugmail)]

[Adding some radars...]
[Tentatively setting milestone to M12, post feature cut-off]

Comment 14

[Hixie (not reading bugmail)]

*** Bug 13119 has been marked as a duplicate of this bug. ***

Comment 15

[Sam Allen]

Bug 13119 dealt with links within floating elements not receiving focus when a
block-level element was adjacent to the table. Some links on sites such as
http://developer.netscape.com/ and http://www.amazon.com/ cannot receive focus
because of this issue.

Comment 16

[Hixie (not reading bugmail)]

We _have_ to do it both ways: all the elements below the cursor and all their
ancestors. Otherwise, links in floats get their events stolen by the blocks
that come after them in the flow.

What we should probably do with the actual click event is pass it to all the
blocks that we made :hover apply to, until one of them eats it. So if we are
over a paragraph in the main flow and a floating link at the same time, they
would be classify as :hover and then :active, but only one of them would trigger
the onclick event -- if the paragraph has an onclick handler then it would be
the paragraph, and otherwise it would be the link (in that order because that
is the stacking order in that case).

But hey, I'm a CSS guy, not a DOM guy, so what you do with the events after
you've made CSS happy is none of my business... ;-)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No you don't (for that).  The event bugs around floats are separate bugs
(events going to the wrong element).  The events are going to the wrong
elements.

The reason you might have to do it as Ian describes is for pages like
http://www.w3.org/Style/CSS/ , which have negative margins.

Ian - FYI, there are quite a number of bugs on events going to the wrong element
with advanced layout features.

Comment 18

[kipp]

Attachment: another test case, this time on a span element

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://www.w3.org/TR/css3-userint , although it's currently a working draft, is
relevant to this bug.  In particular, see:

http://www.w3.org/TR/css3-userint#pseudo-active
http://www.w3.org/TR/css3-userint#user-input

Comment 20

[rickg]

Moving to m13 because Joki seems to be distracted.

Comment 21

[Hixie (not reading bugmail)]

Migrating from {css2} to css2 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...

Comment 22

[ekrock's old account (dead)]

Moving M17. Not required for beta.

Comment 23

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 33736 for a discussion of the :hover aspects of this bug.  I think the
:hover problems and the :active problems should be completely separated,
especially considering the CSS3-userint draft (which would cover the :active
issues).

Comment 25

[Matthew Mastracci]

*** Bug 35315 has been marked as a duplicate of this bug. ***

Comment 26

[Conor Lennon]

Attachment: Test case for A:hover with images inside anchors

Comment 27

[WD]

I've noticed that the following behaves unexpectedly.

<style>
div:hover{ color: red}
</style>
<div>Divtext</div>

The text it turned red when the line containing the text is moved over, but 
when the text itself is moved over with the mouse, it turns back black.   Is 
this related or should I file a new bug?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That problem is already covered by this bug.  It is described by the first
bullet point in my initial comment when opening the bug, although I should have
said "node" rather than "element", since the problem occurs with text nodes not
in inline elements (since they steal the :hover state).

Comment 29

[joki (gone)]

Adding [ESM/CSS] prefix to bugs in EventStateManager with its generated events 
or CSS pseudo-class management.

Comment 30

[joki (gone)]

Updating Milestone to M18.

Comment 31

[sungod]

*** Bug 46213 has been marked as a duplicate of this bug. ***

Comment 32

[Hixie (not reading bugmail)]

As per meeting with ChrisD today, taking QA.

Comment 33

[James Green]

I'm seeing this on Linux too, build from 2000-07-29. My site uses a :hover to
change some grey text to black for emphasis and the mozilla bug is bl**dy annoying.

Suggest moving OS to All?

Comment 34

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I am the virtual joki.

Comment 35

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Per discusion with Nisheeth, marking nsbeta3+. Will email ekrock to verify.

Comment 36

[Hixie (not reading bugmail)]

Heikki -- Just to summarise all our discussions above and thus save you the
confusion of trying to work out exactly how to fix this bug:

The :hover and :active pseudo-classes should match the node hit and all 
ancestor nodes. It should not match any descendant nodes. The 'node hit' should
be the one which would receive any onclick event if the user happened to click
at that very moment.

If you need testcases, give us a shout.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, I think that's only true of :hover.  :active should probably wait for the
CSS3 rules for handling :active, and stay as-is (??) until then.

Comment 38

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Bug triage with nisheeth & ekrock: nsbeta3-. This is CSS2 issue that we 
have not promised full coverage. Adding relnote3 keyword.

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 40

[Hixie (not reading bugmail)]

Eek! :active is CSS1. :hover is CSS2 but as currently implemented, we would be
preventing adoption of this feature for as long as Netscape 6.0 is in 
significant use.

In addition, this is one of the most often author-requested features. Our
customers want this a *lot*.

Removing [nsbeta3-] to trigger reevaluation. This is one of those cases where
pulling the feature altogether is the alternative to fixing it, but pulling
:hover and :active is likely to get us all shot.

Comment 41

[James Green]

As I understand it, this feature (conformance) could single-handedly remove the
need for all the damned javascript navigation rollovers by simply defining rules
for :hover and :active in CSS. I would be annoyed if this gets left out of
Netscape 6 release and would fully support it being nsbeta3+ed.

As it is implemented right now, the bug sticks out like a saw(sp?) thumb on
pages like mine: Linuxnewbie.com, move your mouse over the news items on the
right and see that only the whitespace to the right of the text activates the
black emphasis. I'd welcome any corrections if I've actually written it wrong,
but from what I've heard, it's moz being broken. Please fix ! :)

Comment 42

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Triaging with Nisheeth: nsbeta3+. Reassigning to joki 'cos he's back from 
sabbatical.

The test cases from David (in the first comment) are great!

Comment 43

[James Green]

Attachment: Illustrates broken use of :hover

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regarding what needs to be done to fix this bug:

Because of the CSS3-userint draft, I'm not sure what should be done with
:active.  I used to think it should work like hover, but now I think it should
probably be limited.  I'm not sure how we would want to do it, though.

:hover is reasonably simple.  It needs to apply to multiple elements at the same
time.  It is explained in bug 33736 and especially bug 45822.

Comment 45

[Hixie (not reading bugmail)]

I believe we should ignore CSS3 *draft* specifications at this stage. They are
very much in flux (the UI draft even more than others) and should in any case
be backwards compatible. 

The only problem that needs fixing here is to make :active and :hover match
the node hit and all ancestor nodes. It should not match any descendant nodes. 
The 'node hit' should be the one which would receive any onclick event if the 
user happened to click at that very moment.

When fixing this, make sure that :hover'ing or :active'ating a text node (e.g.,
the textual contents of a <P> element) still propagates the :hover to all the
ancestor elements.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

also when fixing this, please make sure that the :hover propages from anonymous 
content to it's binding element (and it's ancestors).

That would allow us to for example remove the javascript for xbl demo #2

Comment 47

[joki (gone)]

Okay.  This bug has been around for quite some time so I'm going to close it 
out.  Here is the status:

Basic nested hover should be working now.  All of the attached testcases work 
just fine.  The www.linuxnewbie.com site which is mentioned in here somewhere 
has pretty complex hover behavior and only partially works.  I don't know why 
but I'm opening a new bug about it (bug #52766) since the reasons it doesn't 
work seem to be more complex than the other cases.

:active has not been changed.  For the moment it is working as designed.

So I'm going to mark this fixed and close it.

Comment 48

[James Green]

Works on Linux 2000091506. Adding verifyme keyword.

Comment 49

[Eric Hodel]

All divs from the last attachment
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=13678 keep the outset
border for WinNT 2000091505 across the anchor and code elements, and across the
text of the div.

Comment 50

[Hixie (not reading bugmail)]

*** Bug 45822 has been marked as a duplicate of this bug. ***

Comment 51

[Blake Ross]

reopening this because it caused a significant regression in UI performance.

Comment 52

[Nisheeth Ranjan]

Marking nsbeta3- to get off the beta 3+ radar.  Please nominate this for rtm if
we figure out a low risk fix that does not cause performance degradation while
mousing over chrome.  I believe hyatt is working on coming up with such a fix.
CCing him.

Comment 53

[James Green]

*** Bug 54480 has been marked as a duplicate of this bug. ***

Comment 54

[David Hyatt]

Another problem with the :hover patch (besides the common parent check not 
working) is that it is using the content tree and not the frame tree.  This 
means that interleaved anonymous content will not get put into :hover (and it 
should).  This code should not be doing a common parent check by walking content 
nodes.  It should be walking the frame tree.

Comment 55

[Greg Miller]

If this doesn't make it in for RTM, I hope hover as a whole gets disabled... As
it stands right now, the half-support generates some odd-looking behavior.

Comment 56

[David Hyatt]

:hover only applies to elements, so one trivial fix (that I made for :active a 
week or two ago) that would probably really improve things without going the 
whole nine yards would be to never put a node into :hover... look for its 
element parent and put that into :hover.

Comment 57

[Hixie (not reading bugmail)]

BTW, as far as I can tell this actually *regressed*, as well as being backed
out. It appears that :link:hover no longer works, for instance (see 
http://www.webstandards.org/css/macie/linkdemo2.html ). This is very much
not a good thing.

We really should fix joki's performance problem for RTM. Our current state is
very bad. Especially since this is one web authors' most liked parts of CSS.

Comment 58

[WD]

*** Bug 54672 has been marked as a duplicate of this bug. ***

Comment 59

[Doron Rosenberg (IBM)]

*** Bug 54836 has been marked as a duplicate of this bug. ***

Comment 60

[djoham]

cc self

Comment 61

[Nisheeth Ranjan]

Marking rtm- and futuring.  This does not meet the criteria for a stop ship rtm bug.

Comment 62

[Hixie (not reading bugmail)]

RELEASE NOTE ITEM:
   In this release, Mozilla does not correctly support the ':hover' psuedo-
   class, not even on the <a> element as supported in pre-release versions
   and in other browsers. Mozilla will only consider an element to be in
   the :hover state if the mouse cursor is hovering directly over an element,
   that is, not over a child node (such as text). There is no workaround.

Nisheeth: Considering this bug has no workaround and occurs on many (if not
most) top100 sites, could you reconsider? We 'just' need to fix the performance
issues here, and Hyatt seemed to think that that was quite straight forward.

Joki: Do you have any idea how to fix this?

Comment 63

[Jerry Baker]

Can I toss in an extra please, or will that mean it won't get fixed (per 
Brendan's comments in n.p.m.general) :-)

Comment 64

[David Hyatt]

We should at the very least ensure that the nearest enclosing element gets a 
proper :hover.  This can be done without doing full hierarchical hover.

Comment 65

[Jerry Baker]

If it doesn't get fixed, can I suggest a change to the relnotes?

RELEASE NOTE ITEM:
   In this release, Mozilla does not correctly support the ':hover' psuedo-
   class, not even on the <a> element as supported in pre-release versions
   and in other browsers.

   Mozilla will only consider an element to be in the :hover state if the mouse  
   cursor is hovering directly over an element, that is, not over a child node 
   (such as text). There is no workaround.

Talking about child elements will confuse even some intermediate Web developers 
who will surely come looking why their simple CSS won't work. Since this is most 
used on <a> elements, and these have to have children to be visible - so, in 
effect, ":hover" does not work at all.

Putting the details in a separate paragraphs just serves to make a distinction 
between the effect and the cause. The reader sees ":hover doesn't work" and that 
is taken in as one unit. They then see the child element explanation later - as 
a different block of information. This way they do not needlessly get confused 
when they just wanted to see that it doesn't work.

Comment 66

[Stephan Jaensch]

Since (as per other comments) the fix seems to be straightforward, it should be
definitely done for rtm. Many pages use hover effects to increase useability
while minimizing load time.
And btw, every reviewer will characterize this as a big defect in Mozilla/NS 6,
and right he is. After all, this feature is a) widely used and b) already worked
in Mozilla.

PLEASE, fix this.

Comment 67

[James Lariviere]

I like to point out that :hover is not completely broken in html form controls.

In my test case for bug 41819
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=12896), the :hover for
html form controls: radios, checkboxes, buttons (input, submit, reset, button)
and selects work fine.  However, :hover in the other controls do not work correctly.

I would like to suggest that if you can not fix :hover you need to disable it in
these form controls as well.  BUT, I think that this would be a HUGE LOSS for
mozilla.  As a regular participant in the
comp.infosystems.www.authoring.stylesheets, missing :hover support in netscape
has to be the most consistently asked question.  IMHO, no :hover in mozilla (and
netscape6) seams unbelievable and unacceptable considering all the great and
real pseudo conformance since really the beginning of mozilla development.  This
is a most bitter pill to swallow as a web developer on the eve of releasing this
product after years of development.

I can already hear the web standards project getting a new press release ready
praising mozilla for a great browser with super style support and then ending
the article dumbstruck as to why no :hover, yanked a couple of months before
release. A sad day for web developers everywhere. :-(

PLEASE Fix this.

Comment 68

[PTRourke]

If :hover isn't re-enabled to some degree, the reviewers are going to have a
field day.  E.g, they will ask, as the UI is based upon XUL, why :hover works
fine in the UI (i.e., the Personal Toolbar) but not in the layout of pages.  You
clearly recognize the importance of response to mouseover events in the UI; why
take that feature away from us content authors? I suspect the only reason you
folks thinks this is a minor feature is because you don't look at websites in
browsers other than NS4.x enough.

Comment 69

[timeless]

Please stop spamming!

the following are clear:
No one likes as is.
This needs to be fixed
some of us are tired of the comments which aren't helpful.

Here's a hint: I'm adding keyword: helpwanted.

If you want to help, please contribute, otherwise this is not helping any 
developer.

Yes we'll be reamed if we don't fix this.

What you people may not be aware of is that :hover and :active DO NOT work well 
in xul either. The bug is rather visible in classic in the personal toolbar.

If this is so easy to fix, please click 
http://bugzilla.mozilla.org/createattachment.cgi?id=5693 aka "Create a new 
attachment (proposed patch)", select the patch radio button and attach your 
fix.

Joki/Heiki: I'm resetting the milestone target. If you feel any inclination to 
future it, please please find someone else @netscape.com or a mozilla.org 
contributor who has the knowledge to fix this (I don't, but maybe: 
[e-mail], [e-mail], or [e-mail] do) instead of 
futuring this.

Comment 70

[David Hyatt]

I'll take a look at this.

Comment 71

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 53460 has been marked as a duplicate of this bug. ***

Comment 72

[Peter Trudelle]

rtm+ need info, have a partial fix that should make hover work adequately for
N6, sorely needed for tree performance.  small, low-risk patch, fixes highly
visible performance degradation.

Comment 73

[David Hyatt]

Attachment: Patch that fixes basic hover (without fixing full hierarchical hover)

Comment 74

[Nisheeth Ranjan]

David, I've asked Chris Saari to review the patch as he is the resident expert
while joki is away.

Please close bug 55067 once you check in your patch and future this bug.  Bug
55067 was opened to track the fixing of hover without re-enabling the
hierarchical hover notifications.

Thanks!  I'm re-assigning bug 55067 to you.

Comment 75

[saari (gone)]

This patch looks fine to me (other than the printf, which Hyatt said he has 
removed). I'm assuming that targetContent won't be null at this point.

Comment 76

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

couldn't that previous slow fix be applyed for everything but chrome (or just 
html)? It would be very unfortunate if :hover dosn't work for all descendents 
when desining webpages.

Comment 77

[Hixie (not reading bugmail)]

*** Bug 55670 has been marked as a duplicate of this bug. ***

Comment 78

[David Hyatt]

This has had a=waterson and r=saari.  Ready to go.

Comment 79

[Phil Peterson]

rtm++

Comment 80

[David Hyatt]

Ok, fixed on branch and trunk.  Removing [rtm++] and futuring.

Comment 81

[Niko Pavlicek]

perhaps i'm completely wrong, but does hierarchical :hover also means, without
it you can't have a JavaScript-OnMouseOver-event and a :hover on, for example,
links, check out http://www.los-angeles2019.de/navbar.html; the links have a
OnMouseOver (the status text is changing) and they should have a :hover but they
don't have in Mozilla. If that's true I think it is important to check this bug
COMPLETELY because I won't be the last webdesigner with such a combination!

Comment 82

[David Hyatt]

Your links don't go into :hover because you return true in your onmouseover 
handlers.

I quote from page 201 of the OReilly Javascript reference.

"...[returning true from an onMouseOver handler] tells the browser that it 
should not perform its own default action for the event..."

Since :hover is a default action, returning true from the mouseover handler 
indicates to the browser that it should not put the link into :hover.

Comment 83

[David Hyatt]

Question is, though, is :hover really considered a default action?  It seems 
kind of bogus to me that in order to keep us from replacing the status text you 
have to sacrifice :hover capability.

I wonder if this is a bug.  Ian or dbaron, do you have any thoughts?

Comment 84

[David Hyatt]

IE5 puts the links into :hover even when true is returned from onmouseover.  I 
think we should do the same.

Comment 85

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The issue is arguable - please argue on bug 38380 instead of here.  I tend to 
think :hover should still work regardless of the DOM event handlers.

Comment 86

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

regarding hierarchical hover:

Is there a reason that the previous slower patch can't be used on html elements?
Please reevaluate for rtm!

Comment 87

[timeless]

imo, :hover IS a default action, and i am supposed to be writing a complete 
testcase suite for return false/return true. Can someone offer a reason that 
return false; would hurt current users?

Comment 88

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please discuss that on bug 38380, not here.

Comment 89

[James Lariviere]

opps, I think your new patch broke the pointer cursor (as reported today in bug
51220).  I noticed any link that has :hover no longer displays the correct
pointer (hand) normally displayed for links.

Comment 90

[James Lariviere]

Opps wrong bug.

The bug reported today is bug 56094 not 51220 which I think is caused by the new
patch.

Sorry about the SPAM.

Comment 91

[Peter Trudelle]

Could someone give a concise reason why this has to be fixed for rtm?

Comment 92

[Jerry Baker]

I think that JS image rollovers are one of the most common effects used on the 
Internet today. The ability to do the same thing with CSS would be a boon for us 
lowly Web authors. I think it is one of the features most anticipated by the 
community from a CSS-compliant browser, but I may be mistaken.

Comment 93

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also there exists a patch that fixes this. The problem of the patch is that it 
gave performance problems in XUL. I propose that the patch is modifyed to only 
affect html documents.

Comment 94

[David Hyatt]

You don't want to do that.  As I've said before, there are big problems with the
patch. It ends up thrashing because it does too many notifications.  This has
nothing to do with XUL.  It will do it to HTML too.  Until we resolve the
problems, it should not be turned on anywhere.

I understand your desire to have this feature in Netscape 6, but the patch just
isn't ready yet. It's too late.

Comment 95

[Peter Trudelle]

rtm-

Comment 96

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

to try to fix this myself:
Would it work (performance put aside) if I stepped up the DOM tree from the 
targetContent node and did a

SetContentState(element, NS_EVENT_STATE_HOVER);

on every element along the way?

Whould that cause a reflow of every element just the onces that actually has 
a :hover rule?

Comment 97

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My idea is this: step up the tree and check every element if it is in hover 
state. If it is, we are done. If it isn't, put the element in hover state and 
continue.

Hmm, then there is the problem of moving elements out of hover state...

Comment 98

[David Hyatt]

The code should be stepping up the frame tree, not the content tree.  It misses
anonymous frames right now.

Comment 99

[James Lariviere]

I do not know if this is helpful but I have to point out that the select,
checkbox, radio, and button(s) has had :hover remain enabled.  Even with :hover
supposedly disabled.

Since it seems that these form elements are referencing a different "function"
(sorry i'm not a programmer) for :hover than the code area you've been changing
over the past few weeks, might this other function offer some insights on fixing
:hover with other elements.

To see that these elements are still working with :hover, check out test case #3
in bug 41819.

Again I'm sorry if this is not helpful.  I find it odd that these elements can
still use :hover while everything else does not.

Comment 100

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

hyatt:  Why do you say we should be walking the frame tree rather than the
content tree?  I think the content tree is better for at least the following
reasons:

 1) CSS selectors match elements in the document tree.  They never match
anything else.  So the only things that should match :hover are elements.  (But
perhaps there are issues with stylesheets attached to XBL anonymous content?)

 2) Often the content and frame trees do not correspond.  For example, <a
href="http://www.foo.com/"><img align=right src="...">blah blah blah</a>.  If
the user hovers over the image, the link should be highlighted.  The frame for
the image is in the floater list of the anchor's closest block parent, and is
not a child of the a's frame.

If XBL anonymous content is an issue, I don't think walking the frame tree
instead is the solution, since it would break other things.

(Jonas Sicking: A simple algorithm to reduce number of nodes notified would be
to build a list of the path from the "out of" node back to the root, and the
"into" node back to the root, and then compare them in parallel, starting with
the root, and only start notifications when you hit the first difference.)

Comment 101

[David Hyatt]

Ok, we don't have to walk the frame tree, but we do have to deal with
XBL-inserted anonymous content.  Just reinforces the fact that I need to store
this info in the content model somehow.

Comment 102

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Exactly where is the problem of the current hierarchical hover patch? I can find
only two problems which both are "just" preformance issues:

* Finding the common ancestor of the "oldHover" and "newHover" isn't very
  optimal

* Finding out if a element is in "hovermode" requires walking content tree.

The only way I can think of to fix the second one (and to make the first one
really fast) is to somehow be able to flag a nsIContent that it is currently
beeing hovered. However both of theese problems are something we could live with
on html Documents (right?)

Hyatt: you said mentioned something about some other problems, could you please
explain exactly what is trashed?

Comment 103

[David Hyatt]

I'm not sure of the exact problem.  I used Quantify to determine that when
mousing around among siblings, the common parent check wasn't working right.
Both loops were entered (when neither should have been), and both loops called
extra ContentStateChangeds.

Comment 104

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to speed up hierarchical hover

Comment 105

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I've attatched a patch that fixes unnessesary ContentStatesChaned calls. The
problem was that there was no check to see if the new hover element was a child
to the current element. Also the parent of the new and old hover element was
always notifyed.

I've also speeded up the common-parent search to be O(n) instead of 0(n^2)

(woohooo my first real mozillapatch :-) )

Comment 106

[David Hyatt]

Let me apply this to my build and do test quantify runs to see what happens.

Comment 107

[Antti Näyhä]

*** Bug 56976 has been marked as a duplicate of this bug. ***

Comment 108

[Jerry Baker]

I appliead this patch to my tree and respun. I wasn unable to see any difference 
in performance whatsoever. This obviously requires a machine that is much slower 
than mine is to see. The point being that it is a fine patch as far as not 
degrading anything or breaking any building on Win2k anyway.

Comment 109

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch that fixes hierarchical hover and enables it on html documents

Comment 110

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I've attatched a patch that does the same thing as my previous one but also
enables hierarchical hover on all html documents (independently of the value of
the pref).

I really think that the first patch is better but if XUL still is to slow then
we could possibley use the new one. The problem with only doing it on html
documents is that it gives inconsitencies which is always bad.

I cant really test the performace on any of my patches since hierarchical hovers
have always been fast on my mashine, even without patches.

removing rtm- for reevaluation since we now have a couple of bugfixes (and
:hover regressed a couple of days ago)

Comment 111

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hmm.. thinking about this my last patch should be modified to enable 
hierarchical hovers on everything but XUL documents rather then just HTML 
documents.

If it should be used at all, I still recomend the first patch

Comment 112

[Doron Rosenberg (IBM)]

*** Bug 57145 has been marked as a duplicate of this bug. ***

Comment 113

[Hixie (not reading bugmail)]

trudelle/hyatt: could you decide if this bug should be [rtm-] or [rtm+] ? Cheers.
If it is plussed then we should get the last attached patch reviewed ASAP.

Comment 114

[David Hyatt]

I would classify this as moderate risk, and therefore don't think it can be
placed into Netscape 6. IMO (as much as I like the feature) this bug should be
marked rtm-.

Comment 115

[Peter Trudelle]

making it so, rtm-

Comment 116

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that the second patch is medium risk but the first one is just 
rearranging some while loops. Isn't that fix really enough or is XUL still too 
slow? I can't try it out myself since I've never had any performance problems.

Comon people, shipping NS6 without support for :hover (yeah yeah I know there's 
a special hack for <a>) is really embarrassing. :hover is one of the most 
missed features from NS4 and one that could really give dynamic userinterfaces. 
Also it really shows the power and speed of gecko (something that marketing 
should like ;-) )

Comment 117

[David Hyatt]

I'm not going to waste time going before the PDT to argue this bug.  They will 
not approve it.  There are much less risky bugs that they're already turning 
away.  

It's not a question of whether or not I want it in NS6.  I'd love to see it in 
NS6, but I already know what PDT will say, and so there's really no point in 
+ing it.

Comment 118

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Soory, I thought +ing it ment it was PDT approved. All theese ++ and [-- need 
info] are too confusing for me :-)

Comment 119

[ekrock's old account (dead)]

Hi Jonas -- The situation is as you say: (1) the second patch is medium risk so 
we can't take that patch for N6 RTM. (2) the first patch has been observed to 
cause performance problems for the product as a whole on some platforms/machines 
(although apparently those problems don't occur on your own machine), so we 
can't take that patch for N6 RTM either. Marking ns601. We'll shoot for trying 
to get this into the first major post-RTM point release. Changing Severity 
from blocker to normal; this issue is not a blocker for product engineering or 
QA work.

Comment 120

[Peter Trudelle]

->moz0.9

Comment 121

[Peter Trudelle]

->future, until this is better defined.

Comment 122

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Considering there's a patch on this bug, it really shouldn't be futured.  Could
either hyatt or joki (or someone else?) review the patch from Jonas Sicking in
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=17484 and explain what
needs to be tested to get it checked in or why it shouldn't be?

Comment 123

[Brendan Eich [:brendan]]

dbaron's right.  Clearing Target Milestone and reassigning to trudelle for
better assignment.  We need to get a merged-up version of the patch reviewed and
tested.

/be

Comment 124

[James Green]

> ------- Additional Comments From David Hyatt 2000-10-16 20:31 -------

> Let me apply this to my build and do test quantify runs to see what happens.

What were the results of this? From what I can tell, we need to know if the
patch is "correct", i.e. does it do what is intended while not breaking other
things, and second whether the performance hit it acceptable. I suspect the
latter relies on other performance work which leads me to wonder what if
anything has changed in that respect.

Comment 125

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Although my patch should speed things up a bit I don't think the differance is 
that big.

What really should be done is to add a function like:

nsCSSFrameConstructor::HierarchContentStatesChanged(aPresContext, Content1, 
Content2)

where Content1 should be a grandparent to Content2. A function like that should 
be able to do some really heavy optimizations and would be very useful for 
hierarchical :hover (and hierarchical :active)

I'd offer to implement it after I'm done with some other stuff I'm working on 
right now...

Comment 126

[David Hyatt]

From reading the CSS WG mailing list, it looks like the exact definition of
hierarchical hover is still being hammered out.  I'm not sure we should put an
implementation of it into our code until we are sure that we know how exactly
this feature should work.

Comment 127

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

could somebody with access to that list please ask them to investigate the 
hierarchicalness of :active as well

Comment 128

[Hixie (not reading bugmail)]

It's part of the same discussion. (And :focus, as well.)

Comment 129

[Jesse Ruderman]

Bug 65917 covers :active not being hierarchical.  Is that bug a dup of this 
one?  (If so, the summary of this bug should be changed to include :active, 
:hover, and :focus.)

Comment 130

[Jesse Ruderman]

What would make the most sense to me, thinking of the outer element as either 
<body> or <a href>, and the inner element as <input type=text> or <b>:

- if an inner item is hovered over (and isn't itself focusable?), apply the 
hover rule to the outer element.
- if an outer item is activated by clicking on an inner item, apply the active 
rule to the outer element.  I figure the outer item shouldn't be activated if 
the inner item itself takes focus.
- if an inner item has focus, do *not* apply the focus rule to the outer item.  
(This contradicts Hyatt's comment earlier today in bug 42758.)

Comment 131

[Peter Trudelle]

->moz1.0, in hopes that the css working group will decide on this issue by then.

Comment 132

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The consensus at the F2F seemed to be that we should leave the behavior
unspecified.  See the end of:
http://lists.w3.org/Archives/Member/w3c-css-wg/2001JanMar/0290.html

Comment 133

[James Green]

David, that's a link most of us can't use :(. Can you provide a more detailed
summary so that implementors and testers can have an idea of what the WG intends?

Following on from that, what are we to do on this? I've just encountered another
case where I want the text in <div>foo</div> to have the div:hover rule applied
and yet it gets switched off. Be nice to get that problem sorted.

Hyatt: does Jonas' patch(s) do what is needed for correctness (whatever that may
be), and is the perf hit acceptable? You haven't given those quantify results
yet, and I'm sure the patch probably needs to be merged with a current tree :)

Comment 134

[James Green]

*** Bug 80651 has been marked as a duplicate of this bug. ***

Comment 135

[Anders Sjölander]

This seem to apply for onMouseOver on DHTML-layers, too. See http://zoomon.com/
- when moving the mouse over inner content of layers (like on the menuitems on
the webpage above), onMouseOver is not triggered, and I feel that is SERIOUS. I
have reported this bug, don't know if you feel it's a duplication of this one.

Comment 136

[James Green]

Traction needed here. High number of cc's, high number of votes, and we
apparently block a good number of other bugs; David B has explained that the
specs leave this issue up to implementors thus we are free to enable this as we
see fit. dbaron and I just spoke about this on irc and believe that having
:hover effects not happen on anonymous content is a bit silly.

Nominating for 0.9.2. We need to test the patches here, update them as
neccessary, check for correctness and speed impact.

Can we have hyatt's attention once his css re-write stuff has been landed please :)

Comment 137

[David Hyatt]

The issue is not anonymous content.  The issue is performance.  The only 
anonymous content cases that would fail would be XBL-interleaved content, which 
I could live with.  I've written code in the last year that makes it trivial to 
deal with that anyway.

I have planned optimizations to make to ReResolveStyleContext, which I want to 
do in 0.9.2, and I would not want to check this bug in until I make those 
optimizations.

Once I do that, hierarchical hover should be pretty safe to turn on, even if 
it's bone-headed. :)

Comment 138

[James Green]

*** Bug 83032 has been marked as a duplicate of this bug. ***

Comment 139

[Jacek Piskozub]

13 dups. Should be made mostfreq long ago. Marking.

Comment 140

[James Lariviere]

*** Bug 87797 has been marked as a duplicate of this bug. ***

Comment 141

[Pierre Saslawsky]

*** Bug 91186 has been marked as a duplicate of this bug. ***

Comment 142

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 143

[Gervase Markham [:gerv]]

hyatt: given your comment a few months ago in this bug, any chance of a quick
status update?

Gerv

Comment 144

[James Lariviere]

*** Bug 96984 has been marked as a duplicate of this bug. ***

Comment 145

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 97814 has been marked as a duplicate of this bug. ***

Comment 146

[Jeremy M. Dolan]

Attachment: testcase with <li>

Comment 147

[Jeremy M. Dolan]

I don't see what's so hierarchical about my testcase, seem's pretty simple, as
far as CSS goes... that case not working just killed the whole effect for a page
I was making. Adding Cc and a vote.

Comment 148

[Peter Trudelle]

->99

Comment 149

[Gervase Markham [:gerv]]

hyatt: are there any further issues preventing sicking from fixing up his work
and getting it checked in?

I think 0.9.9 is rather late to land this change anyway.

(I'm also rather confused that this bug apparently "blocks" five resolved bugs.)

Gerv

Comment 150

[David Hyatt]

Gerv, yes, there are outstanding issues.

Comment 151

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

while my patch does some cleanup for the code (giving it some performance 
boost) the main performance problems are still there. To fix the real problem i 
think we need to take advangate of the new stylesystem.

Hyatt?

Comment 152

[Pierre Saslawsky]

*** Bug 90146 has been marked as a duplicate of this bug. ***

Comment 153

[Boris Zbarsky [:bzbarsky]]

*** Bug 115479 has been marked as a duplicate of this bug. ***

Comment 154

[Greg K.]

Any ramifications for bug 93598 and bug 93602?

Comment 155

[Boris Zbarsky [:bzbarsky]]

*** Bug 122190 has been marked as a duplicate of this bug. ***

Comment 156

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 122979 has been marked as a duplicate of this bug. ***

Comment 157

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 124646 has been marked as a duplicate of this bug. ***

Comment 158

[Peter Trudelle]

Adding keyword nsbeta1 to several bugs.

Comment 159

[Sören 'Chucker' Kuklau (gone)]

*** Bug 124210 has been marked as a duplicate of this bug. ***

Comment 160

[Peter Trudelle]

nsbeta1- per nav triage team, cc barrowman for possible embedding issues. ->1.2

Comment 161

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: A patch restoring hierarchical hover with some event state style reresolution performance fixes

This patch restores hierarchical hover and makes style reresolution for event
state changes specific to which event state changed.  I also added a comment
about potentially batching style changes -- that would be nice to do,
particularly since the :hover notifications are given in the reverse of the
order that would be optimal for the style system.  However, the state changes
are *incompatible* with batching as I explain in the comment in
nsCSSStyleSheet.  I'm not sure which is really the better solution.  (I think
there are some interesting ways to do batching -- for example, one could sort
by depth in the content tree (lowest to highest), use that sort to quickly
eliminated duplicates (testing each in the list, from lowest to highest, by
walking its parent chain and the prior elements in the list in parallel).)

Another possibility is to remove HasStateDependentStyle completely, and somehow
separate the matching of state-dependent style rules from other style rules,
and merge the two together.  This seems hard, especially given the current
architecture of ReResolveStyleContext (especially w.r.t. sibling/cousin sharing
and the immutability of the mRuleNode of style contexts).

This patch is not yet well tested.  I just wrote it this afternoon/evening.

What performance issues in particular need to be tested?

Comment 162

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 136013 has been marked as a duplicate of this bug. ***

Comment 163

[Håkan Waara]

I applied the patch. Here are some problems:

* attachment 989 [details] (still?) doesn't work.
* attachment 8284 [details] doesn't work fully. Only part of the img turns blue.
* In attachment 49302 [details], the list item bullets themselves become bigger when you
hover the <LI>s.

I also visited some popular top100 sites and couldn't notice any outstanding
problems.

Comment 164

[Sören 'Chucker' Kuklau (gone)]

#163
> * In attachment 49302 [details], the list item bullets themselves become bigger when
> you hover the <LI>s.

Hmm. Do the CSS specs say that this mustn't happen? Without the patch, when
hovering over the bullets (markers), the text is made bold, so Mozilla seems to
recognize the markers as part of the <li>.

Comment 165

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> * attachment 989 [details] (still?) doesn't work.
> * attachment 8284 [details] doesn't work fully. Only part of the img turns blue.

These are because I haven't yet removed the hack that made :hover work for a
elements despite not being hierarchical.  I need to figure out what does that
and remove/modify it.

> * In attachment 49302 [details], the list item bullets themselves become bigger when you
> hover the <LI>s.

This has nothing to do with the current patch, but, rather, has to do with our
treatment of bullets in general.  According to the CSS2 markers concept it's not
a bug.  I'm not sure what the CSS3 list module says.

Comment 166

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that also fixes anchor hack

The only difference between this patch and the previous is the changes added to
nsGenericHTMLElement.cpp.

Comment 167

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, those added changes are wrong, perhaps.  At the very least, they
regress bug 38380, which was fixed in a rather odd way (very specific to the
anchors case, rather than the general fix).

Comment 168

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that also fixes bug 38380 in all cases (not just anchors)

Comment 169

[Hixie (not reading bugmail)]

Per CSS3 Lists, bullets should grow on :hover, if the text grows, because the
bullets are glyphs and are in pseudo-elements that are children of the list item
elements. (This is the same as CSS2.)

Comment 170

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking.

Comment 171

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hyatt said that the main performance problem this caused in the past was with
XUL menus.  I can pretty easily peg my CPU moving the mouse pointer up and down
over my bookmarks menu either with or without the patch -- the patch doesn't
seem to make the pegging any worse, although I might try to measure (somehow)
the speed at which I can move the mouse without causing skipping of the :hover
(which seems to kick in pretty suddenly for some reason).  However, it feels
like the general performance of the two builds is pretty much the same. 
(Caveat:  I'm using a dual P-733 on Linux, although the second processor doesn't
help here since it's all on one thread.)

Comment 172

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: a few minor tweaks to the previous patch

Just a bunch of comment fixes, a few changes to parameter names, and a fix for
one case where I might have caused one unnecessary notification for some hover
changes (I changed the if and do...while to just while for the hover
notification loop, since we should check against the commonHoverAncestor before
entering the loop.)

Comment 173

[Sören 'Chucker' Kuklau (gone)]

Any chance of getting this into 1.0?

Comment 174

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To summarize, this does the following:
 * Changes the state parameter to ContentStatesChanged to be a PRInt32
   mask rather than a pseudo-class to reflect how the ESM works (since
   there are sometimes state changes of multiple states at once).
 * Makes the HasStateDependentStyle code in the style system use that
   parameter so HasStateDependentStyle can be false more often.  (Also
   removes use of NS_COMFALSE.) 
 * Removes the hierarchical hover pref and turns hierarchical hover on 
   permanently.
 * Fixes the O(N^2) nature of the common ancestor finding and cleans up
   other parts of the hierarchical hover notification code a bit 
   (removing some excess notifications too).
 * Removes the hover hack for anchors (i.e., what made :hover work for
   anchors even though :hover wasn't hierarchical) and fixes bug 38380
   in the general case instead of just for that hack.

Comment 175

[Håkan Waara]

Is there any particular type of regressions I should look for or pages I should
look at when trying this patch?

Comment 176

[Aaron Kaluszka]

David: Does the patch help bug 65917?

Comment 177

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Re: comment 175) Mostly things related to dynamic style changes for event
states (active, hover, focus, checked), and particularly hover.

(Re: comment 176) No.

Comment 178

[Brian Ryner (not reading)]

Comment on attachment 78185 [details] [diff] [review]
a few minor tweaks to the previous patch

r=bryner

Comment 179

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I just filed bug 136301 on some menu performance issues I found.

Comment 180

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 136484 has been marked as a duplicate of this bug. ***

Comment 181

[joki (gone)]

Looks good to me, too.  r=joki

Comment 182

[David Hyatt]

Comment on attachment 78185 [details] [diff] [review]
a few minor tweaks to the previous patch

sr=hyatt.  Test carefully. :)

Comment 183

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2002-04-10 20:48/49 PDT.  I'll ask for branch approval
after RC1 ships (hopefully early next week).

Comment 184

[Hixie (not reading bugmail)]

VERIFIED FIXED on the trunk using the W3C Selectors test suite.

dbaron: dude this is sweet. Your next trip to La Fiesta is on me. :-)

Comment 185

[Sören 'Chucker' Kuklau (gone)]

Confirmed on 2002-04-11-03, Win32 Trunk. This rocks!

Comment 186

[Sören 'Chucker' Kuklau (gone)]

http://www.w3.org/Style/CSS/Test/CSS3/Selectors/20020115/html/tests/css3-modsel-63.html

On that page, the first paragraph does not turn green when hovered, but the
second does.

Comment 187

[Manko]

Replying to #186
This page uses :not pseudoclass. It is'nt issue of CSS2, but CSS3.

Comment 188

[Sören 'Chucker' Kuklau (gone)]

#187
Should have noticed. Sorry.

Comment 189

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://www.w3.org/Style/CSS/Test/CSS3/Selectors/20020115/html/tests/css3-modsel-63.html
should work, since we support :not.  That's probably bug 96984, though.

Comment 190

[Warner Young]

dbaron, on the site http://www.aberdeeninc.com, when I move the mouse over the
various links, sometimes I get links that don't underline, and other times, the
underlines don't go away after I move the mouse away.  I don't know if this is
related or not.  Trunk build, 2002041103, Win98.

Comment 191

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The underline repainting issue is filed as bug 136935, although I may mark that
as a duplicate of an existing bug.

Comment 192

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I believe the underlining problems on :hover are actually a regression from
Windows font metrics changes that landed yesterday (bug 76097), not this bug.

Comment 193

[Matthew Mastracci]

Thanks for landing this awesome fix!  I think this should hit a lot of
dependencies as well.

I've just dropped this note on bug #41819.  Any ideas why the filepicker doesn't
react to focus?

----

With the changes from 5693, all of the testcases work 100% as expected, with the
exception of the file picker - it doesn't get the :focus style as expected (but
:hover works).

If the file testcase could be fixed, this bug could be closed.  

---

Comment 194

[Brian Ryner (not reading)]

I need more details here:

- do you mean the Linux filepicker dialog, or <input type=file>?
- in either case, what is it that you are trying to focus (and by what means),
and how is it not showing focus?

Comment 195

[Boris Zbarsky [:bzbarsky]]

Matthew means that when you click in the textfield of an <input type="file"> the
:focus style applied to the <input type="file"> is not applied to that textfield
(or to anything else for that matter).

Comment 196

[R.K.Aa.]

can this have caused bug 137124, bug 137069, bug 137067 and bug 137247?

Comment 197

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this fix exposed an existing problem in the nsIStyleRuleSupplier interface
and the way nsStyleSet used it, because the breaking-out-of-the-loop that
HasStateDependentStyled dependended on wasn't supported properly.  I'm surprised
nobody noticed this before, and I'm pretty sure my changes didn't make the
problem any worse beyond optimizing HasStateDependentStyle to check only the
state that changed, which probably tweaked it in a bunch of skins where some
states were triggered only by style rules in XBL bindings.

The fix for that problem is attached to bug 137067.  That fix should also go in
with this one if this goes on the mozilla 1.0 branch.

Comment 198

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Never mind about it being an existing bug.  I assumed that break-out
optimization always worked (I think I might have even written it myself too,
knowing that it didn't).  There's a simpler fix.  See bug 137067 for the details.

Comment 199

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 119604 has been marked as a duplicate of this bug. ***

Comment 200

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The fix for bug 137556 should also go in to the branch if this goes on the branch.

I also need to investigate whether bug 130999 and bug 138282 were caused by this
patch.

Comment 201

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

NOTE:  The fixes for bug 135776 and bug 137067 should go into the branch when
this does.  I have marked those bugs as fixed since they exist on neither trunk
nor branch.

Comment 202

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, that should have said:

NOTE:  The fixes for bug 137556 and bug 137067 should go into the branch when
this does.  I have marked those bugs as fixed since they exist on neither trunk
nor branch.

Comment 203

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 78185 [details] [diff] [review]
a few minor tweaks to the previous patch

a=[e-mail] for branch checkin.	Please include checkin of the two
regression fixes mentioned

Comment 204

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_0_0_BRANCH, 2002-04-24 17:23/24 PDT.  (See comment
183 above for trunk checkin.)

Comment 205

[Frank Tang]

This is one of the suspect of causing 140983. 

Comment 206

[Hixie (not reading bugmail)]

VERIFIED FIXED on trunk

Comment 207

[Rakesh Mishra]

Verified on the branch build 2002-05-22-08-1.0.0 on Windows 2000 , with the test
cases under the URL :
1)http://www.fas.harvard.edu/~dbaron/csstest/sec051103b 
2)http://www.fas.harvard.edu/~dbaron/tests/nglayout/sec051103b_js 

adding verified1.0.0 to the keyword

Comment 208

[Jerry Baker]

Mass removing self from CC list.

Comment 209

[Jerry Baker]

Now I feel sumb because I have to add back. Sorry for the spam.