Last Comment Bug 15608 - selectors with adjacent sibling combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] (:hover +)
: selectors with adjacent sibling combinator missed by dynamic style reresoluti...
Status: VERIFIED FIXED
[nsbeta3-] [patch]
: css2, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal with 5 votes (vote)
: mozilla1.7alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Hixie (not reading bugmail)
Mentors:
http://www.lysator.liu.se/~alla/moz/c...
: 39976 110972 128040 132695 189309 213304 213464 223672 232694 (view as bug list)
Depends on: 163556
Blocks:
  Show dependency treegraph
 
Reported: 1999-10-05 13:30 PDT by Alexander Larsson
Modified: 2014-04-27 11:50 PDT (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase without :hover (696 bytes, text/html)
2002-04-14 09:00 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Test-case with hover and active, on first element and last (1.35 KB, text/html)
2003-05-16 11:35 PDT, Craig Saila
no flags Details
Testcase showing fickle ":hover + elem" workings (3.30 KB, text/html)
2003-07-18 03:12 PDT, Jan Moesen
no flags Details
testcase showing *:hover + span {} sometimes working (1.05 KB, text/html)
2003-12-14 09:56 PST, Paul Mortier
no flags Details
some work-in-progress (35.99 KB, patch)
2003-12-31 16:58 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
some work-in-progress (50.45 KB, patch)
2003-12-31 17:41 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (52.29 KB, patch)
2004-01-02 10:29 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (53.59 KB, patch)
2004-01-02 14:45 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (50.59 KB, patch)
2004-01-07 16:43 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (53.06 KB, patch)
2004-01-07 16:50 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
patch (53.20 KB, patch)
2004-01-13 16:00 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
patch (53.22 KB, patch)
2004-01-13 16:09 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
Testcase with input display none and label form attribute and adjacent dynamic stylerule. (1.26 KB, text/html)
2004-01-15 05:03 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details

Description Alexander Larsson 1999-10-05 13:30:10 PDT
Given the rules:
   div { color: green; }
   a:hover + div { color: red;}

A <div> after an <a> doesn't turn red when the <a> is hovered.

But this rule works:
   a:hover div { color: red;}
A <div> inside an <a> turns red when the <a> is hovered.

Check out the URL for a complete example.
Comment 1 Hixie (not reading bugmail) 1999-10-05 16:07:59 PDT
Here is a slightly more valid example:
   http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/pseudoselector.html
The problem also shows up with pseudo-elements. For example, the following two
rules have no effect:

   div:hover:after { content: "Hovering..."; }
   div:hover + div { background: navy; }

This is presumably something to do with :hover not triggering the right sort of
style context notification(?).

alla: good catch, BTW.
Comment 2 Pierre Saslawsky 1999-10-21 01:07:59 PDT
Reassigning peterl's bugs to myself.
Comment 3 Pierre Saslawsky 1999-10-21 01:12:59 PDT
Accepting peterl's bugs that have a Target Milestone
Comment 4 Pierre Saslawsky 1999-12-20 21:10:59 PST
Pushing my M15 bugs to M16
Comment 5 Hixie (not reading bugmail) 2000-01-13 16:07:59 PST
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 6 Hixie (not reading bugmail) 2000-05-21 02:55:53 PDT
*** Bug 39976 has been marked as a duplicate of this bug. ***
Comment 7 Hixie (not reading bugmail) 2000-07-25 23:40:03 PDT
As per meeting with ChrisD today, taking QA.
Comment 8 Hixie (not reading bugmail) 2000-08-31 23:40:17 PDT
Pierre: I'd forgotten all about this bug -- it looks to me like we are simply
not generating the right reflow hints, but I have no idea really. Simple fix?
If not then lets move this off and deal with it next time -- but it would be
nice to get this fixed, since :hover is so popular with web authors.
Comment 9 karnaze (gone) 2000-09-11 15:22:48 PDT
Marking nsbeta3-. There are other more important nsbeta3+ bugs to work on.
Comment 10 karnaze (gone) 2000-09-21 11:10:30 PDT
Marking nsbeta3- this time.
Comment 11 Pierre Saslawsky 2000-10-16 09:02:49 PDT
Block moved M17 bugs to mozilla0.9
Comment 12 karnaze (gone) 2001-05-01 12:25:21 PDT
Moving to m0.9.2
Comment 13 karnaze (gone) 2001-05-14 10:53:19 PDT
Moving to m1.0
Comment 14 Kevin McCluskey (gone) 2002-02-22 09:51:52 PST
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-02-27 07:01:29 PST
*** Bug 128040 has been marked as a duplicate of this bug. ***
Comment 16 Hixie (not reading bugmail) 2002-03-22 12:33:32 PST
*** Bug 132695 has been marked as a duplicate of this bug. ***
Comment 17 Jonas Sicking (:sicking) 2002-04-14 04:45:38 PDT
related to bug 110972
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-14 09:00:32 PDT
Created attachment 79158 [details]
testcase without :hover

There are really two different bugs here:  one that deals with all dynamic
style changes related to adjacent combinators, and one that deals with event
state changes and any selector where the event state is not on the target.  I'm
going to make this bug deal with the former since that's what I've always
assumed it was.  However, both fixes are necessary to fix the combination of
:hover and +.

This testcase, therefore, is a demonstration of problems with dynamic
reresolution and + that does not involve :hover.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-14 09:05:44 PDT
In particular, the bug I've always assumed this bug represented was that when we
get a dynamic change on an element, we call ReResolveStyleContext on that
element, but never its siblings (needed for +) or its parent (needed for :empty).

I filed the event state bug as bug 137395.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-14 09:17:30 PDT
Never mind the comments about there being another bug.  However, the more
general subject here still stands.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-04-18 12:21:23 PDT
http://www.hixie.ch/tests/adhoc/css/background/dynamic/001.html is also a
testcase for this bug.
Comment 22 Hixie (not reading bugmail) 2002-04-19 01:21:00 PDT
I'm sure
   http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/pseudoselector.html
used to work better than it did (currently nothing happens at all).
Comment 23 Madhur Bhatia 2002-05-17 14:26:40 PDT
cc'ing myself
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-06-19 21:20:02 PDT
Assigning pierre's remaining Style System-related bugs to myself.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-07-14 15:58:40 PDT
*** Bug 110972 has been marked as a duplicate of this bug. ***
Comment 26 Boris Zbarsky [:bz] 2003-01-07 10:29:30 PST
I've been thinking a bit about this... The real problem is that we make this
assumption that when a node changes somehow (DOM manipulation, entering/leaving
:hover/:active/etc, and so forth) we need to re-resolve style only on it and its
descendants.  This is a decent assumption for CSS2, but most of the CSS3
selectors violate this assumption.... (as do counters and quotes).  We should
consider designing a new architecture to keep track of dynamic changes, imo.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-01-16 06:58:23 PST
*** Bug 189309 has been marked as a duplicate of this bug. ***
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-01-27 08:18:14 PST
One thought on how to handle this would be:
 1. Fix bug 163556
 2. Change Has[State|Attribute]DependentStyle to store and return whether there
are direct/indirect sibling combinators involved in any style changes.
 3. When adding/removing content, just reresolve style on the parent (to handle
empty, +, and all the new CSS3 variants).  (Perhaps there are good ways to
optimize this, but it might just not be that bad.)
Comment 29 Craig Saila 2003-05-16 11:35:06 PDT
Created attachment 123523 [details]
Test-case with hover and active, on first element and last

Valid XHTML Strict and CSS example showing the problem with both a :hover and
:active pseudo class. Using the adjacent sibling selector, where the first
sibling has a pseudo class, the CSS is not applied. If the last sibling is the
one with a pseudo-class, it works as expected.
Comment 30 David "liorean" Andersson 2003-05-16 12:14:12 PDT
Hmm, Hixie (using: Mozilla 1.4b on Win2k, 2003050714) your example crashes the
browser with a "mozilla.exe has generated an error and will be closed by
Windows" message.

Nothing that explains the crash in any way is shown.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-05-16 12:20:46 PDT
Please keep one issue per bug (i.e, file separate bugs on any crashes).
Comment 32 Anne (:annevk) 2003-07-01 10:28:23 PDT
I have created two more test-cases.

http://www.annevankesteren.nl/mozilla/bug/15608/index.xml
Type = text/xml

http://www.annevankesteren.nl/mozilla/bug/15608/index.html
Type = text/html

The xml test checks :hover and :not(:hover)
The html test check :hover, :not(:hover), :target and :focus

I know some of them are CSS3 selectors, but they could be used for futurre
reference.
Comment 33 Jan Moesen 2003-07-18 03:12:09 PDT
Created attachment 128003 [details]
Testcase showing fickle ":hover + elem" workings
Comment 34 Mats Palmgren (:mats) 2003-07-21 10:15:46 PDT
*** Bug 213304 has been marked as a duplicate of this bug. ***
Comment 35 Bill Mason 2003-07-22 16:40:35 PDT
*** Bug 213464 has been marked as a duplicate of this bug. ***
Comment 36 Bill Mason 2003-10-25 10:27:04 PDT
*** Bug 223672 has been marked as a duplicate of this bug. ***
Comment 37 Paul Mortier 2003-12-14 09:56:30 PST
Created attachment 137410 [details]
testcase showing *:hover + span {} sometimes working

A testcase using Mozilla 1.4 showing the *:hover pseudo class and adjacent
child selector intermittently working, depending on mouse movement.
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-12-31 16:58:32 PST
Created attachment 138240 [details] [diff] [review]
some work-in-progress
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-12-31 17:41:20 PST
Created attachment 138241 [details] [diff] [review]
some work-in-progress
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 10:29:34 PST
Created attachment 138295 [details] [diff] [review]
patch
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 10:56:38 PST
Comment on attachment 138295 [details] [diff] [review]
patch

This works for all the testcases in the bug, and I tested using a printf in
RestyleLaterSiblings that it wasn't being called when I didn't expect it to be
called (hovering and clicking UI, loading some bugzilla pages).
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 11:00:28 PST
Comment on attachment 138295 [details] [diff] [review]
patch

I already noticed the two occurrences of bad indentation of the |mHint| member
variable in nsStyleSet.cpp.

Also, is putting the new enum in nsChangeHint.h too evil?  Should I make a new
header file just for it?
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 11:16:18 PST
(Note that this patch does not fix bug 73586, bug 98997, or bug 229915.  All
three of those bug would be fixed by step 3 of comment 28, but I'm not sure if
that's the best approach.)
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 14:17:32 PST
Comment on attachment 138295 [details] [diff] [review]
patch

Never mind.  This patch causes a bunch of weird things...
Comment 45 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 14:38:46 PST
The problems are fixed by backing out the changes to
nsCSSFrameConstructor::AttributeChanged .
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-02 14:45:51 PST
Created attachment 138305 [details] [diff] [review]
patch

The mistake in the previous patch was putting
    // put primary frame on list to deal with, re-resolve may update or add
next in flows
    changeList.AppendChange(primaryFrame, aContent, hint);
inside an |if (rshint & eReStyle_Self)| test.
Comment 47 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-06 11:28:31 PST
I could probably remove the enumeration halting code from WalkRuleProcessors. 
And I should probably wait until bryner lands bug 64116 before landing.
Comment 48 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-07 16:43:55 PST
Created attachment 138575 [details] [diff] [review]
patch

merged with bryner's changes for bug 64116
Comment 49 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-07 16:50:41 PST
Created attachment 138576 [details] [diff] [review]
patch

with the changes to nsStyleSet::WalkRuleProcessors as well...
Comment 50 Boris Zbarsky [:bz] 2004-01-12 18:06:47 PST
Comment on attachment 138576 [details] [diff] [review]
patch

>Index: content/html/style/src/nsCSSStyleSheet.cpp
> CSSRuleProcessor::HasAttributeDependentStyle(AttributeRuleProcessorData* aData,
>   // XXX What about XLinks?

If we kept track of the attr namespace, not just its name, we could do a
restyleSelf on any change of an attr in the xlink namespace.... ;)

>Index: content/shared/public/nsChangeHint.h
>+enum nsReStyleHint {
>+  eReStyle_Self = 0x1,
>+  eReStyle_LaterSiblings = 0x2
>+};

Is it really better to use nsRestyleHint(0) all over instead of having a name
for it here?  I know your feelings on the matter, and I'm ok either way, I
guess.	It's just that looking at code like nsRestyleHint(0) always leaves me
asking "what does 0 mean"?

At the very least, comment here that 0 means no restyling necessary.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>+void
>+nsCSSFrameConstructor::RestyleLaterSiblings(nsIPresContext *aPresContext,

Perhaps this should return at least whether it reconstructed the whole doc?  If
that happens, caller knows it needs to do nothing else.

>+    if (primaryFrame) {
...
>+      // no frames, reconstruct for content
>+      MaybeRecreateFramesForContent(aPresContext, child);

This chunk of code appears in a whole bunch of places, nearly identically. 
With the patch in bug 214013 it'll become completely identical.  Maybe it
should have its own separate function?	Again, one that returns whether it
reconstructed the doc, I guess.

> nsCSSFrameConstructor::ContentStatesChanged(nsIPresContext* aPresContext, 
>+  DoContentStateChanged(aPresContext, aContent1, aStateMask);
>+  return DoContentStateChanged(aPresContext, aContent2, aStateMask);

Do we want to somehow keep the optimization where if the two are ancestor and
descendant we only reresolve style on the ancestor?  That's a valid
optimization to make here (it doesn't matter whether the kids sibling's would
have to be reresolved; reresolving an ancestor will reresolve them anyway). 
Same for the case when aContent1 == aContent2.	Or do these cases get hit so
rarely it's not worth it?

>+nsCSSFrameConstructor::DoContentStateChanged(nsIPresContext* aPresContext, 
>+      if (primaryFrame) {
>+        PRUint8 app = primaryFrame->GetStyleDisplay()->mAppearance;

Um... This is coming after you processed the changelist.  If the hint for this
frame was ReconstructDoc/Frame, you really don't want to be touching it here, I
would think

As a matter of fact, I would probably put this block up in the "else" clause
above, maybe even before I ProcessRestyledFrames.

I like the clarity cleanup here, btw.

> nsCSSFrameConstructor::AttributeChanged(nsIPresContext* aPresContext,

>+    if (hint & nsChangeHint_ReconstructDoc) {
>+      result = ReconstructDocElementHierarchy(aPresContext);
>+      changeList.Clear();

Make that a |return ReconstructDocElementHierarchy(aPresContext);|

> nsCSSFrameConstructor::RecreateFramesForContent(nsIPresContext* aPresContext,

>+  // Rebuilding the frame tree can have bad effects, especially if it's the
>+  // frame tree for chrome (see bug 157322).
>   nsCOMPtr<nsIDocument> doc = aContent->GetDocument();
>   NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);

Hey, as long as you're here, how about:

  NS_ENSURE_TRUE(aContent->GetDocument(), NS_ERROR_FAILURE);

?  "doc" is unused below.
Comment 51 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-13 15:55:11 PST
> Perhaps this should return at least whether it reconstructed the whole doc?  If
> that happens, caller knows it needs to do nothing else.

None of the callers do anything else after calling it, so it doesn't seem worth it.

> Maybe it should have its own separate function?

Once bug 214013 lands.

> Do we want to somehow keep the optimization where if the two are ancestor and
> descendant we only reresolve style on the ancestor?

I don't think it's worth it.  It's a very rare case, and the code is complicated
enough.  (The optimization currently in the tree is wrong, since it's done
before the HasStateDependentStyle calls.)
Comment 52 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-13 15:58:19 PST
> As a matter of fact, I would probably put this block up in the "else" clause
> above, maybe even before I ProcessRestyledFrames.

I put it before the HasStateDependentStyle call instead.  This should be done
even if HasStateDependentStyle doesn't say there's state-dependent style.
Comment 53 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-13 16:00:40 PST
Created attachment 138982 [details] [diff] [review]
patch

Addresses the comments not addressed in comment 51 (with one as described in
comment 52).
Comment 54 Boris Zbarsky [:bz] 2004-01-13 16:06:10 PST
> None of the callers do anything else after calling it, so it doesn't seem worth
> it.

The exception is the second DoContentStatesChanged call (which could maybe be
skipped).  Probably not worth it.

>>+    if (hint & nsChangeHint_ReconstructDoc) {
>>+      result = ReconstructDocElementHierarchy(aPresContext);
>>+      changeList.Clear();
>Make that a |return ReconstructDocElementHierarchy(aPresContext);|

You didn't do this part.
Comment 55 Boris Zbarsky [:bz] 2004-01-13 16:09:27 PST
Comment on attachment 138982 [details] [diff] [review]
patch

With that one change (to return if the whole thing is reconstructed in
AttributeChanged), r+sr=bzbarsky
Comment 56 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-13 16:09:52 PST
Created attachment 138984 [details] [diff] [review]
patch

oops
Comment 57 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-13 17:37:35 PST
Fix checked in to trunk, 2004-01-13 17:36 -0800.

See also additional bugs mentioned in comment 43.
Comment 58 Bill Mason 2004-01-14 23:13:38 PST
v 20030114 PC/WinXP
Comment 59 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2004-01-15 05:03:49 PST
Created attachment 139114 [details]
Testcase with input display none and label form attribute and adjacent dynamic stylerule.

The patch works great afaik.
However, in this testcase I does not work well.
I use input {display:none;} and input:checked+label{background-color:green;}
Comment 60 Boris Zbarsky [:bz] 2004-01-15 13:26:07 PST
Please file a new bug on that testcase, with something resembling steps to
reproduce (and cc me and dbaron).
Comment 61 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-16 11:59:14 PST
comment 59 is now bug 231081.
Comment 62 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-01-30 12:55:53 PST
*** Bug 232694 has been marked as a duplicate of this bug. ***
Comment 63 roth.michael 2004-01-30 14:04:04 PST
(In reply to comment #62)
> *** Bug 232694 has been marked as a duplicate of this bug. ***

Sorry for the duplicate! My misstake.

I tried the nigthly build, dated 2004-01-30 and it works perfectly!

Fantastic work! I'm very happy! Thank you folks so very much! Thank you. Keep on
the great work. 

Note You need to log in before you can comment on or make changes to this bug.