902937 - Move startui scrolling from 'start-scrollbox' to the parent browser

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

[Jim Mathies [:jimm]]

For APZC to work, we need to scroll using the parent browser, or figure out some way to call setDisplayPortForElement on start-scrollbox. Moving to the browser would be best.

Comment 1

[Jim Mathies [:jimm]]

Attachment: trial

I'm having zero luck getting scrollbars to show up on this content, even if I embed another browser inside the tab. I'm going to look for other solutions.

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip

Pretty close, only problem left is with grid vbox height.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: browser scroll for start tab v.1

Comment 6

[Jim Mathies [:jimm]]

Attachment: scroll event support

Comment 7

[Jim Mathies [:jimm]]

Seeing some weird scrollto values here with apz enabled. The scrollTo.y value is always -34. 

MetroWidget::SendAsyncScrollDOMEvent
MetroWidget::SendAsyncScrollDOMEvent
MetroWidget::RequestContentRepaint
APZC scrollId: 6 
APZC scrollTo.x: 302, scrollTo.y: -34 
APZC setResolution: 1 
APZC setDisplayPortForElement: displayPort.x: -302, displayPort.y: 0, displayPort.width: 2042, displayort.height: 774 

bbondy, any idea here? The browser deck has a padding on it that's about 34 px, so the bottom of the actual browser isn't aligned with the window bounds. Wondering if that might throwing something off.

(fwiw, scrolling of the start page looks freaking great aside from this anomaly.)

Comment 8

[Jim Mathies [:jimm]]

Actually the padding is equal to toolbar_height which is 68px, so that maybe not be related.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #8)
> Actually the padding is equal to toolbar_height which is 68px, so that maybe
> not be related.

(although 34 is .5 * 68!)

Comment 10

[Jim Mathies [:jimm]]

and if I get rid of the padding, the problem goes away.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 788928 [details] [diff] [review]
browser scroll for start tab v.1

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

::: browser/metro/base/content/startui/StartUI.js
@@ +7,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +// When setting the max-height of the start tab contents, this is the buffer we subtract
> +// for the nav bar plus white space above it.
> +const kBottomContentMargin = 50;

Can we use a margin or padding in CSS rather than doing this in JS?  Something like:

  max-height: calc(100% - 50px);

I'd also prefer to avoid a magic number here and use something based on the navbar height, but I can see how that might be too complex to be worth it... 

This is fine with me if it's the simplest solution.

::: browser/metro/theme/browser.css
@@ +193,5 @@
> +}
> +
> +#startui-body {
> +  height: 100%;
> +}

Not directly related to this bug, but we should try to split out start page styles into a new "start.css" file, and have Start.xul include that instead of browser.css.

Comment 12

[Brian R. Bondy [:bbondy]]

Hey Jim sorry I don't know why, but I'm forwarding the needinfo in Comment 7 to kats

Comment 13

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> Hey Jim sorry I don't know why, but I'm forwarding the needinfo in Comment 7
> to kats

Kats, this is an interesting side effect to *not* having the browser fill the main view we are panning using apz. If we add padding below or above it, apz gets a little confuses. Any pointers on where to look would be appreciated.

I'm going to land this work which is unrelated aside from adding the padding, and file a new bug on the sideways pan problem. will cc you both in.

Comment 14

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/d203f93cda4c
https://hg.mozilla.org/integration/fx-team/rev/226bb0af00c0

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Sorry, I don't fully understand what the problem is. I looked at your patches but without running them I don't have a good intuition of what effect they have. Is the scroll offset you were seeing always stuck at -34? Or is it 34 off from what it's supposed to be?

Regardless it would be good to try the patches on bug 890280 to see if that makes any difference. I will also be rebuilding the updated metro code today since I need to test some changes there so I'll try to look at this as well.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Sorry, I don't fully understand what the problem is. I looked at your
> patches but without running them I don't have a good intuition of what
> effect they have. Is the scroll offset you were seeing always stuck at -34?
> Or is it 34 off from what it's supposed to be?


We have a stack that contains all browsers - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#197

when the start tab is visible we add a 68px bottom-padding to it. With apz enabled, scrolling sideways results in a constant -34 for scrollTo.y here -

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/apzc.js#97

The two are related since removing this padding addresses the problem.

> Regardless it would be good to try the patches on bug 890280 to see if that
> makes any difference. I will also be rebuilding the updated metro code today
> since I need to test some changes there so I'll try to look at this as well.

Will do, thanks.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d203f93cda4c
https://hg.mozilla.org/mozilla-central/rev/226bb0af00c0