Add reftests, distilled from simple testcases, that demonstrate the correct behaviour. MozReview-Commit-ID: 6ecT1lNbHev

This is to inhibit the nsChangeHint_RecomputePosition optimization for fixed position elements that depend upon their placeholder's position. MozReview-Commit-ID: ANfPlAE7iNN

I don't think this code is read to land and I'm looking for feedback. Eg, we should handle removing NS_FRAME_HAS_FIXED_POS_DEPENDS_ON_STATIC_POS frame state when it's no longer needed, but I'm not sure what's the best way to do that.

It would be difficult to clear that state bit in an efficient way when it's no longer needed. So like some other similar bits (such as NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE) we can just probably just leave it set.

Comment on attachment 8970414 [details] [diff] [review] P1: Add reftests. It might make sense to put these in a new layout/reftests/w3c-css/submitted/css21/abs-pos/ directory, so that they get submitted to web-platform-tests. (When doing that, make sure to include the new directory's manifest in its parent directory's manifest.) You should also avoid the "No newline at end of file" in the manifest. Do both of the tests fail without the patch, or only the second one? It's not clear to me how the first test is particularly related. I think it's probably worth trying to add a few more tests of cases that fail without the patch (cases mentioned in bug 1200585).

Comment on attachment 8970415 [details] [diff] [review] P2: Mark all frames depending upon container pos. nsAbsoluteContainingBlock::FrameDependsOnContainerPos should be static (but don't bother, I tell you to undo that refactoring below). The code in RestyleManager::ProcessRestyledFrames should be changed in a few ways: 1. it shouldn't set nsChangeHint_AllReflowHints; instead it should set NeedReflow | ReflowChangesSizeOrPosition. You actually don't need ClearAncestorIntrinsics here; I think it's worth weakening the "Must be not be set without also setting nsChangeHint_NeedReflow and nsChangeHint_ClearAncestorIntrinsics" comment above the ReflowChangesSizeOrPosition flag's definition. (The exclusion of NeedDirtyReflow is a *major* performance difference here.) 2. You should probably change the code from being bit-twiddling into living inside the RecomputePosition function, which falls back to calling StyleChangeReflow in some cases. It should just look like another similar case. 3. All of the existing StyleChangeReflow calls in RecomputePosition are incorrect in that they should include the nsChangeHint_ReflowChangesSizeOrPosition flag. This probably doesn't show up today since abs-pos elements are never reflow roots, but it may show up in the near future when we move to implementing CSS containment. We should just fix this. 4. In light of that, it might be better to move the StyleChangeReflow() call from RecomputePosition to its caller. That will change the number of callsites from three (or four, with your addition) to one. The code in CreatePlaceholderFrameFor should be conditional on the frame being fixed-positioned. Otherwise you're hitting *way* more cases than you should be. I'm also not sure CreatePlaceholderFrameFor is the best place for this. I'd be inclined to put it in the code where we do hypothetical box computations, i.e., in ReflowInput::CalculateHypotheticalPosition. This means you no longer need to refactor FrameDependsOnContainerPos, and you're also using a much more exact test, and in a less-frequently-hit codepath. You'll still need to test for being fixed-position, though.

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #5) > Comment on attachment 8970414 [details] [diff] [review] > P1: Add reftests. > > It might make sense to put these in a new > layout/reftests/w3c-css/submitted/css21/abs-pos/ directory, so that they get > submitted to web-platform-tests. (When doing that, make sure to include the > new directory's manifest in its parent directory's manifest.) > > You should also avoid the "No newline at end of file" in the manifest. I will do that. Thanks. > Do both of the tests fail without the patch, or only the second one? It's > not clear to me how the first test is particularly related. Both tests fail before the changes made. pos-depends-on-placeholder-1.html is derived from "simple testcase" written by you on Bug 1200585. pos-depends-on-placeholder-2.html is derived from "Another testcase that demonstrates this" written by :mattwoodrow on Bug 1200585. > I think it's probably worth trying to add a few more tests of cases that > fail without the patch (cases mentioned in bug 1200585). I'll try to decipher the other cases that you documented in Bug 1200585 and add them as ref tests.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1da619585dac Inhibit RecomputePosition when descendants depend on the out of flow position. r=dbaron

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18147 for changes under testing/web-platform/tests

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Upstream PR merged