Last Comment Bug 778810 - Show/Hide scrollbars depending on activity
: Show/Hide scrollbars depending on activity
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
Mentors:
Depends on: 636564 793848 842166
Blocks: 780540 1081072
  Show dependency treegraph
 
Reported: 2012-07-30 11:37 PDT by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2014-10-15 13:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a ScrollbarActivity class (14.86 KB, patch)
2012-07-30 11:41 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
b2g/ part. Show/Hide the scrollbar based on the |active| attribute (1.17 KB, patch)
2012-07-30 11:42 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
fabrice: review+
Details | Diff | Splinter Review
Patch v0.2 (15.23 KB, patch)
2012-08-01 20:00 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
roc: review-
Details | Diff | Splinter Review
Patch v0.3 (15.13 KB, patch)
2012-08-01 20:41 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
roc: review+
Details | Diff | Splinter Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 11:37:57 PDT
For B2G it would be useful to be able to hide the overlaying scrollbars if they are unactive.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 11:41:06 PDT
Created attachment 647233 [details] [diff] [review]
Add a ScrollbarActivity class

This patch is shamelessly stolen from bug 636564 but I have removed all the unneeded parts. It adds a new ScrollbarActivity class that toggle an active flag for scrollbars.
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 11:42:44 PDT
Created attachment 647234 [details] [diff] [review]
b2g/ part. Show/Hide the scrollbar based on the |active| attribute
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-30 16:01:25 PDT
Comment on attachment 647233 [details] [diff] [review]
Add a ScrollbarActivity class

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

::: layout/generic/ScrollbarActivity.h
@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace layout {
> +
> +class ScrollbarActivity : public nsISupports {

This needs a big comment explaining what it's for and how it works.

Also, it can just live in the mozilla namespace.

@@ +33,5 @@
> +  void ActivityEnded();
> +  void Destroy();
> +
> +protected:
> +  PRInt32 mNestedActivityCounter;

This needs to be documented. Also, put the member variables together in the file.

@@ +38,5 @@
> +  bool IsActivityOngoing() {
> +    return mNestedActivityCounter > 0;
> +  }
> +
> +  bool mIsActive;

Same here.

@@ +41,5 @@
> +
> +  bool mIsActive;
> +  void SetIsActive(bool aNewActive);
> +
> +  static const PRUint32 kScrollbarActivityEndedDelay = 450; // milliseconds

This won't compile on some compilers. Use an enum instead.

::: layout/generic/nsGfxScrollFrame.h
@@ +117,5 @@
>    private:
>      nsGfxScrollFrameInner *mInner;
>    };
>  
> +  void FinishReflowForScrollbar(nsIContent* aContent, nscoord aMinX,

Why did this change to aMinX?
Comment 4 [:fabrice] Fabrice Desré 2012-07-31 01:58:30 PDT
Comment on attachment 647234 [details] [diff] [review]
b2g/ part. Show/Hide the scrollbar based on the |active| attribute

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

::: b2g/chrome/content/content.css
@@ +46,3 @@
>  xul|scrollbar[disabled] {
> +  opacity: 0;
> +  -moz-transition: opacity 1s ease;

Nit: maybe 1s is a bit long?
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 20:00:40 PDT
Created attachment 648198 [details] [diff] [review]
Patch v0.2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
 
> ::: layout/generic/nsGfxScrollFrame.h
> @@ +117,5 @@
> >    private:
> >      nsGfxScrollFrameInner *mInner;
> >    };
> >  
> > +  void FinishReflowForScrollbar(nsIContent* aContent, nscoord aMinX,
> 
> Why did this change to aMinX?

That's a typo, sorry.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 20:11:49 PDT
Comment on attachment 648198 [details] [diff] [review]
Patch v0.2

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

::: layout/generic/ScrollbarActivity.cpp
@@ +13,5 @@
> +NS_IMPL_ISUPPORTS0(ScrollbarActivity)
> +
> +void
> +ScrollbarActivity::Destroy() {
> +  CancelActivityFinishedTimer();

{ on new line
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 20:12:52 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 648198 [details] [diff] [review]
> Patch v0.2
> 
> Review of attachment 648198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/ScrollbarActivity.cpp
> @@ +13,5 @@
> > +NS_IMPL_ISUPPORTS0(ScrollbarActivity)
> > +
> > +void
> > +ScrollbarActivity::Destroy() {
> > +  CancelActivityFinishedTimer();
> 
> { on new line

Thanks a lot for the quick review.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 20:14:56 PDT
Comment on attachment 648198 [details] [diff] [review]
Patch v0.2

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

::: layout/generic/ScrollbarActivity.h
@@ +36,5 @@
> + * ActivityOccured() has been made. When the timeout expires ActivityFinished()
> + * is call and reset the active state.
> + */
> +
> +class ScrollbarActivity : public nsISupports {

Actually, why does this have to be refcounted? I think you can and should just make this explicitly managed and give nsGfxScrollFrameInner an nsAutoPtr<ScrollbarActivity>. Instead of Destroy() you can just use the destructor.
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 20:41:22 PDT
Created attachment 648206 [details] [diff] [review]
Patch v0.3
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-03 03:40:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f687cf5c1bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c449b548784e
Comment 12 Leonard Camacho [:lcamacho] 2012-08-13 10:12:40 PDT
With XFCE with Ubuntu I'm seeing a regression in Firefox nightly where bugzilla, gmail and other pages shows a horizontal scrollbar even when the content fit on window.

After use mozregression I get this pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20fc34efd733&tochange=9453cf029b72

It is possible that this bug or another one related is causing this regression?
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-13 22:59:15 PDT
That's bug 781086.

Note You need to log in before you can comment on or make changes to this bug.