Closed Bug 1078373 Opened 5 years ago Closed 5 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)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: njpark, Assigned: kats)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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
No longer depends on: 1078316
this also happens in http://www.mozilla.org site
[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?
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
Attached patch Patch (obsolete) — Splinter Review
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)
blocking-b2g: 2.1? → 2.1+
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 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)
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+
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 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?
(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
(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.
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: bugmail.mozilla → tnikkel
> 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)
Flags: needinfo?(dbaron)
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 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+
(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.
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
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)
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?
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?
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
Attachment #8502150 - Flags: qa-approval?(npark)
No-jun, can you give this patch a push and see if it fixes this bug?   it will help justify the uplift.  thanks!
Attached patch Patch v2 (obsolete) — Splinter Review
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)
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+
Attachment #8503531 - Attachment is obsolete: true
Attachment #8503531 - Flags: review?(tnikkel)
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?
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)
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.
https://hg.mozilla.org/mozilla-central/rev/552283a8c7f1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Attachment #8503532 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bbajaj)
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]
(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.
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)
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
Depends on: 1084350
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)
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.