The default bug view has changed. See this FAQ.

Show/Hide scrollbars depending on activity

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

For B2G it would be useful to be able to hide the overlaying scrollbars if they are unactive.
Depends on: 636564
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.
Attachment #647233 - Flags: review?(roc)
Created attachment 647234 [details] [diff] [review]
b2g/ part. Show/Hide the scrollbar based on the |active| attribute
Attachment #647234 - Flags: review?(fabrice)
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 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?
Attachment #647234 - Flags: review?(fabrice) → review+
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.
Attachment #647233 - Attachment is obsolete: true
Attachment #647233 - Flags: review?(roc)
Attachment #648198 - Flags: review?(roc)
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
Attachment #648198 - Flags: review?(roc) → review+
(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.
Status: NEW → ASSIGNED
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.
Attachment #648198 - Flags: review+ → review-
Created attachment 648206 [details] [diff] [review]
Patch v0.3
Assignee: nobody → 21
Attachment #648198 - Attachment is obsolete: true
Attachment #648206 - Flags: review?(roc)
Attachment #648206 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f687cf5c1bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c449b548784e
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/f687cf5c1bb5
https://hg.mozilla.org/mozilla-central/rev/c449b548784e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 780540
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?
That's bug 781086.

Updated

5 years ago
Depends on: 793848

Updated

4 years ago
Depends on: 842166
Blocks: 1081072
You need to log in before you can comment on or make changes to this bug.