Jesse Ruderman Reporter
	Description • 14 years ago

###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsNodeOfType(nsINode::eELEMENT)', file /Users/jruderman/mozilla-central/layout/base/nsStyleChangeList.cpp, line 96

The node in question appears to be a text node.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 14 years ago

Hmm.  So the issue is that the style-if-visited colors are different for the before and after style contexts for the text child of the link:

(gdb) p/x otherVis->GetStyleColor()->mColor
$37 = 0xff8b1a55
(gdb) p/x thisVis->GetStyleColor()->mColor
$38 = 0xffee0000

The former is the default visited color.  The latter is the default link color.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 14 years ago

|this| and |thisVis| have the same mCachedInheritedData->mColorData.

They also have the same mRuleNode, of course: the root.

They also have the same mParent, which looks wrong to me.  The mParent does have a style-if-visited style context, which seems like it would be the right thing to parent them to.

Maybe our style tree verification code should assert that any given style-if-visited either has the if-visited style of the parent as the parent or has a different rulenode than the main style context?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 14 years ago

OK, so this looks like a bug in the interaction of nsStyleSet::ReparentStyleContext and nsStyleSet::GetContext.  When we're originally reparenting the kids of the ::first-line, we end up in ReparentStyleContext for the text, of course.  At that point, our new parent context is the new style context for the <a>.  This of course has a style if visited.  So do we, since our old parent had one too.

Now we land in GetContext.  aIsLink is true (as passed in from ReparentStyleContext), so we set parentIfVisited to aParentContext, which is wrong.  It should be aParentContext->GetStyleIfVisited() in this case.

It's not clear to me yet whether ReparentStyleContext should be passing PR_FALSE or whether it should pass something depending on the old context or whether GetContext needs some changes or what.

David, do you want to take a look?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 14 years ago

Oh, and I think this blocks bug 494117, since I want to use ReparentStyleContext there.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 14 years ago

Not doing this triggers asserts on some tests once I move to using ReparentStyleContext more.

	David Baron :dbaron:
	Comment 11 • 14 years ago

Comment on attachment 445738 [details] [diff] [review]
Even better style tree check

I think you can actually make this even stronger:

if (childStyleIfVisited &&
    !((childStyleIfVisited->GetRuleNode() != aContext->GetRuleNode() &&
       childStyleIfVisited->GetParent() == aContext->GetParent()) ||
      childStyleIfVisited->GetParent() == aContext->GetParent()->GetStyleIfVisited())) {
  NS_ERROR(...);
  etc.
}

though it's worth adding a comment that the "ruleNode != ruleNode &&" depends on the pref style sheet having different rules for :link and :visited.

r=dbaron with that if you agree

	David Baron :dbaron:
	Comment 12 • 14 years ago

Comment on attachment 447703 [details] [diff] [review]
And handle the visited rulenode better

r=dbaron, although maybe it's worth putting the parent equality test into a method on nsStyleContext?  It would also have to null-check mStyleIfVisited, but I think that's ok.  (Then the third sentence of the big comment could go there too.)

	David Baron :dbaron:
	Comment 13 • 14 years ago

(The method could be nsStyleContext::IsLinkContext(), perhaps?)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 14 years ago

> I think you can actually make this even stronger:

I made it even stronger than that:

  // Since we have different rules for :link and :visited in our ua/user sheets,
  // we know that either childStyleIfVisited has a different rulenode than
  // aContext (in which case it has :visited rules applied and its parent must
  // be aContext->GetParent()), or it has the same rulenode and then its parent
  // must be aContext->GetParent()->GetStyleIfVisited().
  if (childStyleIfVisited &&
      !((childStyleIfVisited->GetRuleNode() != aContext->GetRuleNode() &&
         childStyleIfVisited->GetParent() == aContext->GetParent()) ||
        (childStyleIfVisited->GetRuleNode() == aContext->GetRuleNode() &&
         childStyleIfVisited->GetParent() ==
         aContext->GetParent()->GetStyleIfVisited()))) {

etc.  Though I'm a little torn about moving that outermost ! in all the way using deMorgan; might be more readable... hard to say.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 14 years ago

> (The method could be nsStyleContext::IsLinkContext(), perhaps?)

Good idea.  Done.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 14 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/4a0fcf9b58ff for the assertions, but the strengthening in comment 14 is wrong.  See bug 570866.

Pushed http://hg.mozilla.org/mozilla-central/rev/e88b028a0483 for the main patch.