Closed Bug 911786 Opened 6 years ago Closed 6 years ago

images on eBay display off to the right from where they should due to relative positioning bug

Categories

(Core :: Layout: Positioned, defect)

26 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox25 --- unaffected
firefox26 + verified
firefox27 --- verified

People

(Reporter: anshprat, Assigned: coyotebush)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

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
Blocks: 898794
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: R & A Pos
Ever confirmed: true
Keywords: regression
OS: Linux → All
Product: Firefox → Core
Summary: Ebay CSS broekn on FF nightly - linux 64 bit → Ebay CSS broekn on FF nightly - linux 64 bit and Windows
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.)
Flags: needinfo?(cford)
Summary: Ebay CSS broekn on FF nightly - linux 64 bit and Windows → images on eBay display off to the right from where they should due to relative positioning bug
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).
Assignee: nobody → cford
Flags: needinfo?(cford)
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?
Flags: needinfo?(dbaron)
Flags: needinfo?(alice0775)
Attached file give up reduce html
Flags: needinfo?(alice0775)
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.
Flags: needinfo?(dbaron)
(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.
Attachment #802502 - Flags: review?(dholbert)
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
Attachment #802509 - Flags: review?(dholbert)
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.
Attachment #802502 - Flags: review?(dholbert) → review+
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 :-/ )
Flags: in-testsuite?
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]
Attachment #802509 - Flags: review?(dholbert) → review+
Addressed those comments on the MovePositionBy implementation.

Running it by Try again:
https://tbpl.mozilla.org/?tree=Try&rev=0086bfb175f5
Attachment #802509 - Attachment is obsolete: true
Attachment #803204 - Flags: review+
Try looks good.
Keywords: checkin-needed
Attachment #799083 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f304767ded19
https://hg.mozilla.org/mozilla-central/rev/bf930e7d61d3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Attachment #805620 - Flags: review?(dholbert)
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
Attachment #805620 - Flags: review?(dholbert) → review+
Tracking since any additional cleanup & fixes will need approval-mozilla-aurora
Depends on: 916751
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.