Closed
Bug 926155
Opened 11 years ago
Closed 11 years ago
Images can not be displayed when using the overflow-x: hidden; and position: sticky;
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: nayinain, Assigned: kip, NeedInfo)
References
Details
(Keywords: regression)
Attachments
(4 files, 5 obsolete files)
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131012030202
Steps to reproduce:
1. Clear your FX's cache.
2. Open the html file.
3. Resize the FX's window.
Actual results:
Images displayed in step 3, not displayed in step 2.
Expected results:
In the step 2 and 3, Images should be displayed.
Sorry for my bad English.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Updated•11 years ago
|
Component: DOM: CSS Object Model → Layout
Comment 1•11 years ago
|
||
Can reproduce in Nightly on Linux and OS X using Shift-Refresh.
I could imagine some sort of problem with the assumption, in the case of replaced elements, that all sizes have been figured out by the time we reflow the scroll container... though once we do figure their sizes out, I assume we'd have to reflow again anyway. And that wouldn't explain why the images themselves get 0x0 frames.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 2•11 years ago
|
||
Just making a note in case it helps - adding position: overflow to a parent container seems to fix the issue for me.
Comment 3•11 years ago
|
||
(In reply to Corey Ford [:corey] from comment #1)
> Can reproduce in Nightly on Linux and OS X using Shift-Refresh.
Yes, I also see it only with Shift-reload (Shift+Ctrl+R).
This seems similar to bug 976655, and might be related.
Comment 4•11 years ago
|
||
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/38d8c6c2c223
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918101519
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d033c301dfd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918104117
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=38d8c6c2c223&tochange=2d033c301dfd
Triggered by:
2d033c301dfd Corey Ford — Bug 902992 - Enable position:sticky in non-release builds (e.g. Nightly and Aurora). r=dholbert
Regression window w/ force layout.css.sticky.enabled = true (m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9fa7a3f9e4d6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906063454
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb399c38e3b6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906063854
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9fa7a3f9e4d6&tochange=bb399c38e3b6
Regressed by: Bug 886646
Keywords: regression
Assignee | ||
Comment 5•11 years ago
|
||
An assertion fails every time the images fail to load:
ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /Users/kgilbert/dev/mozilla-central/layout/base/nsPresShell.cpp, line 2617
The call stack involves StickyScrollContainer::UpdatePositions
Assignee | ||
Comment 6•11 years ago
|
||
Stack trace for assert:
* thread #1: tid = 0x260141, 0x0000000103d673f3 XUL`PresShell::FrameNeedsReflow(this=0x0000000138fe94a0, aFrame=0x000000011b329c70, aIntrinsicDirty=eResize, aBitToAdd=NS_FRAME_HAS_DIRTY_CHILDREN) + 195 at nsPresShell.cpp:2617, queue = 'com.apple.main-thread, stop reason = breakpoint 1.1
frame #0: 0x0000000103d673f3 XUL`PresShell::FrameNeedsReflow(this=0x0000000138fe94a0, aFrame=0x000000011b329c70, aIntrinsicDirty=eResize, aBitToAdd=NS_FRAME_HAS_DIRTY_CHILDREN) + 195 at nsPresShell.cpp:2617
frame #1: 0x0000000103f066eb XUL`mozilla::ScrollFrameHelper::UpdateOverflow(this=0x000000011b329cf8) + 219 at nsGfxScrollFrame.cpp:4002
frame #2: 0x0000000103f2699c XUL`nsHTMLScrollFrame::UpdateOverflow(this=0x000000011b329c70) + 28 at nsGfxScrollFrame.h:488
frame #3: 0x0000000103c621ac XUL`mozilla::OverflowChangedTracker::Flush(this=0x00007fff5fbfba28) + 332 at RestyleTracker.h:116
frame #4: 0x0000000103e7b394 XUL`mozilla::StickyScrollContainer::UpdatePositions(this=0x000000013cb752b0, aScrollPosition=0x00007fff5fbfba98, aSubtreeRoot=0x0000000112d3ad88) + 420 at StickyScrollContainer.cpp:384
frame #5: 0x0000000103efcdc3 XUL`mozilla::ScrollFrameHelper::UpdateSticky(this=0x0000000112d3ae10) + 147 at nsGfxScrollFrame.cpp:4020
frame #6: 0x0000000103efbcef XUL`nsHTMLScrollFrame::Reflow(this=0x0000000112d3ad88, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc088, aReflowState=0x00007fff5fbfbf88, aStatus=0x00007fff5fbfc334) + 2543 at nsGfxScrollFrame.cpp:863
frame #7: 0x0000000103ea72c3 XUL`nsContainerFrame::ReflowChild(this=0x0000000112d3a058, aKidFrame=0x0000000112d3ad88, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc088, aReflowState=0x00007fff5fbfbf88, aX=0, aY=0, aFlags=0, aStatus=0x00007fff5fbfc334, aTracker=0x0000000000000000) + 323 at nsContainerFrame.cpp:958
frame #8: 0x0000000103f7b8a4 XUL`ViewportFrame::Reflow(this=0x0000000112d3a058, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc2e0, aReflowState=0x00007fff5fbfc378, aStatus=0x00007fff5fbfc334) + 708 at nsViewportFrame.cpp:221
frame #9: 0x0000000103d646d8 XUL`PresShell::DoReflow(this=0x0000000138fe94a0, target=0x0000000112d3a058, aInterruptible=true) + 2088 at nsPresShell.cpp:8283
frame #10: 0x0000000103d6ccd0 XUL`PresShell::ProcessReflowCommands(this=0x0000000138fe94a0, aInterruptible=true) + 512 at nsPresShell.cpp:8439
frame #11: 0x0000000103d6c8d8 XUL`PresShell::FlushPendingNotifications(this=0x0000000138fe94a0, aFlush=ChangesToFlush at 0x00007fff5fbfc7b8) + 1736 at nsPresShell.cpp:4091
frame #12: 0x0000000103d8fbab XUL`nsRefreshDriver::Tick(this=0x000000013b939140, aNowEpoch=1394146536433875, aNowTime=TimeStamp at 0x00007fff5fbfcb80) + 2907 at nsRefreshDriver.cpp:1162
frame #13: 0x0000000103d952cc XUL`mozilla::RefreshDriverTimer::TickDriver(driver=0x000000013b939140, jsnow=1394146536433875, now=TimeStamp at 0x00007fff5fbfcbb8) + 92 at nsRefreshDriver.cpp:168
frame #14: 0x0000000103d95154 XUL`mozilla::RefreshDriverTimer::Tick(this=0x00000001001b06e0) + 308 at nsRefreshDriver.cpp:160
frame #15: 0x0000000103d95011 XUL`mozilla::RefreshDriverTimer::TimerTick(aTimer=0x00000001001b0dc0, aClosure=0x00000001001b06e0) + 33 at nsRefreshDriver.cpp:185
frame #16: 0x000000010113ca35 XUL`nsTimerImpl::Fire(this=0x00000001001b0dc0) + 981 at nsTimerImpl.cpp:551
frame #17: 0x000000010113ce41 XUL`nsTimerEvent::Run(this=0x00000001129d3410) + 209 at nsTimerImpl.cpp:635
frame #18: 0x0000000101137ca9 XUL`nsThread::ProcessNextEvent(this=0x00000001107055c0, mayWait=false, result=0x00007fff5fbfcf73) + 1561 at nsThread.cpp:643
frame #19: 0x000000010103582b XUL`NS_ProcessPendingEvents(thread=0x00000001107055c0, timeout=20) + 171 at nsThreadUtils.cpp:210
frame #20: 0x0000000102bbeee9 XUL`nsBaseAppShell::NativeEventCallback(this=0x0000000100159c30) + 201 at nsBaseAppShell.cpp:98
frame #21: 0x0000000102b4abec XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x0000000100159c30) + 428 at nsAppShell.mm:388
frame #22: 0x00007fff911f4731 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
frame #23: 0x00007fff911e5ea2 CoreFoundation`__CFRunLoopDoSources0 + 242
frame #24: 0x00007fff911e562f CoreFoundation`__CFRunLoopRun + 831
frame #25: 0x00007fff911e50b5 CoreFoundation`CFRunLoopRunSpecific + 309
frame #26: 0x00007fff85363a0d HIToolbox`RunCurrentEventLoopInMode + 226
frame #27: 0x00007fff85363685 HIToolbox`ReceiveNextEventCommon + 173
frame #28: 0x00007fff853635bc HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 65
frame #29: 0x00007fff8d15a3de AppKit`_DPSNextEvent + 1434
frame #30: 0x00007fff8d159a2b AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
frame #31: 0x0000000102b49b57 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x0000000100125460, _cmd=0x00007fff8db8d5c3, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x00007fff76e59d00, flag='\x01') + 119 at nsAppShell.mm:165
frame #32: 0x00007fff8d14db2c AppKit`-[NSApplication run] + 553
frame #33: 0x0000000102b4b6d2 XUL`nsAppShell::Run(this=0x0000000100159c30) + 162 at nsAppShell.mm:742
frame #34: 0x000000010480f07c XUL`nsAppStartup::Run(this=0x0000000100158ef0) + 156 at nsAppStartup.cpp:276
frame #35: 0x00000001046c7bdc XUL`XREMain::XRE_mainRun(this=0x00007fff5fbff050) + 6044 at nsAppRunner.cpp:4008
frame #36: 0x00000001046c83e9 XUL`XREMain::XRE_main(this=0x00007fff5fbff050, argc=2, argv=0x00007fff5fbff958, aAppData=0x00007fff5fbff2e8) + 697 at nsAppRunner.cpp:4075
frame #37: 0x00000001046c885d XUL`XRE_main(argc=2, argv=0x00007fff5fbff958, aAppData=0x00007fff5fbff2e8, aFlags=0) + 77 at nsAppRunner.cpp:4285
frame #38: 0x0000000100002117 firefox`do_main(argc=2, argv=0x00007fff5fbff958, xreDirectory=0x000000010011e220) + 1639 at nsBrowserApp.cpp:282
frame #39: 0x0000000100001651 firefox`main(argc=2, argv=0x00007fff5fbff958) + 321 at nsBrowserApp.cpp:643
frame #40: 0x00000001000010b4 firefox`start + 52
Comment 7•11 years ago
|
||
Ah, so the overflow updates triggered by sticky positioning end up traversing too far, in particular to that nsHTMLScrollFrame @ 0x000000011b329c70. It would be useful to cross-reference the stack trace with a frame tree dump to see whether that's the root scroll frame or the "overflow-x:hidden;position:sticky;" element itself.
Possibly we're missing a GetParent() call somewhere such that some part of the code thinks the sticky element is its own scroll container?
Assignee | ||
Comment 8•11 years ago
|
||
The StickyScrollContainer assigned to the root scroll frame was created with StickyScrollContainer::mFrames containing the innermost nsHTMLScrollFrame (which has "overflow-x:hidden;position:sticky;".
Assignee | ||
Comment 9•11 years ago
|
||
nsHTMLScrollFrame::UpdateOverflow() detects that its scroll frame will need to be re-flowed when the overflow area changes and there are scroll bars or the scroll offset is not at the origin. The assert occurs due to calling nsMathMLContainerFrame::UpdateOverflow() during a reflow.
The example testcase can be corrected by adding an overflow-y:hidden to bypass this branch. This results in the image appearing and the assert no longer occurring.
A similar problem may occur with nsMathMLContainerFrame::UpdateOverflow(), which also calls FrameNeedsReflow()
Assignee | ||
Comment 10•11 years ago
|
||
Perhaps this issue can be resolved by deferring the call to FrameNeedsReflow with PresShell::PostReflowCallback; however, perhaps a cleaner solution could solve it without additional passes.
Comment 11•11 years ago
|
||
Why are we using UpdateOverflow on frames currently being reflowed? It should only be used outside of reflow or on frames not currently being reflowed; if a frame is currently being reflowed the reflow will take care of updating overflow.
Comment 12•11 years ago
|
||
So the way this is supposed to work, starting from nsHTMLScrollFrame::Reflow, is:
1. At the end of reflowing a scroll frame (after reflowing its contents), we call StickyScrollContainer::UpdatePositions.
2. UpdatePositions sets up an OverflowChangedTracker with the scroll frame as mSubtreeRoot. As each sticky frame is positioned, it's added to the OverflowChangedTracker.
3. After that's all done, OverflowChangedTracker::Flush updates overflow areas, walking up the frame tree to (but not including) mSubtreeRoot.
4. Then we can return all the way back to nsHTMLScrollFrame::Reflow, update the scroll frame's overflow areas, and continue on.
We indeed ought not to be calling UpdateOverflow on frames currently being reflowed. In the test case, maybe something about the <img> or <table> breaks some of my assumptions about how reflow works?
Comment 13•11 years ago
|
||
Huh, looks like merely "overflow-x:hidden;position:sticky;" is sufficient to trigger that assertion (and then it's unsurprising that that causes problems for later reflows, like the one when the image is loaded).
Comment 14•11 years ago
|
||
And looking at Kip's stack trace from comment 6 again, we have
> nsHTMLScrollFrame::UpdateOverflow(this=0x000000011b329c70)
> nsHTMLScrollFrame::Reflow(this=0x0000000112d3ad88, ...)
I'd guess that the scroll frame Reflow was called on is the root scroll frame (since per comment 8 this does indeed have a StickyScrollContainer attached). But that would mean we're hitting the assertion when calling UpdateOverflow on the sticky frame. Why would we still be reflowing it at that point?
Assignee | ||
Comment 15•11 years ago
|
||
The assert is testing a variable, nsPresShell::mIsReflowing, which does not track individual frames that are reflowing.
Could it be that a different instance of OverflowChangedTracker should be used, which is flushed outside of the reflow window? Perhaps RestyleManager::mOverflowChangedTracker or one instantiated by nsPresShell?
Comment 16•11 years ago
|
||
Following some IRC discussion, it seems the part I hadn't paid enough attention to was that ScrollFrameHelper::UpdateOverflow() can call PresShell::FrameNeedsReflow() when it thinks the scroll position could have changed:
https://hg.mozilla.org/mozilla-central/file/b01286b4ed37/layout/generic/nsGfxScrollFrame.cpp#l3994
So, given a sticky frame that also has scrollbars in at least one dimension, when we position it as part of reflowing its scroll container, the overflow-updating hits this logic and leads to the assertion.
My initial thought is that that logic in ScrollFrameHelper::UpdateOverflow() seems unnecessarily cautious in this situation. We're updating the overflow areas of the scrollable+sticky frame only because we changed its position, not its contents or its size, and so it doesn't seem that its scroll position could have changed. I wonder if we can detect that that's all that's happened and that it's safe to skip the reflow?
Comment 17•11 years ago
|
||
Or... since we're only changing the position of sticky frames, maybe we could get away with not calling UpdateOverflow on them at all, but instead starting with their parents? (Looks like that might involve tweaking the semantics of OverflowChangedTracker, though.)
Assignee | ||
Comment 18•11 years ago
|
||
I have experimented with adding a parameter to nsIFrame::UpdateOverflow to allow the caller to hint if only the position of the overflow is expected to be updated. The experiment included a similar parameter to pass the hint through OverflowChangedTracker::Flush when called from StickyScrollContainer::UpdatePositions. It appears to correct the disappearing image in the testcase and eliminates the assertion.
I am digging a bit further to see if there are further side-effects and that the sticky scroll frames will be reflowed when necessary with the patch in place.
Would this warrant a change to an interface (nsIFrame)? Perhaps Corey's solution, localized to OverflowChangedTracker, would be less intrusive and preferable?
Assignee | ||
Comment 19•11 years ago
|
||
This proposed fix is experimental. Additional testcases and reftests will be added to confirm if it is effective and without side effects.
Also please see comment 17 and 18 for alternate solution which may be less invasive.
Attachment #8390774 -
Flags: review?(corey)
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
Comment on attachment 8390774 [details] [diff] [review]
V1 fix for Bug 926155 (experimental)
I believe overflow areas are stored relative to the frame's position, so when we position sticky frames we should actually expect their overflow areas not to change at all. So I don't think this is the right approach.
Attachment #8390774 -
Flags: review?(corey)
Comment 23•11 years ago
|
||
The fact that MathML can (indeed, always does) fall back to a full reflow means that the just-start-with-the-parent(s) idea won't suffice either -- sticky inside MathML triggers the same assertion,
Comment 24•11 years ago
|
||
Or rather, starting overflow updates from sticky frames' parents seems like a sensible solution to the (original) scrollable+sticky scenario.
I don't know why nsMathMLContainerFrame::UpdateOverflow needs to fall back to a reflow, but some options there might include (a) optimizing it to not reflow (b) deferring that reflow to another pass (c) disallowing position:sticky on/in MathML.
(Ultimately, this all appears to come from the fact that UpdateOverflow was designed as an optimization to avoid full reflows where possible, yet the position:sticky implementation assumed that UpdateOverflow could be safely used while another reflow was in progress.)
dbaron, assuming the problem makes sense to you now, do you have any ideas?
Flags: needinfo?(dbaron)
Assignee | ||
Comment 25•11 years ago
|
||
Patch submitted for Bug 984226 partially corrects this issue: https://bugzilla.mozilla.org/attachment.cgi?id=8393971
In addition to that patch need reftest / mochitest and need to address issue demonstrated in the MathML testcase.
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8394027 -
Flags: review?(corey)
Assignee | ||
Comment 27•11 years ago
|
||
Reftest has been updated to use MozReftestInvalidate instead of setTimeout
Attachment #8394027 -
Attachment is obsolete: true
Attachment #8394027 -
Flags: review?(corey)
Attachment #8394046 -
Flags: review?(dholbert)
Comment 28•11 years ago
|
||
Comment on attachment 8394046 [details] [diff] [review]
V2 Test for Bug 926155 - Non MathML test case
Two extreme nits:
>+++ b/layout/reftests/bugs/926155-1.html
>+ function doTest() {
>+ var x=document.getElementById('testdiv');
>+ x.style.width="200px";
>+ document.documentElement.className = "";
Mozilla coding style is to have spaces around operators like "=", so I'd suggest adding spaces around the "=" in the assignment for x and x.style.width. (for consistency with the last line, too)
>+++ b/layout/reftests/bugs/reftest.list
> == 921716-1.html 921716-1-ref.html
> fuzzy-if(cocoaWidget,1,40) == 928607-1.html 928607-1-ref.html
>+== 926155-1.html 926155-1-ref.html
> == 931464-1.html 931464-1-ref.html
This is inserting 1 line too low, for sorted order.
(926155-1.html should go before 928607-1.html)
r=me with those fixed. Thanks!
Attachment #8394046 -
Flags: review?(dholbert) → review+
Comment 29•11 years ago
|
||
With bug 63895 landed (including a new use of OverflowChangedTracker), I wonder whether we can similarly trigger this assertion with absolutely-positioned elements inside scrollframes/MathML inside relatively-positioned table parts, or something like that.
Assignee | ||
Comment 30•11 years ago
|
||
Updated with feedback in Comment 28
Attachment #8394046 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → kgilbert
Assignee | ||
Comment 31•11 years ago
|
||
Comment on attachment 8390774 [details] [diff] [review]
V1 fix for Bug 926155 (experimental)
Patch obsoleted by fix applied in Bug 984226
Attachment #8390774 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 8390925 [details] [diff] [review]
Alternative fix for Bug 926155, based on Comment 17
Patch obsoleted by fix applied in Bug 984226
Attachment #8390925 -
Attachment is obsolete: true
Assignee | ||
Comment 33•11 years ago
|
||
Comment on attachment 8391093 [details]
asserting testcase with sticky inside MathML
A separate bug has been entered related to this testcase, Bug 997893. The testcase attachment has also been moved there.
Assignee | ||
Updated•11 years ago
|
Attachment #8391093 -
Attachment is obsolete: true
Assignee | ||
Comment 34•11 years ago
|
||
This bug has been corrected with Bug 984226.
The attached reftest, bug926155_reftest.patch, needs check-in.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 35•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 36•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 37•11 years ago
|
||
But if you want sticky past mozilla-aurora, you have to ask for it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/810c2b6a278d
https://hg.mozilla.org/releases/mozilla-aurora/rev/84046431c830
Comment 38•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•