Closed
Bug 1078373
Opened 10 years ago
Closed 10 years ago
[Flame] horizontal scrollbar doesn't appear after zooming in on a page that is not initially horizontally scrollable
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
People
(Reporter: njpark, Assigned: kats)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
kats
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
STR:
Open browser, and go to http://cnn.com
Zoom in slightly, and scroll horizontally.
Expected:
the scroll bar moves smoothly.
Actual:
both the horizontal and vertical scroll bars disappear, unless the user scrolls the zoomed in screen to the leftmost position (then the vertical scroll bar appears)
Version Info:
Gaia-Rev 470826d13ae130a5c3d572d1029e595105485fb0
Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/e0d714f43edc
Build-ID 20141006040204
Version 35.0a1
Device-Name flame
FW-Release 4.4.2
FW-Incremental eng.cltbld.20141006.074035
FW-Date Mon Oct 6 07:40:45 EDT 2014
Bootloader L1TC00011840
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
this also happens in http://www.mozilla.org site
Comment 2•10 years ago
|
||
[Blocking Requested - why for this release]:
If we want bug 995519 uplifted for 2.1, we need this bug fixed as well.
blocking-b2g: --- → 2.1?
Assignee | ||
Comment 3•10 years ago
|
||
I looked at this a bit and part of this is the same issue as bug 1076447. When you zoom in and scroll horizontally on cnn.com, you run into the case described at bug 1076447 comment 2 where the vertical scrollbar disappears.
The other issue is the horizontal scrollbar disappearing - which is not quite accurate because the horizontal scrollbar never appears on this page at all. The layout code is not aware that the page is horizontally scrollable (because it only becomes horizontally scrollable after applying a zoom) and so never draws a horizontal scrollbar at all.
I'll use this bug to track the issue as described above.
Summary: [Flame] scrolling when zoomed in causes the scroll bar to disappear on some websites → [Flame] horizontal scrollbar doesn't appear after zooming in on a page that is not initially horizontally scrollable
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
This seems to do the job and from some limited testing I don't see any other fallout.
Assignee: nobody → bugmail.mozilla
Attachment #8501987 -
Attachment is obsolete: true
Attachment #8502150 -
Flags: review?(tnikkel)
Updated•10 years ago
|
blocking-b2g: 2.1? → 2.1+
status-b2g-v2.0:
--- → unaffected
Reporter | ||
Comment 6•10 years ago
|
||
I tested with applying all related 2.1 patches from http://people.mozilla.org/~kgupta/tmp/scrollbar/ on flame, and I could not reproduced this issue, and scroll functionality looks good as well.
Comment 7•10 years ago
|
||
Comment on attachment 8502150 [details] [diff] [review]
Patch
So this patch is only working because the patch for bug 1078316 was accidentally triggering a full reflow of the scroll frame, instead of just the scrollbars.
Attachment #8502150 -
Flags: review?(tnikkel)
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
I rebased tn's patch on top of master and split it into two for easier review. This first one is pure refactoring; it just pulls out a few helper methods.
Attachment #8503146 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
This second part does the actual work of updating the scrollbar code when the scrollport size changes.
Attachment #8502150 -
Attachment is obsolete: true
Attachment #8503102 -
Attachment is obsolete: true
Attachment #8503147 -
Flags: review+
Comment 11•10 years ago
|
||
Comment on attachment 8503147 [details] [diff] [review]
Part 2 - Update scrollbars when the scrollport changes
Review of attachment 8503147 [details] [diff] [review]:
-----------------------------------------------------------------
I've tried to review this, but it probably needs somebody with more reflow experience.
::: layout/generic/nsGfxScrollFrame.cpp
@@ +2186,5 @@
> (nsLayoutUtils::GetCrossDocParentFrame(mOuter->PresContext()->PresShell()->GetRootFrame()));
> return subdocFrame && !subdocFrame->ShouldClipSubdocument();
> }
>
> +void
static void
@@ +2245,5 @@
> + }
> + mHasVerticalScrollbar = wantVScrollbar;
> +
> + nsRect contentRect = mOuter->GetContentRectRelativeToSelf();
> + LayoutScrollbarsInternal(boxState, contentRect);
This makes me uncomfortable. This is basically doing a reflow on two frames outside of the official reflow, right? Do we do similar things in other places?
@@ +2255,3 @@
> if (mVScrollbarBox) {
> + presShell->FrameNeedsReflow(mVScrollbarBox,
> + nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN);
Why is this needed? And why did this change from NS_FRAME_IS_DIRTY to NS_FRAME_HAS_DIRTY_CHILDREN?
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #11)
> This makes me uncomfortable. This is basically doing a reflow on two frames
> outside of the official reflow, right? Do we do similar things in other
> places?
Basically, yes. We already do a reflow on just fixed-position elements when the scroll-position-clamping scrollport size (SPCSPS) changes, at [1]. That code has been there for a long time. There are also lots of other callers to nsPresShell::FrameNeedsReflow where we reflow individual frames for various reasons. Does that answer your question?
> @@ +2255,3 @@
> > if (mVScrollbarBox) {
> > + presShell->FrameNeedsReflow(mVScrollbarBox,
> > + nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN);
>
> Why is this needed? And why did this change from NS_FRAME_IS_DIRTY to
> NS_FRAME_HAS_DIRTY_CHILDREN?
The reason it was NS_FRAME_IS_DIRTY to begin with was due to an error in my patch for bug 1078316. We wanted to do a reflow on just the scrollbars, but the NS_FRAME_IS_DIRTY actually triggered a reflow on the entire scrollframe. That was a regression, but given the structure of the code it was actually the only way to re-run the code that determines if the scrollbars are visible or not.
This patch refactors the code so that the overlay scrollbar frames are always present (hence no need to reflow the scrollframe) and then reflows the children of the scrollbar frames (the thumbs) so that are made shown/hidden and sized correctly when the APZ code changes the SPCSPS.
[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=7fce65c28e7c#10647
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> (In reply to Markus Stange [:mstange] from comment #11)
> > This makes me uncomfortable. This is basically doing a reflow on two frames
> > outside of the official reflow, right? Do we do similar things in other
> > places?
>
> Basically, yes. We already do a reflow on just fixed-position elements when
> the scroll-position-clamping scrollport size (SPCSPS) changes, at [1]. That
> code has been there for a long time. There are also lots of other callers to
> nsPresShell::FrameNeedsReflow where we reflow individual frames for various
> reasons. Does that answer your question?
Ignore that answer; I misunderstood the question. The FrameNeedsReflow just marks a thing as needing reflow whereas here we're actually running reflow code, which is different. I don't know the answer then.
Assignee | ||
Comment 14•10 years ago
|
||
Just to clarify the question above, the call to LayoutScrollbarsInternal invokes nsBoxFrame::LayoutChildAt, which we're unsure can be safely invoked from outside a reflow. I asked dholbert and he wasn't sure either. bz, do you know? The function is being called as part of this patch so that we lay out the thumbs for the overlay scrollbars while avoiding a reflow of the entire scrollframe contents.
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Assignee: bugmail.mozilla → tnikkel
Comment 15•10 years ago
|
||
> which we're unsure can be safely invoked from outside a reflow
I don't know offhand. :(
In the general case it certainly can't be. But in this specific case, when you know what's in there and know there is no non-box content, it might be ok.
Flags: needinfo?(bzbarsky)
Updated•10 years ago
|
Flags: needinfo?(dbaron)
Comment 16•10 years ago
|
||
I looked at all the things we get in PresShell::FlushPendingNotifications, PresShell::ProcessReflowCommands, and PresShell::DoReflow, the things that might be important are a script blocker and incrementing mChangeNestCount. I also looked at what we do for UpdateOverflow.
Alternatively the only thing we want from LayoutScrollbarsInternal is to set the rect of the scrollbar frames (from/to zero width (or height) to hide/show them). We don't need a full reflow/layout. The reflow of the scrollbars that we request after should take care of reflowing inside the scrollbars.
(We need the reflow of the scrollbars to reflow them with the new attributes they get in UpdateScrollbarAttributes.)
Comment 17•10 years ago
|
||
Comment on attachment 8502150 [details] [diff] [review]
Patch
Since we are already reflowing (wrongly) this patch will work as long as we keep that reflow (but I'd really rather not).
Attachment #8502150 -
Attachment is obsolete: false
Attachment #8502150 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #17)
> Comment on attachment 8502150 [details] [diff] [review]
> Patch
>
> Since we are already reflowing (wrongly) this patch will work as long as we
> keep that reflow (but I'd really rather not).
Agreed. Since we're doing the reflow anyway I'll land this for now and we can uplift to 2.1 and then do the proper fix (which is more risky) as a follow-up.
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8503146 [details] [diff] [review]
Part 1 - Refactoring of existing code
Moved this patch to bug 1081300 which I filed as the follow-up.
Attachment #8503146 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8503147 [details] [diff] [review]
Part 2 - Update scrollbars when the scrollport changes
Ditto. Also moving dbaron's needinfo to the other bug.
Attachment #8503147 -
Attachment is obsolete: true
Flags: needinfo?(dbaron)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8502150 [details] [diff] [review]
Patch
Approval Request Comment
[Feature/regressing bug #]: bug 995519
[User impact if declined]: when pages are loaded in the b2g browser that are not initially scrollable, but then become scrollable by zooming (e.g. cnn.com), the scrollbars do not appear
[Describe test coverage new/current, TBPL]: tested by no-jun on b2g 2.1, tested by me on master
[Risks and why]: pretty small patch that tweaks the condition to make the scrollbars visible. code is shared by desktop platforms but should only affect APZ-enabled platforms (B2G and metro).
[String/UUID change made/needed]: none
Attachment #8502150 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8502150 [details] [diff] [review]
Patch
Backed out for various mochitest and reftest failures. Clearing uplift request too since I'm not sure why the failures are happening.
https://hg.mozilla.org/integration/b2g-inbound/rev/b4457e42fcc9
Attachment #8502150 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•10 years ago
|
||
After looking at the code a bit more I suspect I know what's happening. Try push to verify my theory: https://tbpl.mozilla.org/?tree=Try&rev=cd69b79d491f
Updated•10 years ago
|
Attachment #8502150 -
Flags: qa-approval?(npark)
Comment 25•10 years ago
|
||
No-jun, can you give this patch a push and see if it fixes this bug? it will help justify the uplift. thanks!
Assignee | ||
Comment 26•10 years ago
|
||
Try push was clean. The problem was that mScrollPort may not be set at the point that this runs, so we need to fall back to the locally-calculated scrollPortSize.
@No-Jun: sorry, probably best to wait until this lands before doing anything else.
Assignee: tnikkel → bugmail.mozilla
Attachment #8502150 -
Attachment is obsolete: true
Attachment #8502150 -
Flags: qa-approval?(npark)
Attachment #8503531 -
Flags: review?(tnikkel)
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8503532 [details] [diff] [review]
Patch v2
Seeing as you copied this change into bug 1081300, assuming I can carry the r+, and landed:
https://hg.mozilla.org/integration/b2g-inbound/rev/552283a8c7f1
Attachment #8503532 -
Attachment description: 1-scrollbar-8206269 → Patch v2
Attachment #8503532 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8503531 -
Attachment is obsolete: true
Attachment #8503531 -
Flags: review?(tnikkel)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8503532 [details] [diff] [review]
Patch v2
Approval Request Comment
(see comment 22)
Attachment #8503532 -
Attachment is patch: true
Attachment #8503532 -
Flags: approval-mozilla-aurora?
Comment 30•10 years ago
|
||
Bhavana, can you a+ this? I am working this weekend, so i can try this out. (or if no-jun gets this, even better if you can help verify this before monday)
Flags: needinfo?(npark)
Flags: needinfo?(bbajaj)
Comment 31•10 years ago
|
||
i forgot about a sheriff that would need to uplift. chances are this wont get done until monday anyway, when sheriffs scour the a+ list.
Comment 32•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
Attachment #8503532 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bbajaj)
Assignee | ||
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
The bug still repros on Flame 2.2 and Flame 2.1
Actual result: For Flame 2.2, the scrollbars appear but the horizontal scrollbar disappears when the user scrolls up the page. Scrolling down the page causes the horizontal scrollbar to reappear.
For Flame 2.1, the scrollbars don't appear at all once the page is zoomed in as described in the original report.
Flame 2.2
BuildID: 20141013040202
Gaia: 3b81896f04a02697e615fa5390086bd5ecfed84f
Gecko: f547cf19d104
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Platform Version: 35.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Flame 2.1
BuildID: 20141013001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Platform Version: 34.0a2
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Another bug has already been written for this, bug 1081300.
QA Whiteboard: [failed-verification]
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Chris Kreinbring [:CKreinbring] from comment #34)
> Actual result: For Flame 2.2, the scrollbars appear but the horizontal
> scrollbar disappears when the user scrolls up the page. Scrolling down the
> page causes the horizontal scrollbar to reappear.
This is expected behaviour, because of the way the dynamic toolbar/rocketbar pushes the entire page down.
> For Flame 2.1, the scrollbars don't appear at all once the page is zoomed in
> as described in the original report.
The version of 2.1 you used doesn't have the patch. I only uplifted it this morning but the build you used is from Friday.
Reporter | ||
Comment 36•10 years ago
|
||
I saw this fix landed in master, and made the build locally to test it out. same as when I tested kat's 2nd patch, this looks good, and I don't see any issues anymore with horizontal scroll bar.
Flags: needinfo?(npark)
Reporter | ||
Comment 37•10 years ago
|
||
Marking it to verified since the bug was tested in 2.1 and 2.2 and didn't find any related anomaly.
Status: RESOLVED → VERIFIED
Comment 38•10 years ago
|
||
This issue is verified fixed on Flame 2.1 and 2.2.
Result: Both horizontal and vertical scrollbars appear properly.
Device: Flame 2.2 (319mb, KK, Shallow Flash)
BuildID: 20141110040206
Gaia: 5f8206bab97cdd7b547cc2c8953cadb2a80a7e11
Gecko: d380166816dd
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Device: Flame 2.1 (319mb, KK, Shallow Flash)
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [failed-verification] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•