Closed Bug 1133392 Opened 5 years ago Closed 5 years ago

handling of nsChangeHint_UpdateSubtreeOverflow should handle continuations correctly

Categories

(Core :: Layout, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files)

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.)
Attached file testcase
After the second dynamic change (changing visibility), the underline on the recently-unhidden text (which should be above it) doesn't show up.
Assignee: nobody → dbaron
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).
Attachment #8564858 - Flags: review?(mats)
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>" ?
Attachment #8564858 - Flags: review?(mats) → review+
(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.
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
FYI, you can remove this nsChangeHint_UpdateSubtreeOverflow too:
https://hg.mozilla.org/try/rev/8db171dd3a62#l1.48
Indeed; fixed that locally.
https://hg.mozilla.org/mozilla-central/rev/35db3e41a206
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1134966
You need to log in before you can comment on or make changes to this bug.