User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release) Build ID: 20130902030220 Steps to reproduce: Go to page: http://geb.ebay.in/g/ImportHubSearch?catId=9355&qwd=iphone%205%20new%20unlocked&aspectName=iPhone%205&format=Model&minPriceValue=&maxPriceValue=&sortCriteria=BestMatch&totalPages=3&selAspStr=Model%3AiPhone+5!~&_df=force&_trkparms=clkid%3D999343397229617430 Actual results: The page displays properly for a second and then the css gets messed up. The images shift to the right Expected results: The page should be displayed properly.

Unable to reproduce Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0

Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/9bac3ba885eb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130809142638 Bad: http://hg.mozilla.org/mozilla-central/rev/cead6eb63964 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130809160941 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9bac3ba885eb&tochange=cead6eb63964 Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/076b8758ecb0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130809110236 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/5e0293081301 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130809113838 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=076b8758ecb0&tochange=5e0293081301 Regressed by: 5e0293081301 Corey Ford — Bug 898794 - Store normal frame position before applying relative positioning. r=dbaron

Corey, do you have time to look at this? (If it jeopardizes getting position:sticky landed by the end of your internship, we can probably find somebody else.)

FYI, After resizing browser width, then redraw properly.

Interesting. I'll poke at this (but will have to keep it relatively low-priority for now).

Indeed, I can reproduce this on Linux but not on OS X (even when spoofing User-Agent).

Never mind, I see it on OS X, just not in my day-to-day browsing profile, it seems.

If I restore the ComputedOffsetProperty infrastructure and revert the change to nsBlockFrame::RecoverFloatsFor, the eBay page looks better. (And indeed the div.tabs-b that gets incorrectly placed is relatively positioned and contains a float.) But I'm not sure why that change would be problematic -- I could imagine that we might not have applied relative positioning (and thus stored the normal position) at that point, but the problem seems to happen sometime after the initial rendering. It's also proving difficult to reduce the eBay page to a shorter testcase.

dbaron, any ideas? Alice0775 (or anyone), feel like trying to find a reduced testcase?

Okay, thanks!

So my guess would be that the problem is that there are cases where we move frames (knowing how much to move them) without doing reflow. nsBlockFrame::SlideLine is one of them, but probably not the one you're hitting here. For example, bug 898794 should have fixed SlideLine (for both the block and inline cases) to use GetNormalPosition, adjust the position, and call ApplyRelativePositioning. I suppose that in order to make position:sticky work correctly, we actually need to have common code that all of these cases hit. I'd hope that looking through all the callers of nsIFrame::SetPosition and SetRect should uncover all of the places that need this, and I'm hoping there aren't *too* many of those.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #12) > So my guess would be that the problem is that there are cases where we move > frames (knowing how much to move them) without doing reflow. That would make sense. > I'd hope that looking through all the callers of nsIFrame::SetPosition and > SetRect should uncover all of the places that need this, and I'm hoping > there aren't *too* many of those. No more than a couple dozen that look potentially relevant, anyway...

(In reply to David Baron [:dbaron] (needinfo? me) from comment #12) > use GetNormalPosition, adjust the position, and call ApplyRelativePositioning. It would likely make sense to abstract that even further into a method like nsIFrame::MovePosition(nsPoint aDistance), which could allow doing cleverer things with sticky positioning per bug 904197 comment 5.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #12) > nsBlockFrame::SlideLine is one of them, but probably not the one you're > hitting here. Looks like it was, in fact.

Naturally, reapplying relative positioning means we have to keep track of the computed offsets, so this restores some code similar to pieces I removed in bug 898794.

I only ended up with a couple places where we move a frame by some offset (and I'm not even 100% sure about that RecoverFloats change). Besides occurrences of SetPosition/SetRect that are very clearly during reflow of the frame, I left out a bunch of others, including: * forms/ * mathml/ * tables/ (though bug 35168 might mean some changes are needed there) * nsGfxScrollFrameInner operating on the scrolled frame * nsBlockFrame operating on bullet frames * various uses in nsLineLayout that I *think* are all part of its reflow process and so occur before nsLineLayout::RelativePositionFrames * MoveChildTo in nsColumnSetFrame.cpp Let me know if any of those stand out as potentially needing attention here. This fixes the eBay page from comment 0, but I still haven't figured out enough about how these codepaths are used to be able to write tests for this :( A try run to see whether I broke anything else (both these patches together): https://tbpl.mozilla.org/?tree=Try&rev=d90f647a610a

Comment on attachment 802502 [details] [diff] [review] Part 1: Store computed relative position offsets. >+++ b/layout/generic/nsIFrame.h > NS_DECLARE_FRAME_PROPERTY(NormalPositionProperty, DestroyPoint) >- NS_DECLARE_FRAME_PROPERTY(ComputedStickyOffsetProperty, DestroyMargin) >+ NS_DECLARE_FRAME_PROPERTY(ComputedOffsetProperty, DestroyMargin) "Offset" is a bit of an overloaded term. (e.g. we have nsHTMLReflowState::InitOffsets, which has nothing to do with relative positioning but is actually regarding margin/border/padding.) So I think it'd be worth avoiding having a frame property called "computed offset property" is a bit vague. (though I guess that's what this property used to be called, before bug 898794...) Maybe rename it to "ComputedRelativeOffsetProperty" to indicate that this is *specifically* for IsRelativelyPositioned()-type offsets? (Also: it might be worth splitting this into "part 0", which just does the rename but doesn't change behavior, and then "part 1", which adds the chunk of code in ComputeRelativeOffset; but it's fine as one patch, too.) I'll leave both of those up to you; r=me regardless.

Comment on attachment 802509 [details] [diff] [review] Part 2: Reapply relative positioning when moving frames without reflowing them. >diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp >--- a/layout/generic/nsFrame.cpp >+++ b/layout/generic/nsFrame.cpp >+void >+nsIFrame::MovePosition(const nsPoint& aDistance) >+{ "MovePosition" and "SetPosition" sound kind of the same, and they're ripe for being mistaken for each other since they take the same argument-type. Maybe call this new function "AdjustPositionBy()"? ALSO: I'd rename its argument, "aDistance" -- "distance" is a one-dimensional concept, so I'd avoid using it as a name for a two-dimensional movement. Maybe "aTranslation" or "aShift"? (It looks like we actually usually name this sort of thing "nsPoint offset", but that's probably worth avoiding in this case, to minimized the number of "offset"-named things in play :)) >+ // XXX Probably want to check StyleDisplay()->IsRelativelyPositionedStyle() first >+ const nsMargin* computedOffsets = static_cast<nsMargin*> >+ (Properties().Get(nsIFrame::ComputedOffsetProperty())); >+ nsHTMLReflowState::ApplyRelativePositioning(this, computedOffsets ? >+ *computedOffsets : nsMargin(), >+ &position); I think I agree with that XXX comment -- it seems like we don't need to bother with the property-table lookup, if we're not relatively positioned, right? Maybe replace that with something like const nsMargin* relativeOffsets = nullptr; if (IsRelativelyPositioned()) { relativeOffsets = static_cast<nsMargin*> (Properties().Get(nsIFrame::ComputedOffsetProperty())); } ...possibly with an assertion right after the proptable lookup, to be sure we do have relative offsets stored? >--- a/layout/generic/nsIFrame.h >+++ b/layout/generic/nsIFrame.h >+ * Move the frame, accounting for relative positioning. >+ */ >+ void MovePosition(const nsPoint& aDistance); Please elaborate in this header-comment a bit more. In particular, it'd be useful to explain when to use this function vs. SetPosition(). e.g. "If you're adjusting the frame's position by a fixed amount, you probably want to call this function instead of SetPosition; this will update the frame's saved 'NormalPosition()', among other things."

(In reply to Corey Ford [:corey] from comment #17) > Let me know if any of those stand out as potentially needing attention here. I don't think any of those are particularly worrisome. > This fixes the eBay page from comment 0, but I still haven't figured out > enough about how these codepaths are used to be able to write tests for this > :( Not the end of the world. I'll tag "in-testsuite?" to indicate that this needs testing, but for now I'd be fine with verification based on loading the original URL. > A try run to see whether I broke anything else (both these patches together): > https://tbpl.mozilla.org/?tree=Try&rev=d90f647a610a (You probably noticed already -- unfortunately Try is busted today (bug 914821), so you didn't get any unit tests results :-/ )

Comment on attachment 802509 [details] [diff] [review] Part 2: Reapply relative positioning when moving frames without reflowing them. [r=me on part 2, with comment 19 addressed]

Addressed those comments on the MovePositionBy implementation. Running it by Try again: https://tbpl.mozilla.org/?tree=Try&rev=0086bfb175f5

Try looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f304767ded19 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf930e7d61d3

https://hg.mozilla.org/mozilla-central/rev/f304767ded19 https://hg.mozilla.org/mozilla-central/rev/bf930e7d61d3

I'm worried that part 2 was the cause of bug 916751. If it was, I think this assertion should catch the problem. Even if it's not, I think it's a good assertion to have.

Comment on attachment 805620 [details] [diff] [review] part 3: Add an assertion to check that part 2 only changes things for position:sticky. >+ NS_ASSERTION(StyleDisplay()->mPosition == NS_STYLE_POSITION_STICKY || >+ GetPosition() + aTranslation == position, >+ "MovePositionBy should always lead to the movement " >+ "specified, unless the frame is position:sticky."); Maybe drop the period at the end of the message, to avoid the awkward-looking ".;" in assertion-logging when this assertion fails. r=me either way

Tracking since any additional cleanup & fixes will need approval-mozilla-aurora

I see Try turned up a test that changes an element from position:relative to position:static and hits that assertion via nsLineLayout::TrimTrailingWhiteSpaceIn.

I'll put a patch for that in bug 916751. I suspect it's the problem there.

verified with Nightly and Aurora with builds of 20130919