1133392 - handling of nsChangeHint_UpdateSubtreeOverflow should handle continuations correctly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Bug 727125 added a nsChangeHint_UpdateSubtreeOverflow hint.  But the handling (the call to AddSubtreeToOverflowTracker) doesn't handle continuations, which should be done.

I'm adding a FIXME to indicate this in bug 992077 patch 1.

Once this is fixed the handling of the hint below that code can probably be removed as well, since I think it trumps all the other overflow-related hints except for UpdateParentOverflow, although we should check that more carefully.  (At the very least, specific handling for this hint below could be removed.)

Comment 1

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

Attachment: testcase

After the second dynamic change (changing visibility), the underline on the recently-unhidden text (which should be above it) doesn't show up.

Comment 2

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

Attachment: Make handling of nsChangeHint_UpdateSubtreeOverflow iterate continuations, as all change hint handling needs to

The test fails without the patch (missing underline on "an underline")
and passes with the patch (where the underline is present, and aligned
with the non-displaced text).

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8564858 [details] [diff] [review]
Make handling of nsChangeHint_UpdateSubtreeOverflow iterate continuations, as all change hint handling needs to

>           if (hint & (nsChangeHint_UpdateOverflow |
>-                      nsChangeHint_UpdateSubtreeOverflow |
>                       nsChangeHint_UpdatePostTransformOverflow)) {

AddSubtreeToOverflowTracker calls AddFrame(aFrame,CHILDREN_CHANGED),
so we've already done that for all continuations.  I think we should
avoid doing it again here for performance reasons by adding
"&& !(hint & nsChangeHint_UpdateSubtreeOverflow)" to the above.
(and remove the other nsChangeHint_UpdateSubtreeOverflow a few lines down)

r=mats with that.

Both HTML files have "</span><p>" - did you mean "</span></p>" ?

Comment 4

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

(In reply to Mats Palmgren (:mats) from comment #3)
> AddSubtreeToOverflowTracker calls AddFrame(aFrame,CHILDREN_CHANGED),
> so we've already done that for all continuations.  I think we should
> avoid doing it again here for performance reasons by adding
> "&& !(hint & nsChangeHint_UpdateSubtreeOverflow)" to the above.
> (and remove the other nsChangeHint_UpdateSubtreeOverflow a few lines down)

The performance benefit doesn't seem likely to cancel out the cost (never mind the complexity) of the extra test, since I expect both hints to happen together quite rarely.  The subtree hint is also rather expensive, so one extra call seems like a drop in the bucket in that case.

> Both HTML files have "</span><p>" - did you mean "</span></p>" ?

Yes.

Comment 5

[Mats Palmgren (inactive)]

How about "hint &= ~(nsChangeHint_UpdateOverflow |
                     nsChangeHint_UpdatePostTransformOverflow)"
in the UpdateSubtreeOverflow block then?  We don't need those bits
beyond that point, and technically we are addressing them there
as a side-effect of addressing UpdateSubtreeOverflow.

Comment 6

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

OK, that works.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee029fea0fbe

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee455c9586d

Comment 9

[Mats Palmgren (inactive)]

FYI, you can remove this nsChangeHint_UpdateSubtreeOverflow too:
https://hg.mozilla.org/try/rev/8db171dd3a62#l1.48

Comment 10

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

Indeed; fixed that locally.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/35db3e41a206

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35db3e41a206