Scrollbars are not drawn on b2g

RESOLVED FIXED in Firefox 28, Firefox OS v1.2

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

({regression})

unspecified
1.3 Sprint 5 - 11/22
regression

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 fixed, b2g-v1.2 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 754868 [details] [diff] [review]
Patch

It seems like the prefs for it has changed.
Attachment #754868 - Flags: review?(poirot.alex)
Comment on attachment 754868 [details] [diff] [review]
Patch

Review of attachment 754868 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should still keep `ui.showHideScrollbars` as there is still one usage of it:
  http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.cpp#344
Attachment #754868 - Flags: review?(poirot.alex) → review+
This stuff is sitting in my patch queue for a long time. It feels like turning it on again slow down panning because the scrollbar is repainted way too often.

Milan do you know who can have a look at it?

Steps to reproduce:
 - add pref("ui.showOverlayScrollbars", 1); in b2g/app/b2g.js
 - On the device go to Settings -> Informations -> More Informations -> Developer -> Show Repaints
 - Pan a little bit and observe that the scrollbar is repainted on every moves.
Flags: needinfo?(milan)
Seems like the perf issues are resolved with bug 813041
Depends on: 813041
Vivien, is the question still the same, after the comment 3?
(In reply to Milan Sreckovic [:milan] from comment #4)
> Vivien, is the question still the same, after the comment 3?

Nope. Sorry I forgot to remove the needinfo.
Flags: needinfo?(milan)

Updated

4 years ago
Duplicate of this bug: 917428
Talked with rel man about this - given that bug 813041 has existed on 1.01 already, we can ship another release with it present again. However, given that scrollbars have always been present across apps in 1.01 & 1.1, we need to turn them back on again, as it's a regression otherwise to have them off. On that regard, I'm pulling the dependency on bug 813041, as we do not need to wait on bug 813041 landing to turn the prefs back on. I'm also noming this since we need this back on in the 1.2 timeframe.
blocking-b2g: --- → koi?
No longer depends on: 813041
Keywords: regression
We need bug 813041 because the perfs sucks without the patch there. Thanks for keeping the dependency which is one of the reason why I have not landed this patch.
Depends on: 813041
Vivien based on conversation with :jason

a) We need to enable scrollbars in 1.2, since we have had them since 1.0.1 and we would definitely need it for various use-cases (I think we all agree here ?)
b) Your concern is that perf is impacted and is further worse in 1.2 if we enabled them(Correct ?). that's being tacked in 813041
c) Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=813041#c13 the rebased patch works well. Is it just review away ? What's the risk there ?

We would need to block on both if you confirm that the perf is worse and the upcoming patch helping the perf is low risk.Can you confirm ?
(In reply to bhavana bajaj [:bajaj] from comment #9)
> Vivien based on conversation with :jason
> 
> a) We need to enable scrollbars in 1.2, since we have had them since 1.0.1
> and we would definitely need it for various use-cases (I think we all agree
> here ?)

Yep.

> b) Your concern is that perf is impacted and is further worse in 1.2 if we
> enabled them(Correct ?). that's being tacked in 813041

Yes. 

> c) Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=813041#c13 the
> rebased patch works well. Is it just review away ? What's the risk there ?
> 

I can not really tell for the risk for bug 813041. You may want to ask directly there, sorry :/
koi+ for regression.
blocking-b2g: koi? → koi+
(In reply to Preeti Raghunath(:Preeti) from comment #11)
> koi+ for regression.

This is tagged as depending on bug 813041 - if that is the case, that bug should be koi+ as well.
Sounds like this is in 1.2, so removing the "mozilla-central" from the summary to avoid confusion.
Summary: Scrollbars are not draw onb2g with mozilla-central → Scrollbars are not drawn on b2g
Created attachment 830875 [details] [diff] [review]
bug876741.patch

Forwarding the r+ from ochameau after his comment.
Assignee: nobody → 21
Attachment #754868 - Attachment is obsolete: true
Attachment #830875 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/276e2868ea2a
Keywords: checkin-needed
Backed out for B2G reftest failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/0db9bc1e8e39

https://tbpl.mozilla.org/php/getParsedLog.php?id=30482967&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30483514&tree=B2g-Inbound
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #16)
> Backed out for B2G reftest failures.
> https://hg.mozilla.org/integration/b2g-inbound/rev/0db9bc1e8e39
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30482967&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30483514&tree=B2g-Inbound

Sigh. Those are purely random since scrollbars appears automatically during reflows and dissapears after a while. So the image comparison depends on when the screenshot is done :(
I've added some random-if(B2G) and push to try to see if some other tests fails because of this randomess thing. https://tbpl.mozilla.org/?tree=Try&rev=27b8e930b0da
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18)
> I've added some random-if(B2G) and push to try to see if some other tests
> fails because of this randomess thing.
> https://tbpl.mozilla.org/?tree=Try&rev=27b8e930b0da

humm. The more try tests I'm running the more random I see. I feel like this stuff may introduce a lot of random oranges just because the scrollbars are shown when there is a reflow and fades out with a timeout, that obviously is hard to get identical between the page and the reference page. :/

Not sure what's the best solution here. I more or less considering to disable this prefs during reftests (if doable).
Maybe my best chance is to fix those randoms by running tons of try builds and once there are green multiple times then this is good. :(
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #20)
> Maybe my best chance is to fix those randoms by running tons of try builds
> and once there are green multiple times then this is good. :(

This run is Green https://tbpl.mozilla.org/?tree=Try&rev=ba7b77619dd9

I will re-run just to make sure to catch more random oranges.
https://tbpl.mozilla.org/?tree=Try&rev=c319dd076cce

Green again. Let's see if Roc is fine with the patch.
Created attachment 8335489 [details] [diff] [review]
bug876741.patch
Attachment #830875 - Attachment is obsolete: true
Attachment #8335489 - Flags: review?(roc)
Attachment #8335489 - Flags: review?(roc) → review+
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> Sigh. Those are purely random since scrollbars appears automatically during
> reflows and dissapears after a while.

Really, scrollbars appear just because we did a reflow? I thought user interaction was required to trigger scrollbars on B2G. Is scrollbars appearing because we did a reflow desired behavior for some reason?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> > Sigh. Those are purely random since scrollbars appears automatically during
> > reflows and dissapears after a while.
> 
> Really, scrollbars appear just because we did a reflow? I thought user
> interaction was required to trigger scrollbars on B2G. Is scrollbars
> appearing because we did a reflow desired behavior for some reason?

I think this is desireable from a UX point of view. It allows scrollbars to be displayed when a scrollable area is just created on the screen, letting the user knows that he/she can pan.

At least this has been the rationale until today. I will land this patch and check with UX if this is something they want to change or not.
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d8b156a140
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → 1.3 Sprint 5 - 11/22
https://hg.mozilla.org/mozilla-central/rev/20d8b156a140
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/28953b2f02ba
status-b2g-v1.2: --- → fixed
status-firefox26: --- → wontfix
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
You need to log in before you can comment on or make changes to this bug.