Closed Bug 902937 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch trial (obsolete) — Splinter Review
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.
Assignee: nobody → jmathies
Attached patch wip (obsolete) — Splinter Review
Pretty close, only problem left is with grid vbox height.
Attachment #788120 - Attachment is obsolete: true
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #788313 - Attachment is obsolete: true
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #788920 - Attachment is obsolete: true
Attachment #788922 - Attachment is obsolete: true
Attachment #788928 - Flags: review?(mbrubeck)
Attachment #788943 - Flags: review?(mbrubeck)
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.)
Flags: needinfo?(netzen)
Actually the padding is equal to toolbar_height which is 68px, so that maybe not be related.
(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!)
and if I get rid of the padding, the problem goes away.
Blocks: 876042
No longer blocks: 876042
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.
Attachment #788928 - Flags: review?(mbrubeck) → review+
Attachment #788943 - Flags: review?(mbrubeck) → review+
Hey Jim sorry I don't know why, but I'm forwarding the needinfo in Comment 7 to kats
Flags: needinfo?(netzen) → needinfo?(bugmail.mozilla)
(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.
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.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/d203f93cda4c
https://hg.mozilla.org/mozilla-central/rev/226bb0af00c0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.