Closed
Bug 1092626
Opened 10 years ago
Closed 9 years ago
[non-e10s] twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: masayuki, Assigned: roc)
References
()
Details
(Whiteboard: Test with layout.interruptible-reflow.enabled=true and disabling e10s)
Attachments
(5 files, 2 obsolete files)
359 bytes,
text/html
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
360 bytes,
text/html
|
Details | |
1.87 KB,
patch
|
roc
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
STR #1:
1. Scroll down your timeline a lot.
2. Click a tweet which has one or more images.
3. Click an image in the tweet for zoom.
4. Close the image.
STR #2:
1. Scroll down your timeline a lot.
2. Click RT.
3. Click RT button on the confirmation dialog.
Actual result:
After 1-4 or 2-3, i.e., closing twitter's dialog, timeline scrolls up.
Expected result:
Even after closing twitter's dialog, timeline should keep the scroll position.
I'm not sure if this is a bug of us or twitter's bug. I cannot reproduce this bug with Google Chrome.
I've reproduce this bug at least for several months with Nightly.
Comment 1•10 years ago
|
||
This bug is a real annoyance - I'm on Ubuntu Linux 14.10 x86_64 & Firefox 33.0.
I have to switch to Chrome while using twitter. Chrome does not have this bug.
Reporter | ||
Updated•10 years ago
|
OS: Windows 8.1 → All
Reporter | ||
Comment 2•10 years ago
|
||
When user opens image, twitter.com sets overflow-y of <body> to hidden. And when user close the image dialog, twitter.com restores the overflow-y to scroll. However, I cannot reproduce this bug with simplest testcase which just hides and restores scrollbar of <body>. So, I guess that something doing for the dialog breaks the scroll position of <body>.
twitter_more_1.bundle.css:
> .modal-enabled,.grid-enabled,.overlay-enabled,.gallery-enabled{
> position:relative;
> overflow:hidden
> }
> this.open = function (a, b) {
> this.calculateScrollbarWidth(),
> this.fromGrid = b && !!b.fromGrid,
> this.title = b && b.title ? b.title : this.attr.defaultGalleryTitle,
> this.select('galleryTitleSelector').text(this.title),
> b && b.inOverlay ? this.$node.addClass(this.attr.inOverlayGalleryClass) : this.$node.removeClass(this.attr.inOverlayGalleryClass),
> b && b.showGrid && b.profileUser ? (this.select('gallerySelector').removeClass('no-grid'), this.select('gridSelector').attr('href', '/' + b.profileUser.screen_name + '/media'), this.select('gridSelector').find('.visuallyhidden').text(b.profileUser.name), this.select('gridSelector').addClass('js-nav')) : (this.select('gallerySelector').addClass('no-grid'), this.select('gridSelector').removeClass('js-nav'));
> var c = $(a.target).closest(this.attr.mediaSelector);
> if (this.isOpen() || c.length == 0) return;
> b && b.timelineSelector ? this.$timeline = c.closest(b.timelineSelector) : this.$timeline = c.parent(),
> imageResizer.resetMinSize(this.select('gallerySelector')),
> this.render(c),
> $('body').addClass('gallery-enabled'),
> this.select('gallerySelector').addClass('show-controls'),
> this.on('mousemove', function () {
> this.select('gallerySelector').removeClass('show-controls')
> }.bind(this)),
> this.trigger('uiGalleryOpened')
> },
> this.closeGallery = function () {
> $('body').hasClass('gallery-enabled') && ($('body').removeClass('gallery-enabled'), this.select('galleryMediaSelector').empty(), this.hideNav(), this.enableNav(!1, !1), this.off(window, 'resize', this.debouncedResize), this.off('mousemove'), delete this.$timeline, delete this.$current, this.trigger('uiGalleryClosed'), this.trigger('uiDialogClosed'))
> },
Adding gallery-enabled class overwrites the overflow property.
Component: Untriaged → Layout
Comment 3•10 years ago
|
||
I can reproduce the problem(STR #1) on Nightly36.0a1, Firefox 31esr, 24esr, 17esr, 10esr as well as Firefox4, but not Firefox3.6.
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/b94cf8147ccd
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre ID:20100111140136
Bad:
http://hg.mozilla.org/mozilla-central/rev/47cd92d616d7
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre ID:20100111150802
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b94cf8147ccd&tochange=47cd92d616d7
Triggered by Bug 526394
In local build
Last Good: fada8c5cef07
First Bad: 2f7d76044ee8
Comment 4•10 years ago
|
||
Similar to Bug 691095
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Alice0775 White from comment #4)
> Similar to Bug 691095
Yeah, but Asa said that it's scrolled without user action. I guess that the bug has already been fixed by twitter.com side or our side.
Reporter | ||
Comment 6•10 years ago
|
||
roc:
The regression point is your bug. Do you have any ideas? Or, do you know somebody who can research this? This is really annoying bug for twitter uses who use Web UI...
Flags: needinfo?(roc)
Assignee | ||
Comment 7•10 years ago
|
||
I don't see this bug on trunk currently. Masayuki, do you still see it?
Flags: needinfo?(roc)
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> I don't see this bug on trunk currently. Masayuki, do you still see it?
Yeah, I can reproduce this on today's Nightly build too.
If you want some stack trace or something, I could provide it.
Reporter | ||
Comment 9•10 years ago
|
||
When this occurs, ScrollFrameHelper::ScrollToImpl() is called by ScrollFrameHelper::ReflowFinished().
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=0b743302cbfb#4532
> 4532 // Clamp current scroll position to new bounds. Normally this won't
> 4533 // do anything.
> 4534 nsPoint currentScrollPos = GetScrollPosition();
> 4535 ScrollToImpl(currentScrollPos, nsRect(currentScrollPos, nsSize(0, 0)));
Could be the position is already clamped or still overflow is hidden yet?
Reporter | ||
Comment 10•10 years ago
|
||
When it's scrolled up, each value is:
ScrollFrameHelper::ScrollToImpl():
aPt={ x=0, y=3703029 }
> 2355 nsPoint curPos = GetScrollPosition();
curPos={ x=0, y=3703029 }
> 2356 nsPoint alignWithPos = mScrollPosForLayerPixelAlignment == nsPoint(-1,-1)
> 2357 ? curPos : mScrollPosForLayerPixelAlignment;
alignWithPos={ x=0, y=3703029 }
> 2358 // Try to align aPt with curPos so they have an integer number of layer
> 2359 // pixels between them. This gives us the best chance of scrolling without
> 2360 // having to invalidate due to changes in subpixel rendering.
> 2361 // Note that when we actually draw into a PaintedLayer, the coordinates
> 2362 // that get mapped onto the layer buffer pixels are from the display list,
> 2363 // which are relative to the display root frame's top-left increasing down,
> 2364 // whereas here our coordinates are scroll positions which increase upward
> 2365 // and are relative to the scrollport top-left. This difference doesn't actually
> 2366 // matter since all we are about is that there be an integer number of
> 2367 // layer pixels between pt and curPos.
> 2368 nsPoint pt =
> 2369 ClampAndAlignWithLayerPixels(aPt,
> 2370 GetScrollRangeForClamping(),
> 2371 aRange,
> 2372 alignWithPos,
> 2373 appUnitsPerDevPixel,
> 2374 scale);
pt={ x=0, y=30660 }
> 2381 nsPoint dist(std::abs(pt.x - mLastUpdateImagesPos.x),
> 2382 std::abs(pt.y - mLastUpdateImagesPos.y));
mLastUpdateImagesPos={ x=-1, y=-1 }
dist={ x=1, y=30661 }
> 2383 nsSize scrollPortSize = GetScrollPositionClampingScrollPortSize();
scrollPortSize={ width=69240, height=50520 }
> 2384 nscoord horzAllowance = std::max(scrollPortSize.width / std::max(sHorzScrollFraction, 1),
> 2385 nsPresContext::AppUnitsPerCSSPixel());
> 2386 nscoord vertAllowance = std::max(scrollPortSize.height / std::max(sVertScrollFraction, 1),
> 2387 nsPresContext::AppUnitsPerCSSPixel());
horzAllowance=34620, vertAllowance=25260
> 2388 if (dist.x >= horzAllowance || dist.y >= vertAllowance) {
> 2389 needImageVisibilityUpdate = true;
> 2390 }
> 2391
> 2392 if (needImageVisibilityUpdate) {
> 2393 presContext->PresShell()->ScheduleImageVisibilityUpdate();
> 2394 }
> 2395
> 2396 // notify the listeners.
> 2397 for (uint32_t i = 0; i < mListeners.Length(); i++) {
> 2398 mListeners[i]->ScrollPositionWillChange(pt.x, pt.y);
> 2399 }
> 2400
> 2401 nsPoint oldScrollFramePos = mScrolledFrame->GetPosition();
oldScrollFramePos={ x=0, y=-3703029 }
> 2402 // Update frame position for scrolling
> 2403 mScrolledFrame->SetPosition(mScrollPort.TopLeft() - pt);
mScrolledFrame->GetPosition()={ x=0, y=-30660 }
I still don't understand why the pt is strange point value.
Reporter | ||
Comment 11•10 years ago
|
||
aPt={ x=0, y=2894350 },
aRange={ x=0, y=2894350, XMost()=0, YMost()=2894350 },
appUnitsPerDevPixel=60,
scale={ width=1.000000, height=1.000000 },
curPos={ x=0, y=2894350 },
GetScrollRangeForClamping()={ x=0, y=0, XMost()=0, YMost()=43620 }(ShouldClampScrollPosition()=true),
alignWithPos={ x=0, y=2894350 },
pt={ x=0, y=43620 },
mLastUpdateImagesPos={ x=-1, y=-1 },
dist={ x=1, y=43621 },
scrollPortSize={ width=69240, height=50520 },
horzAllowance=34620, vertAllowance=25260,
oldScrollFramePos={ x=0, y=-2894350 },
mScrolledFrame->GetPosition()={ x=0, y=-43620 }
Looks like that GetScrollRangeForClamping() result causes computing wrong scroll destination.
Roc, any idea?
FYI: When it's not scrolled, looks like |if (pt == curPos)| is true and quit.
Flags: needinfo?(roc)
Assignee | ||
Comment 12•10 years ago
|
||
I still can't reproduce this.
It certainly looks like GetScrollRangeForClamping is at fault. Can you drill down further? What is aScrolledFrameOverflowArea in ScrollFrameHelper::GetScrolledRectInternal? I'm guessing its height is too small.
I wonder if this is due to interruptible reflow. Can you try disabling that by setting layout.interruptible-reflow.enabled to false?
Flags: needinfo?(roc)
Comment 13•10 years ago
|
||
Wow! Setting layout.interruptible-reflow.enabled to false
It seems to fix the problem for me.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150503030209
Reporter | ||
Comment 14•10 years ago
|
||
Hmm... Odd. I cannot reproduce this bug with the latest Nightly with default setting around layout. I'll check deeper more.
Comment 15•10 years ago
|
||
It is easy to reproduce the problem when e10s disabled.
ex. page https://twitter.com/IE_Japan
I do not know whether the problem occurs when e10s enabled.
Reporter | ||
Comment 16•10 years ago
|
||
Excellent!! This bug can be reproduced only without e10s. And I confirmed not to reproduce this bug with layout.interruptible-reflow.enabled=false.
Roc, don't you enable e10s when you test this?
Flags: needinfo?(roc)
Summary: twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image → [non-e10s] twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image
Whiteboard: Test with layout.interruptible-reflow.enabled=true and disabling e10s
Assignee | ||
Comment 17•10 years ago
|
||
I had tested with e10s enabled. With e10s disabled, I can reproduce this.
So I guess an interruptible reflow occurs and we stop restoring the scroll position prematurely.
Flags: needinfo?(roc)
Updated•10 years ago
|
tracking-e10s:
--- → -
Assignee | ||
Comment 18•9 years ago
|
||
Load the page, and when it's finished loading click in the page and quickly move the mouse. The scroll position should shift from the end of the page to somewhere further up.
Assignee: nobody → roc
Assignee | ||
Comment 19•9 years ago
|
||
Bug 1092626. Add aInterrupted parameter to ReflowFinished. r=mats
Attachment #8684514 -
Flags: review?(mats)
Assignee | ||
Comment 20•9 years ago
|
||
Bug 1092626. When a reflow is interrupted and a scroll position is clamped, try to restore to the old scroll position when the reflow completes. r=mats
Attachment #8684515 -
Flags: review?(mats)
Assignee | ||
Comment 21•9 years ago
|
||
I don't have any good ideas for testing this...
Comment 22•9 years ago
|
||
I'm not sure I understand what the problem is. After attachment 8684208 [details] is
fully loaded, is the expected result that there should be no scroll movement
when I click on the text?
The actual result I see in Linux builds is that the text moves due to a vertical
scroll (and that by just clicking - no mouse movement needed after the click).
I see that result also with the patches applied, and also with
layout.interruptible-reflow.enabled=false
Flags: needinfo?(roc)
Assignee | ||
Comment 23•9 years ago
|
||
Sorry, in an opt build the previous testcase completes too quickly to trigger a layout interrupt.
Try this testcase instead. Same steps as last time: wait for the page to finish loading and scroll to the bottom, then click in the body and move the mouse quickly. You should find that it scrolls to somewhere near the top.
Flags: needinfo?(roc)
Updated•9 years ago
|
Attachment #8684515 -
Flags: review?(mats)
Comment 24•9 years ago
|
||
Comment on attachment 8684515 [details]
MozReview Request: Bug 1092626. When a reflow is interrupted and a scroll position is clamped, try to restore to the old scroll position when the reflow completes. r=mats
https://reviewboard.mozilla.org/r/24597/#review22141
r- since it doesn't fix the bug for me.
::: layout/generic/nsGfxScrollFrame.cpp:4758
(Diff revision 1)
> + mRestorePos = currentScrollPos;
Thanks for the additional testcase. I can now reproduce the reported problem. Unfortunately though, applying both patches here doesn't fix the problem for me (Linux x86-64 m-i debug build). I see that we hit this line but it just doesn't seem to help for some reason.
Also, why not simply use "mOuter->PresContext()->HasPendingInterrupt()" instead of propagating the aInterrupted flag? and possibly also NS_SUBTREE_DIRTY(mOuter) to avoid interrupts that didn't affect this subtree.
FWIW, I still see the issue when just clicking as described in comment 22. In this case it scrolls just 10px or so. It might be related though.
Comment 25•9 years ago
|
||
Comment on attachment 8684514 [details]
MozReview Request: Bug 1092626. Add aInterrupted parameter to ReflowFinished. r=mats
https://reviewboard.mozilla.org/r/24595/#review22143
Maybe we can just use "mOuter->PresContext()->HasPendingInterrupt()" instead?
and/or NS_SUBTREE_DIRTY(mOuter).
Attachment #8684514 -
Flags: review?(mats)
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #24)
> Thanks for the additional testcase. I can now reproduce the reported
> problem. Unfortunately though, applying both patches here doesn't fix the
> problem for me (Linux x86-64 m-i debug build). I see that we hit this line
> but it just doesn't seem to help for some reason.
It definitely works for me. Can you look into why it isn't working for you?
> Also, why not simply use "mOuter->PresContext()->HasPendingInterrupt()"
> instead of propagating the aInterrupted flag? and possibly also
> NS_SUBTREE_DIRTY(mOuter) to avoid interrupts that didn't affect this subtree.
I don't want to create a dependency on exactly when the interrupt state is cleared. Only applying this change when NS_SUBTREE_DIRTY is true would be a good improvement though.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mats)
Comment 27•9 years ago
|
||
Sure, on the next call (after the interrupted reflow) to ReflowFinished,
ScrollToRestoredPosition() is called but it takes the early return
because mLastPos=-1,-1. So if I add "mLastPos = GetScrollPosition()"
after the mRestorePos assignment in you patch, then it works as
expected. I'm not happy with the result though, the scrollbar thumb
jumps very clearly from the end to the top and then back to the end.
In my debug build this takes about one second and is clearly visible.
I guess it's less of a problem in an optimized build but I think we
can do better.
If I *instead of* your part 2 patch add this as the first statement
in ReflowFinished():
if (mRestorePos.x == -1 && aInterrupted && NS_SUBTREE_DIRTY(mOuter)) {
return false;
}
this also fixes the bug, and it looks much better - the scrollbar
thumb doesn't jump and the "paint flash" feels less disturbing.
Does that also work for you?
Flags: needinfo?(mats) → needinfo?(roc)
Comment 28•9 years ago
|
||
This also works:
if (aInterrupted && NS_SUBTREE_DIRTY(mOuter)) {
return false;
}
I don't think we really care what mRestorePos is at the top of this
method. A comment to go along with the code above would be:
// We will get another call after the next reflow and doing the
// scroll later is less janky.
Comment 29•9 years ago
|
||
BTW, just checking "NS_SUBTREE_DIRTY(mOuter)" also works for me.
I don't really see why we need to check aInterrupted at all.
All we need to know is that we will get another call after the
next reflow, right?
Assignee | ||
Comment 30•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #29)
> BTW, just checking "NS_SUBTREE_DIRTY(mOuter)" also works for me.
> I don't really see why we need to check aInterrupted at all.
> All we need to know is that we will get another call after the
> next reflow, right?
Yes, that sounds right.
I worry about the consequences of allowing the scroll position to be outside the scroll range, though. That breaks an invariant that could be important. Seems to me the scrollbar could be rendered completely incorrectly in this case and the user might even be able to interact with it and get surprising results. The fix that I posted here is conservative and doesn't break any invariants.
I guess we can go with your approach and see what happens. Do you want to post it, or should I?
Flags: needinfo?(roc)
Comment 31•9 years ago
|
||
OK, let's try that and see if it works. It's almost bedtime here so it's faster
if you post it and make a Try run. I can review it tomorrow.
Comment 32•9 years ago
|
||
I pushed this to Try, let's see what falls out:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d63d5c0103
Comment 33•9 years ago
|
||
Whoa, plenty of failures in the above Try push.
I think the problem is that we need to always reset mPostedReflowCallback
before returning, so I moved that to the top. New Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea34282db2e
Attachment #8685389 -
Attachment is obsolete: true
Comment 34•9 years ago
|
||
... and here's a Try run with Opt builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=449a9bdf29ab
It looks OK to me; I think the failures are unrelated.
I also removed the NS_SUBTREE_DIRTY(mOuter) check later in the method
since it can't be true there.
I've tested that this patch fixes the problem locally in a Linux debug build.
Can you verify one of the Try builds fix it on Windows?
Attachment #8685526 -
Attachment is obsolete: true
Attachment #8685697 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8685697 -
Flags: review?(roc) → review+
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #34)
> I've tested that this patch fixes the problem locally in a Linux debug build.
> Can you verify one of the Try builds fix it on Windows?
Not easily, no, my Windows laptop is sick.
Reporter | ||
Comment 36•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> (In reply to Mats Palmgren (:mats) from comment #34)
> > I've tested that this patch fixes the problem locally in a Linux debug build.
> > Can you verify one of the Try builds fix it on Windows?
>
> Not easily, no, my Windows laptop is sick.
I can check it after meeting.
Reporter | ||
Comment 37•9 years ago
|
||
I tested briefly the trysever build in comment 34 on Windows (non-e10s, layout.interruptible-reflow.enabled is true), I don't see the reported problem on twitter.com.
Thank you very much!!!
Comment 38•9 years ago
|
||
Thanks for testing it Masayuki-san.
Comment 39•9 years ago
|
||
Comment 40•9 years ago
|
||
Masayuki-san, is the reported use case common when using Twitter?
Do you think it's worth uplifting?
Flags: needinfo?(masayuki)
Reporter | ||
Comment 41•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #40)
> Masayuki-san, is the reported use case common when using Twitter?
Yes. I don't see similar symptom in other websites.
> Do you think it's worth uplifting?
If the patch is not so risky, I'd like you to do that. I still see some tweets to complain about this bug. When they try to use Chrome, this bug could be a reason of switching their default browser.
Flags: needinfo?(masayuki)
Comment 42•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 43•9 years ago
|
||
Comment on attachment 8685697 [details] [diff] [review]
Alternative fix, v3
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: scroll jank on Twitter timeline page
[Describe test coverage new/current, TreeHerder]: manual testing (testcase attached to this bug)
[Risks and why]: medium risk
[String/UUID change made/needed]: none
The reasons I think this might be worth an uplift:
* Twitter is a popular site so the issue might affect many users
* we're still early in the cycle(?)
Attachment #8685697 -
Flags: approval-mozilla-beta?
Attachment #8685697 -
Flags: approval-mozilla-aurora?
Comment 44•9 years ago
|
||
Comment on attachment 8685697 [details] [diff] [review]
Alternative fix, v3
Let's uplift this, as mats says it's high impact on users and we're still early in beta. This should make it into beta 3.
Attachment #8685697 -
Flags: approval-mozilla-beta?
Attachment #8685697 -
Flags: approval-mozilla-beta+
Attachment #8685697 -
Flags: approval-mozilla-aurora?
Attachment #8685697 -
Flags: approval-mozilla-aurora+
Comment 45•9 years ago
|
||
Tracking since we're uplifting, so I make sure this lands and sticks.
Updated•9 years ago
|
status-firefox43:
--- → affected
status-firefox44:
--- → affected
tracking-firefox43:
--- → +
tracking-firefox44:
--- → +
tracking-firefox45:
--- → +
Comment 46•9 years ago
|
||
bugherder uplift |
Comment 47•9 years ago
|
||
bugherder uplift |
Comment 48•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
Updated•9 years ago
|
Flags: qe-verify+
Comment 49•9 years ago
|
||
Reproduced the initial issue using an old nightly from 2015-10-01, verified that the issue is fixed using Firefox 43 beta 3 build 2 across platforms (Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
Comment 50•9 years ago
|
||
Also verified using latest Developer Edition 44.0a2 and latest Nightly 45.0a1.
Status: VERIFIED → RESOLVED
Closed: 9 years ago → 9 years ago
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•