Closed Bug 15608 Opened 25 years ago Closed 21 years ago

selectors with adjacent sibling combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] (:hover +)

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.7alpha

People

(Reporter: alla-old, Assigned: dbaron)

References

()

Details

(Keywords: css2, testcase, Whiteboard: [nsbeta3-] [patch])

Attachments

(6 files, 7 obsolete files)

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.
Summary: adjacent selector doesn't work with a :hover rule. → {css2} adjacent selector doesn't work with a :hover rule.
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.
Status: NEW → ASSIGNED
Target Milestone: M15
Reassigning peterl's bugs to myself.
Accepting peterl's bugs that have a Target Milestone
Pushing my M15 bugs to M16
Keywords: css2
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...
Summary: {css2} adjacent selector doesn't work with a :hover rule. → adjacent selector doesn't work with a :hover rule.
Target Milestone: M16 → M17
Summary: adjacent selector doesn't work with a :hover rule. → adjacent combinator doesn't work with a :hover rule.
*** Bug 39976 has been marked as a duplicate of this bug. ***
As per meeting with ChrisD today, taking QA.
QA Contact: chrisd → py8ieh=bugzilla
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.
OS: Linux → All
Priority: P3 → P5
Hardware: PC → All
Summary: adjacent combinator doesn't work with a :hover rule. → adjacent combinator doesn't work with a :hover rule [CASCADE]
Marking nsbeta3-. There are other more important nsbeta3+ bugs to work on.
Marking nsbeta3- this time.
Whiteboard: [nsbeta3-]
Block moved M17 bugs to mozilla0.9
Target Milestone: M17 → mozilla0.9
Target Milestone: mozilla0.9 → mozilla0.9.1
Moving to m0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Moving to m1.0
Target Milestone: mozilla0.9.2 → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.1
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling work post Mozilla1.0.
Priority: P5 → P1
Target Milestone: mozilla1.1 → Future
*** Bug 128040 has been marked as a duplicate of this bug. ***
*** Bug 132695 has been marked as a duplicate of this bug. ***
Summary: adjacent combinator doesn't work with a :hover rule [CASCADE] → adjacent combinator doesn't work with a :hover rule [SELECTORS-DYNAMIC]
Attached file testcase without :hover (deleted) —
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.
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.
Summary: adjacent combinator doesn't work with a :hover rule [SELECTORS-DYNAMIC] → selectors with adjacent combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC]
Never mind the comments about there being another bug. However, the more general subject here still stands.
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).
cc'ing myself
Summary: selectors with adjacent combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] → selectors with adjacent combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] (:hover +)
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
*** Bug 110972 has been marked as a duplicate of this bug. ***
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.
Summary: selectors with adjacent combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] (:hover +) → selectors with adjacent sibling combinator missed by dynamic style reresolution [SELECTORS-DYNAMIC] (:hover +)
*** Bug 189309 has been marked as a duplicate of this bug. ***
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.)
Depends on: 163556
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.
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.
Please keep one issue per bug (i.e, file separate bugs on any crashes).
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.
*** Bug 213304 has been marked as a duplicate of this bug. ***
*** Bug 213464 has been marked as a duplicate of this bug. ***
*** Bug 223672 has been marked as a duplicate of this bug. ***
A testcase using Mozilla 1.4 showing the *:hover pseudo class and adjacent child selector intermittently working, depending on mouse movement.
Attached patch some work-in-progress (obsolete) (deleted) — Splinter Review
Attached patch some work-in-progress (obsolete) (deleted) — Splinter Review
Attachment #138240 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #138241 - Attachment is obsolete: true
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).
Attachment #138295 - Flags: superreview?(bz-vacation)
Attachment #138295 - Flags: review?(bz-vacation)
Whiteboard: [nsbeta3-] → [nsbeta3-] [patch]
Target Milestone: Future → mozilla1.7alpha
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?
(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 on attachment 138295 [details] [diff] [review] patch Never mind. This patch causes a bunch of weird things...
Attachment #138295 - Attachment is obsolete: true
Attachment #138295 - Flags: superreview?(bz-vacation)
Attachment #138295 - Flags: review?(bz-vacation)
The problems are fixed by backing out the changes to nsCSSFrameConstructor::AttributeChanged .
Attached patch patch (obsolete) (deleted) — Splinter Review
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.
Attachment #138305 - Flags: superreview?(bz-vacation)
Attachment #138305 - Flags: review?(bz-vacation)
I could probably remove the enumeration halting code from WalkRuleProcessors. And I should probably wait until bryner lands bug 64116 before landing.
Attached patch patch (obsolete) (deleted) — Splinter Review
merged with bryner's changes for bug 64116
Attachment #138305 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
with the changes to nsStyleSet::WalkRuleProcessors as well...
Attachment #138575 - Attachment is obsolete: true
Attachment #138305 - Flags: superreview?(bz-vacation)
Attachment #138305 - Flags: review?(bz-vacation)
Attachment #138576 - Flags: superreview?(bz-vacation)
Attachment #138576 - Flags: review?(bz-vacation)
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.
> 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.)
> 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.
Attached patch patch (obsolete) (deleted) — Splinter Review
Addresses the comments not addressed in comment 51 (with one as described in comment 52).
Attachment #138576 - Attachment is obsolete: true
Attachment #138576 - Flags: superreview?(bz-vacation)
Attachment #138576 - Flags: review?(bz-vacation)
Attachment #138982 - Flags: superreview?(bz-vacation)
Attachment #138982 - Flags: review?(bz-vacation)
> 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 on attachment 138982 [details] [diff] [review] patch With that one change (to return if the whole thing is reconstructed in AttributeChanged), r+sr=bzbarsky
Attachment #138982 - Flags: superreview?(bz-vacation)
Attachment #138982 - Flags: superreview+
Attachment #138982 - Flags: review?(bz-vacation)
Attachment #138982 - Flags: review+
Attached patch patch (deleted) — Splinter Review
oops
Attachment #138982 - Attachment is obsolete: true
Fix checked in to trunk, 2004-01-13 17:36 -0800. See also additional bugs mentioned in comment 43.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
v 20030114 PC/WinXP
Status: RESOLVED → VERIFIED
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;}
Please file a new bug on that testcase, with something resembling steps to reproduce (and cc me and dbaron).
*** Bug 232694 has been marked as a duplicate of this bug. ***
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: