Closed Bug 1342738 Opened 7 years ago Closed 7 years ago

stylo: need to set mOriginalDisplay correctly

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Unassigned)

References

Details

Unless I'm missing something, stylo always sets mDisplay and mOriginalDisplay to the same value.  In particular, it's only set to something other than a copy from another style struct in set_display over in gecko.mako.rs.  And this is only called from the blockification code in properties.mako.rs.

So there are, afaict, two problems:

1)  When blockifying, we set mOriginalDisplay to the same thing as the new blockified display, which ix expressely what should NOT happen.

2)  When not blockifying, we don't set mOriginalDisplay at all, afaict, which means it has some random value.  This seems bad.

It looks like properties.mako.rs makes calls to set__servo_display_for_hypothetical_box, in the product == "servo" case, which is conceptually similar to Gecko's mOriginalDisplay bit.  So maybe we can just make that call in the Gecko case too, and set mOriginalDisplay from it as needed.  Plus of course set it in general in all the cases when this code is not reached.
That said, set__servo_display_for_hypothetical_box is called with the computed, not specified, display for the case of "blockifying kid of flex or inline-flex or grid or inline-grid".  This doesn't match Gecko's behavior at first glance...  Not sure whether that's observable.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> 2)  When not blockifying, we don't set mOriginalDisplay at all, afaict,
> which means it has some random value.  This seems bad.

This bit is not completely right, we do set it from set_display or copy_display_from, which is what happens when inheriting, etc. Though the copy_display_from bit is incorrect as far as I know (we should set both mDisplay and mOriginalDisplay to the parent's mDisplay if I'm not wrong).

> It looks like properties.mako.rs makes calls to
> set__servo_display_for_hypothetical_box, in the product == "servo" case,
> which is conceptually similar to Gecko's mOriginalDisplay bit.  So maybe we
> can just make that call in the Gecko case too, and set mOriginalDisplay from
> it as needed.  Plus of course set it in general in all the cases when this
> code is not reached.

Right, I think the reason why Cameron wrote it this way is because in servo mutating the struct unconditionally destroys the Copy-on-writing. But, we should remember to not touching it while adjusting the display value :).

Also, we need to set mOriginalFloat too.

I'll work on a fix, should be straight-forward.
After looking at mOriginalFloat, I think it may not be needed, since it's not used from layout, only from rule node computations, and seems that Servo may do this fine transparently.
> we do set it from set_display

Ah, the key for anyone following along is that set_display is called as set_${property.ident} from servo/components/style/properties/helpers.mako.rs, which doesn't obviously show up in set_display searches.
https://hg.mozilla.org/mozilla-central/rev/3c8eff6b0d79
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.