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)

Product:
Core
Component:
Layout
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
e10s - ---
firefox43 + verified
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: masayuki, Assigned: roc)

References

(
URL
)

Details

(Whiteboard: Test with layout.interruptible-reflow.enabled=true and disabling e10s)

Attachments

(5 files, 2 obsolete files)

simple testcase
359 bytes, text/html
Details
MozReview Request: Bug 1092626. Add aInterrupted parameter to ReflowFinished. r=mats
40 bytes, text/x-review-board-request
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
40 bytes, text/x-review-board-request
Details
updated testcase
360 bytes, text/html
Details
Alternative fix, v3
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.
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.
OS: Windows 8.1 → All
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
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
Similar to Bug 691095
(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.
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)
I don't see this bug on trunk currently. Masayuki, do you still see it?
Flags: needinfo?(roc)
(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.
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?
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.
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)
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)
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
Hmm... Odd. I cannot reproduce this bug with the latest Nightly with default setting around layout. I'll check deeper more.
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.
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
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)
tracking-e10s: --- → -
Attached file simple testcase —
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
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)
I don't have any good ideas for testing this...
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)
Attached file updated testcase —
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)
Attachment #8684515 - Flags: review?(mats)
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 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)
(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.
Flags: needinfo?(mats)
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)
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.
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?
(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)
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.
Attached patch Alternative fix (obsolete) — — Splinter Review
I pushed this to Try, let's see what falls out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d63d5c0103
Attached patch Alternative fix, v2 (obsolete) — — Splinter Review
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
... 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)
Attachment #8685697 - Flags: review?(roc) → review+
(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.
(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.
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!!!
Thanks for testing it Masayuki-san.
Masayuki-san, is the reported use case common when using Twitter? Do you think it's worth uplifting?
Flags: needinfo?(masayuki)
(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)
https://hg.mozilla.org/mozilla-central/rev/f748eade3e29
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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 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+
Tracking since we're uplifting, so I make sure this lands and sticks.
Flags: qe-verify+
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
status-firefox43: fixed → verified
Also verified using latest Developer Edition 44.0a2 and latest Nightly 45.0a1.
Status: VERIFIED → RESOLVED
Closed: 9 years ago → 9 years ago
Status: RESOLVED → VERIFIED
Forgot to drop qeverify as well...sorry for the noise.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: