1758020 - Audit nsFlexContainerFrame::MoveFlexItemToFinalPosition if relatively positioning is needed for sticky flex items

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

In this check in MoveFlexItemToFinalPosition(), we perform relative positioning only for position:relative flex items. Should we also check for position:sticky items? (Emilio discovered the same issue in bug 1585254 comment 1.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #0)

In this check in MoveFlexItemToFinalPosition(), we perform relative positioning only for position:relative flex items. Should we also check for position:sticky items? (Emilio discovered the same issue in bug 1585254 comment 1.)

Yeah, looks like we should - thanks!

I think MoveFlexItemToFinalPosition() only gets exercised if we judge that the flex item does not need a final reflow (e.g. if it got a measuring reflow and the size happens to match its final post-flexing size); so this might only matter under certain specific circumstances like that.

Comment 2

[Daniel Holbert [:dholbert]]

Oh wait, I didn't take a close enough look... I was assuming that this check guarded an ApplyRelativePositioning( call, but it doesn't.

I don't recall precisely what that check was for (or what breaks if we remove it). Maybe we should try deleting it and seeing what breaks, and then (assuming something breaks), try to create position:sticky versions of those testcases, which would serve as testcases for this bug here & would potentially be fixed if we broadened the check? (if that is indeed the right thing to do)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://phabricator.services.mozilla.com/D140274 tweak the check a bit, but I've added a note pointing to this bug.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1758020 - Revise MoveFlexItemToFinalPosition(). r?dholbert

This patch shouldn't change the behavior.

• It's OK to get the item's offset only when its relatively positioned, because
  ApplyRelativePositioning doesn't use the offset parameter if the frame is
  sticky positioned. Document ApplyRelativePositioning's parameters to clarify
  this. Also, we don't need to call apply relative positioning to a
  non-relatively-and-non-sticky-positioned item.

• Print FLEX_LOG after applying relative-positioning because aFramePos will
  be adjusted in ApplyRelativePositioning, and we really want to see the
  item's "final position".

• aReflowInput parameter is removed because it's only used to get flex
  container's WritingMode. We can get it from FlexItem.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/833264be11f2
Revise MoveFlexItemToFinalPosition(). r=dholbert

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/833264be11f2