822141 - changes to <li value> do not cause the bullet to be repainted immediately

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: test

In the attached test, you should see the <li>'s number change from 1 to 5 after one second, and then for it to become blue after two seconds.  Instead, you see the number and the colour change simultaneously at two seconds.

In nsBlockFrame::AttributeChanged, we call FrameNeedsReflow(..., nsIPresShell::eStyleChange, NS_FRAME_HAS_DIRTY_CHILDREN), which is asserting.  Should this be eTreeChange maybe?  When I change it to that, we still don't manage to repaint the bullet.  Seems like an invalidation problem.

Comment 1

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

eStyleChange is the most destructive value, so changing it doesn't seem likely to help; see http://hg.mozilla.org/mozilla-central/file/c8a1314aa449/layout/base/nsIPresShell.h#l469

Comment 2

[Matt Woodrow (:mattwoodrow)]

Indeed, we never actually invalidate anything when we change the 'value' attribute on the <li>, not entirely sure where we should fix this though.

We get multiple frames constructed for the <li> element, a block frame container, with a bullet frame child.

nsCSSFrameConstructor::AttributeChanged calls AttributeChanged on the block frame, which schedules a reflow, and we never call anything on the bullet frame (but we do reflow it).

Does anyone have suggestions on how this should work ideally? We really want the bullet frame to get an invalidate call, though I could make DLBI know to check the parent for bullet display items. We do a similar thing for text where the underlines are stored on a different frame to the one painting them.

It seems like we should be calling AttributeChanged on all frames, not just the primary one, and have nsBulletFrame know to invalidate itself if 'value' changes.

Alternatively we could make GetAttributeChangeHint return nsChangeHint_RepaintFrame, but that would mark all children as invalid, so we'd unnecessarily repaint the text as well.

I guess we could just always invalidate bullet frames when we get reflowed (text does this), but that's not ideal.

Comment 3

[Matt Woodrow (:mattwoodrow)]

CC'ing roc for input on Comment 2.

Comment 4

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

Two possibilities of the top of my head:

  * make nsDisplayBullet remember enough information to know whether it's changed due to non-style reasons (so we compare it correctly in DLBI code).  Or more drastically, change nsDisplayBullet so nsBulletFrame::PaintBullet doesn't need any information from the frame (and could be moved to a method on nsDisplayBullet).

 * make this code in nsBlockFrame::RenumberListsFor do explicit invalidation (inside the "if (changed)"):

        *aOrdinal = bullet->SetListItemOrdinal(*aOrdinal, &changed, aIncrement);
        if (changed) {
          kidRenumberedABullet = true;

          // The ordinal changed - mark the bullet frame dirty.
          listItem->ChildIsDirty(bullet);
        }

Comment 5

[Matt Woodrow (:mattwoodrow)]

I guess the important question is: is this likely to be a problem specific to changing the value of bullets, or is there a large set of attribute changes that result in different rendering?

If it's the former, then getting DLBI to check the mOrdinal value for nsBulletFrames will be easy.

The latter would suggest looking at a more generic solution.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think either option in comment #4 would work well. I'd lean slightly towards the first option --- you're not likely to see a very large number of bullets rendered at once.

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> I guess the important question is: is this likely to be a problem specific
> to changing the value of bullets, or is there a large set of attribute
> changes that result in different rendering?

I lean towards the former. If there was a large set of attributes that result in different rendering (and aren't handled by mapped attributes/style change hints), wouldn't we know about it by now?

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Check the ordinal value

Comment 8

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e594f7b7a12

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9e594f7b7a12

Comment 10

[Cameron McCormack (:heycam)]

Just wrote a test, but it causes an assertion; filed 824264 for that.