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

RESOLVED FIXED in Firefox 26

Status

Firefox for Metro
Firefox Start
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 26
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 788120 [details] [diff] [review]
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.
Assignee: nobody → jmathies
(Assignee)

Comment 2

4 years ago
Created attachment 788313 [details] [diff] [review]
wip

Pretty close, only problem left is with grid vbox height.
Attachment #788120 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Created attachment 788920 [details] [diff] [review]
patch v.1
Attachment #788313 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 788922 [details] [diff] [review]
patch v.1
Attachment #788920 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Created attachment 788928 [details] [diff] [review]
browser scroll for start tab v.1
Attachment #788922 - Attachment is obsolete: true
Attachment #788928 - Flags: review?(mbrubeck)
(Assignee)

Comment 6

4 years ago
Created attachment 788943 [details] [diff] [review]
scroll event support
(Assignee)

Updated

4 years ago
Attachment #788943 - Flags: review?(mbrubeck)
(Assignee)

Comment 7

4 years ago
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)
(Assignee)

Comment 8

4 years ago
Actually the padding is equal to toolbar_height which is 68px, so that maybe not be related.
(Assignee)

Comment 9

4 years ago
(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!)
(Assignee)

Comment 10

4 years ago
and if I get rid of the padding, the problem goes away.
(Assignee)

Updated

4 years ago
Blocks: 876042
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/d203f93cda4c
https://hg.mozilla.org/integration/fx-team/rev/226bb0af00c0
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)
(Assignee)

Comment 16

4 years ago
(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
Last Resolved: 4 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.