1198607 - Returning nsChangeHint_UpdateOverflow from nsStyleText::CalcDifference() causes "###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly, except if it's a display:contents child"

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Adjust nsStyleText::CalcDifference() to return
      nsChangeHint_UpdateOverflow | nsChangeHint_SchedulePaint
    instead of NS_STYLE_HINT_REFLOW  when mTextShadow differs.
  (I'll attach a patch to do this.)
  (Be sure to also update nsStyleText::MaxDifference(), too.)

 2. Compile a debug build with this change.

 3. Visit attachment 593939 [details] (from bug 723669)

ACTUAL RESULTS: The testcase renders correctly, but it triggers this assertion on load:
{
 ###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly, except if it's a display:contents child: '!aContent || aContent->IsElement() || (aFrame && aContent->GetParent() && aFrame->PresContext()->FrameManager()-> GetDisplayContentsStyleFor(aContent->GetParent()))', file layout/base/nsStyleChangeList.cpp, line 69
}

The frame being restyled is for a text node (i.e. a non-null content node which is not an element, hence the assertion). We're trying to restyle it to update its overflow region, I believe.

This assertion dates back to bug 367650 from bz.  (It's been modified a bit, but the spirit of the assertion remains the same.) I'm hoping that perhaps bz can clue me in on whether the assertion is actually telling me I'm doing something wrong (entirely likely), or if the assertion just needs changing. 

[CC'ing mats too, since he touched the assertion most recently for 'display:contents' & perhaps can comment]

The frame in question is for a text node (which is not an element), and we're trying to restyle it to update its overflow regions, I believe. I don't know if this is actually problematic (i.e. if there's some more code that needs changing for this to work properly), or if the assertion just needs adjusting to allow [certain?] restyles of text-nodes...

Comment 1

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

It's not valid for a CalcDifference hint to return a non-inherited hint for an inherited property.

You should use UpdateSubtreeOverflow instead of UpdateOverflow.

Comment 2

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

ni? dholbert to get a bug filed on fixing text-shadow hints if he doesn't want to reuse bug 1194480.

Comment 3

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

It also might make sense to make the assertion text point this out more directly.

Comment 4

[smithhayley509]

[Tracking Requested - why for this release]:

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⌚UTC+9 [busy, returning November 2] from comment #2)
> ni? dholbert to get a bug filed on fixing text-shadow hints if he doesn't
> want to reuse bug 1194480.

I did end reusing bug 1194480 for this (fixing both text-shadow & box-shadow there) -- its patch made us update overflow regions (with UpdateSubtreeOverflow as dbaron suggested) instead of reflowing for "text-shadow" changes, as shown e.g. here:
 https://hg.mozilla.org/mozilla-central/rev/009f9286101b#l1.164

So I think this is addressed. Please tag me again if I'm misunderstanding, though.