Closed
Bug 1340723
Opened 7 years ago
Closed 7 years ago
stylo: dynamic changes to CSS offsets on relatively positioned (rel-pos) inline with a block child don't work right
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(5 files)
188 bytes,
text/html
|
Details | |
159 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
bholley
:
superreview+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
bholley
:
superreview+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
bholley
:
superreview+
|
Details |
Because we don't restyle the anon block anonymous box.
Assignee | ||
Comment 1•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8842889 [details] Bug 1340723 part 1. Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls. https://reviewboard.mozilla.org/r/116622/#review118378
Attachment #8842889 -
Flags: review?(emilio+bugs) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8842890 [details] Bug 1340723 part 2. Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager. https://reviewboard.mozilla.org/r/116624/#review118380
Attachment #8842890 -
Flags: review?(emilio+bugs) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8842891 [details] Bug 1340723 part 3. Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes. https://reviewboard.mozilla.org/r/116626/#review118306 Awesome! Thanks for fixing this :) r=me w/ nits ::: layout/generic/nsInlineFrame.h:121 (Diff revision 1) > : (!GetNextInFlow()); > } > > + // Update the style on the block wrappers around our non-inline-outside kids. > + // This will only be called when such wrappers in fact exist. > + virtual void UpdateStyleOfOwnedAnonBoxes(mozilla::ServoStyleSet* aStyleSet, nit: no need for `virtual`. ::: layout/generic/nsInlineFrame.cpp:1036 (Diff revision 1) > + MOZ_ASSERT(GetStateBits() & NS_FRAME_OWNS_ANON_BOXES, > + "Why did we get called?"); > + MOZ_ASSERT(GetStateBits() & NS_FRAME_PART_OF_IBSPLIT, > + "Why did we have the NS_FRAME_OWNS_ANON_BOXES bit set?"); > + // Note: this assert _looks_ expensive, but it's cheap in all the cases when > + // it passes! Also, I guess IB splits aren't ultra-common like, let's say, text frames (though probably not specially rare). ::: layout/reftests/ib-split/relpos-inline-1a.html:12 (Diff revision 1) > + <link rel="help" href="http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level"/> > + <meta name="flags" content="" /> > + <style> > + span { position: relative; left: 200px } > + html { > + overflow: hidden; I can't believe we had no tests for this tbh :/. ::: layout/reftests/ib-split/relpos-inline-1b.html:26 (Diff revision 1) > + <script> > + onload = function() { > + var s = document.querySelector("span"); > + s.offsetWidth; // flush layout > + s.style.left = "200px"; > + document.documentElement.className = ""; I thought reftests waited after the onload event, is `reftest-wait` really necessary?
Attachment #8842891 -
Flags: review?(emilio+bugs) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8842889 [details] Bug 1340723 part 1. Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls. https://reviewboard.mozilla.org/r/116622/#review118386 ::: layout/generic/nsFrame.cpp:10257 (Diff revision 1) > return IsFrameScrolledOutOfView(this); > } > > +/* virtual */ > +void > +nsIFrame::UpdateStyleOfOwnedAnonBoxes(ServoStyleSet* aStyleSet, Also, one last suggestion. I'd love to see this being passed by reference. I didn't do it initially in `RecreateStyleContexts` because heycam said that the local style in `RestyleManager` was passing it by pointer. I kind of disagree with that (and I'd love to see more references used), but of course I don't think I have a word there. You do. Anyway, if you want to change it here, that's fine by me, otherwise by pointer is also ok.
Assignee | ||
Comment 9•7 years ago
|
||
Just to make it clear it won't be null? I can absolutely do that.
Comment 10•7 years ago
|
||
That'd be great, thanks! :)
Updated•7 years ago
|
Attachment #8842889 -
Flags: superreview+
Comment 11•7 years ago
|
||
Comment on attachment 8842890 [details] Bug 1340723 part 2. Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager. sr=me with the bit check hoisted into an inline function.
Attachment #8842890 -
Flags: superreview+
Comment 12•7 years ago
|
||
Comment on attachment 8842891 [details] Bug 1340723 part 3. Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes. sr=me, though I'm no expert on {ib} splits.
Attachment #8842891 -
Flags: superreview+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/130e62d89663 part 1. Add an nsIFrame function that can be called from the stylo restyle manager to update style contexts on anonymous boxes associated with that frame, and a frame state bit that can be used to optimize out the virtual calls. r=emilio https://hg.mozilla.org/integration/autoland/rev/f6370635b090 part 2. Call the new UpdateStyleOnOwnedAnonBoxes function as needed from ServoRestyleManager. r=emilio https://hg.mozilla.org/integration/autoland/rev/8335b277da3f part 3. Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes. r=emilio
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8842891 [details] Bug 1340723 part 3. Fix stylo to properly update styles on the anonymous blocks we create in a block-inside-inline situation when the style of the inline changes. https://reviewboard.mozilla.org/r/116626/#review118306 > nit: no need for `virtual`. Following file style. > Also, I guess IB splits aren't ultra-common like, let's say, text frames (though probably not specially rare). They're actually pretty common. Not like textframes, of course, but more than a lot of other layout things. :( > I can't believe we had no tests for this tbh :/. Yeah, I was pretty sad to see that. We had some tests that test it incidentally, but they fail in stylo for other reasons. :( > I thought reftests waited after the onload event, is `reftest-wait` really necessary? I basically don't trust any of that stuff, and especially don't trust someone not changing it in the future, so trying to be explicit...
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/130e62d89663 https://hg.mozilla.org/mozilla-central/rev/f6370635b090 https://hg.mozilla.org/mozilla-central/rev/8335b277da3f
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.
Description
•