898794 - Store normal frame position before applying relative positioning

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Corey Ford [:coyotebush]]

nsHTMLReflowState::ApplyRelativePositioning, before changing the position given, should store it in a frame property. There are already a number of calculations that need this "normal" position and get it by using |GetPosition() - GetRelativeOffset()|. Storing it explicitly is cleaner and will work when sticky positioning changes the frame's position. This stored position will also be used in sticky positioning calculations.

Comment 1

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

Is this instead of the offset or in addition to the offset?

Comment 2

[Corey Ford [:coyotebush]]

It looks like ComputedOffsetProperty will then be unnecessary, yes. I only notice one use that doesn't directly follow the pattern I've noted, but of course that can be reworked using GetPosition and GetNormalPosition.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3952

Comment 3

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

ok, sounds good.

I'm not sure if GetNormalPosition() is the best name, though I don't have a better one off the top of my head.

Comment 4

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

Maybe GetStaticPosition(), though I'm not crazy about that, either?

Comment 5

[Corey Ford [:coyotebush]]

ReflowPosition (or FlowPosition) might also work, though I still favor NormalPosition.

Comment 6

[Corey Ford [:coyotebush]]

Attachment: Store normal frame position before applying relative positioning.

That worked out well enough. Most of the changes here are of the form |GetPosition() - GetRelativeOffset()| => |GetNormalPosition()|, but a few are slightly more involved, and ApplyRelativePositioning had to be changed to take the whole frame as a parameter.

A try push with this and the patches from bug 898797:
https://tbpl.mozilla.org/?tree=Try&rev=49ea49b2d4f2

Comment 7

[Corey Ford [:coyotebush]]

Attachment: Use GetNormalPosition in nsFlexContainerFrame::Reflow

Oh, plus this similar bit. (Still not sure I could safely store computed sticky offsets in mComputedOffsets, but those I might want to only store in a frame property anyway.)

Comment 8

[Corey Ford [:coyotebush]]

(I'll fold the patches together before checking in.)

Comment 9

[Corey Ford [:coyotebush]]

Attachment: Store normal frame position before applying relative positioning.

Folded patches and rebased onto m-c (with the nsTextFrameThebes move).

Comment 10

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

Comment on attachment 785820 [details] [diff] [review]
Store normal frame position before applying relative positioning.

nsFlexContainerFrame.cpp:

Looks ok to me, but maybe run it by dholbert?

nsFloatManager.cpp:

Probably best to initialize as:
  nsRect region(aFloat->GetNormalPosition(), aFloat->GetSize());


nsFrame.cpp:

Given the code that was there before, I suspect it might be faster
for GetNormalPosition to check IsRelativelyPositioned() before getting 
the frame property, although maybe frame property access is faster now.
Maybe at least leave a comment suggesting that it might be faster?


r=dbaron with that

(though I'm still wondering about the name, but ok with it)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 785820 [details] [diff] [review]
Store normal frame position before applying relative positioning.

>+++ b/layout/generic/nsFlexContainerFrame.cpp
>@@ -2408,20 +2408,20 @@ nsFlexContainerFrame::Reflow(nsPresConte
>-      // (We subtract mComputedOffsets.top because we don't want relative
>-      // positioning on the child to affect the baseline that we read from it).
>-      flexContainerAscent = physicalPosn.y + childDesiredSize.ascent -
>-        childReflowState.mComputedOffsets.top;
>+      // (We don't want relative positioning on the child to affect the
>+      // baseline that we read from it).
>+      flexContainerAscent = curItem.Frame()->GetNormalPosition().y +
>+        childDesiredSize.ascent;

Just one nit -- could you tweak the comment to say:
 // We use GetNormalPosition() instead of GetRect() because we don't want
instead of just "We don't want"? (or something to that effect)

Otherwise it's not really clear what part of the code the comment is talking about, unless the reader knows what GetNormalPosition() is. And most readers won't recognize that at first, since GetNormalPosition is all-new :)

Comment 12

[Corey Ford [:coyotebush]]

Attachment: Store normal frame position before applying relative positioning.

Done.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> nsFrame.cpp:
> 
> Given the code that was there before, I suspect it might be faster
> for GetNormalPosition to check IsRelativelyPositioned() before getting 
> the frame property, although maybe frame property access is faster now.
> Maybe at least leave a comment suggesting that it might be faster?

Given that most of the current callers of GetRelativeOffset didn't bother passing aDisplay, I'd guess the performance impact would be pretty minimal. But I added a comment, and see also bug 404215 comment 42.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0293081301

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e0293081301