The default bug view has changed. See this FAQ.

B2G phone ignores |body { overflow: hidden }|

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: timdream, Assigned: vingtetun)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Scroll bar of body is always present and overflow content can be scrolled to.

The test case would be the UI Tests app.
Nominate blocking-basecamp, B2G phone should behavior the same as the web.
blocking-basecamp: --- → ?
Is it in the browser application or is it for apps?
In all apps. Please read Andreas' email on dev-gaia on how we workarounds (back when we were in Paris actually)
Created attachment 646666 [details] [diff] [review]
b2g/ css changes
Attachment #646666 - Flags: review?(fabrice)
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 :)
Attachment #646668 - Flags: review?(roc)
(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 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.
Attachment #646666 - Flags: review?(fabrice) → review+
(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.
(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.
Attachment #646668 - Flags: review?(roc)
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.
Attachment #646668 - Attachment is obsolete: true
Attachment #647147 - Flags: review?(roc)
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.
Created attachment 648161 [details] [diff] [review]
Patch v0.3
Attachment #647147 - Attachment is obsolete: true
Attachment #647147 - Flags: review?(roc)
Attachment #648161 - Flags: review?(roc)
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"!
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.:/
Assignee: nobody → 21
Attachment #648161 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #648161 - Flags: review?(roc)
Attachment #648182 - Flags: review?(roc)
Attachment #648182 - Flags: review?(roc) → review+
Component: General → Layout
Product: Boot2Gecko → Core
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.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a13303220dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/a741513132bd
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla17
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.
https://hg.mozilla.org/mozilla-central/rev/6a13303220dd
https://hg.mozilla.org/mozilla-central/rev/a741513132bd
https://hg.mozilla.org/mozilla-central/rev/2d8bd0de9362
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 780538
blocking-basecamp: ? → +
You need to log in before you can comment on or make changes to this bug.