Last Comment Bug 777672 - B2G phone ignores |body { overflow: hidden }|
: B2G phone ignores |body { overflow: hidden }|
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 780538
  Show dependency treegraph
 
Reported: 2012-07-26 03:51 PDT by Tim Guan-tin Chien [:timdream] (please needinfo)
Modified: 2012-08-06 08:42 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
b2g/ css changes (4.50 KB, patch)
2012-07-27 12:17 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
fabrice: review+
Details | Diff | Splinter Review
layout/generic/nsGfxScrollFrame.cpp changes (4.71 KB, patch)
2012-07-27 12:21 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.2 (2.17 KB, patch)
2012-07-30 07:06 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.3 (2.13 KB, patch)
2012-08-01 17:31 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
Patch v0.4 (2.12 KB, patch)
2012-08-01 19:07 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
roc: review+
Details | Diff | Splinter Review

Description Tim Guan-tin Chien [:timdream] (please needinfo) 2012-07-26 03:51:31 PDT
Scroll bar of body is always present and overflow content can be scrolled to.

The test case would be the UI Tests app.
Comment 1 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-07-26 03:53:19 PDT
Nominate blocking-basecamp, B2G phone should behavior the same as the web.
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-26 12:07:17 PDT
Is it in the browser application or is it for apps?
Comment 3 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-07-26 12:42:02 PDT
In all apps. Please read Andreas' email on dev-gaia on how we workarounds (back when we were in Paris actually)
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-27 12:17:26 PDT
Created attachment 646666 [details] [diff] [review]
b2g/ css changes
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-27 12:21:03 PDT
Created attachment 646668 [details] [diff] [review]
layout/generic/nsGfxScrollFrame.cpp changes

Roc, I'm not sure this is the better way to fix it or not. It seems that nsGfxScrollableFrame has an optimization to always create scrollbars for top level content. In the case of b2g/ the horizontal scrollbar is floating above the content even if the container has a |overflow: hidden| rule. 

The css rules that seems to be trigger this behavior are: 
xul|scrollbar {
  position: relative;
  margin-top: -8px;
}

Removing one of those seems to fix the behavior but obviously it breaks floating scrollbars :)
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-27 14:12:34 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #5)
> Roc, I'm not sure this is the better way to fix it or not. It seems that
> nsGfxScrollableFrame has an optimization to always create scrollbars for top
> level content.

It's not an optimization. It's necessary for correctness. We create the scrollbar frames but they should remain hidden when not needed. The question is, why are they showing up as visible on B2G?

Does setting html { overflow:hidden; } work as intended?

body{overflow:hidden} should cause nsCSSFrameConstructor::PropagateScrollToViewport to call nsPresContext::SetViewportOverflowOverride with NS_STYLE_OVERFLOW_HIDDEN, which should then be picked up by GetScrollbarStyles in nsGfxScrollFrame, which should cause the scrollbars to be collapsed by LayoutAndInvalidate being called with a zero-width or zero-height rect.

So something's going wrong during that process.
Comment 7 [:fabrice] Fabrice Desré 2012-07-27 15:01:57 PDT
Comment on attachment 646666 [details] [diff] [review]
b2g/ css changes

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

::: dom/browser-element/BrowserElementChild.js
@@ +20,5 @@
>    Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN // Volume down.
>  ];
>  
>  function debug(msg) {
> +  dump("BrowserElementChild - " + msg + "\n");

Nit: comment before landing

::: dom/browser-element/BrowserElementScrolling.js
@@ +117,5 @@
> +
> +let canHaveHorizontal = {};
> +let canHaveVertical = {};
> +scrollable.getScrollbarVisibility(canHaveHorizontal, canHaveVertical);
> +debug('horizontal: ' + canHaveHorizontal.value + ' vertical: ' + canHaveVertical.value);

If you want to keep this debug code, indent it properly.
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-27 15:43:31 PDT
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 646666 [details] [diff] [review]
> b2g/ css changes
> 
> Review of attachment 646666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChild.js
> @@ +20,5 @@
> >    Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN // Volume down.
> >  ];
> >  
> >  function debug(msg) {
> > +  dump("BrowserElementChild - " + msg + "\n");
> 
> Nit: comment before landing
> 
> ::: dom/browser-element/BrowserElementScrolling.js
> @@ +117,5 @@
> > +
> > +let canHaveHorizontal = {};
> > +let canHaveVertical = {};
> > +scrollable.getScrollbarVisibility(canHaveHorizontal, canHaveVertical);
> > +debug('horizontal: ' + canHaveHorizontal.value + ' vertical: ' + canHaveVertical.value);
> 
> If you want to keep this debug code, indent it properly.

Obviously this debug code will be removed. I |hg qrefresh| it without paying attention.
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 07:04:55 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Vivien Nicolas (:vingtetun) from comment #5)
> > Roc, I'm not sure this is the better way to fix it or not. It seems that
> > nsGfxScrollableFrame has an optimization to always create scrollbars for top
> > level content.
> 
> It's not an optimization. It's necessary for correctness. We create the
> scrollbar frames but they should remain hidden when not needed. The question
> is, why are they showing up as visible on B2G?
> 
> Does setting html { overflow:hidden; } work as intended?

It does not. The horizontal scrollbar is still visible.
> 
> body{overflow:hidden} should cause
> nsCSSFrameConstructor::PropagateScrollToViewport to call
> nsPresContext::SetViewportOverflowOverride with NS_STYLE_OVERFLOW_HIDDEN,
> which should then be picked up by GetScrollbarStyles in nsGfxScrollFrame,
> which should cause the scrollbars to be collapsed by LayoutAndInvalidate
> being called with a zero-width or zero-height rect.
> 
> So something's going wrong during that process.

Thanks for all those details. I have made a new patch that ignore margin in the rect passed to LayoutAndInvalidate if the scrollbar is not supposed to be here.
Comment 10 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-30 07:06:33 PDT
Created attachment 647147 [details] [diff] [review]
Patch v0.2

This version do not add margin to the rect sent to LayoutAndInvalidate if the scrollbars are hidden.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-30 16:13:17 PDT
Comment on attachment 647147 [details] [diff] [review]
Patch v0.2

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3749,5 @@
>      NS_PRECONDITION(mVScrollbarBox->IsBoxFrame(), "Must be a box frame!");
>      nsRect vRect(mScrollPort);
>      vRect.width = aContentArea.width - mScrollPort.width;
>      vRect.x = scrollbarOnLeft ? aContentArea.x : mScrollPort.XMost();
> +    if (styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) {

I think you should just test mHasVerticalScrollbar here.
Comment 12 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 17:31:05 PDT
Created attachment 648161 [details] [diff] [review]
Patch v0.3
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-01 17:33:53 PDT
Comment on attachment 648161 [details] [diff] [review]
Patch v0.3

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3753,5 @@
>      NS_PRECONDITION(mVScrollbarBox->IsBoxFrame(), "Must be a box frame!");
>      nsRect vRect(mScrollPort);
>      vRect.width = aContentArea.width - mScrollPort.width;
>      vRect.x = scrollbarOnLeft ? aContentArea.x : mScrollPort.XMost();
> +    if (!mNeverHasVerticalScrollbar) {

No, I really meant "mHasVerticalScrollbar"!
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 19:07:43 PDT
Created attachment 648182 [details] [diff] [review]
Patch v0.4

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 648161 [details] [diff] [review]
> Patch v0.3
> 
> Review of attachment 648161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +3753,5 @@
> >      NS_PRECONDITION(mVScrollbarBox->IsBoxFrame(), "Must be a box frame!");
> >      nsRect vRect(mScrollPort);
> >      vRect.width = aContentArea.width - mScrollPort.width;
> >      vRect.x = scrollbarOnLeft ? aContentArea.x : mScrollPort.XMost();
> > +    if (!mNeverHasVerticalScrollbar) {
> 
> No, I really meant "mHasVerticalScrollbar"!

Sorry about that, I should have read your comment more closely.:/
Comment 15 David Baron :dbaron: ⌚️UTC-10 2012-08-01 20:02:54 PDT
Comment on attachment 648182 [details] [diff] [review]
Patch v0.4

>+  ScrollbarStyles styles = GetScrollbarStylesFromFrame();

This line is unused, and should be removed unless I'm missing something.
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-01 20:45:10 PDT
(In reply to David Baron [:dbaron] from comment #15)
> Comment on attachment 648182 [details] [diff] [review]
> Patch v0.4
> 
> >+  ScrollbarStyles styles = GetScrollbarStylesFromFrame();
> 
> This line is unused, and should be removed unless I'm missing something.

That's correct. It comes from an old version of this patch. Thanks for catching it. I will removed it before checkin.
Comment 17 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-03 03:39:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a13303220dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a741513132bd
Comment 18 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-03 03:59:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8bd0de9362

Sigh, I pushed changes to dom/browser-element/ so I have reverted them. Next time I will post updated patches on the corresponding bug instead of having the changes living somewhere on my hard drive.

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