461843 - Show indication of where on the page you are when scrolling

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Madhava Enros [:madhava]]

I'd like to have small scroll-indicators that are only present when a user is scrolling.  These wouldn't actually be scroll-handles -- the user moves by panning the whole page directly -- but would give a sense of where on the page the user is.  Something along these lines, from this old m1 fennec UI idea:

https://wiki.mozilla.org/Mobile/UI/Designs/TouchScreen/Proposal6#Overview

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch displays floating scroll position indicators.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

> Created an attachment (id=347471) [details]

If the scrolling / panning is not required because the content fits into the given viewport, the scroll indicator for that direction is not shown.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

We now pan in content _and_ in chrome (lists and stuff). I think the first priority here is indicators for scrolling in content.

Comment 5

[Madhava Enros [:madhava]]

I agree - much easier to get lost in a non-structured area, which content can more easily be, than in a list.  Nice to have for both, but much more of a priority for content.

Comment 6

[Ben Combee (:bcombee)]

The earlier implementation must be redone a bit for tile work.  I'll push this until after we see how Stuart handles the sidebars.

Comment 7

[Ben Combee (:bcombee)]

Talked with mfinkle, waiting on tilecache to land to see how to overlay this UI now.

Comment 8

[TuringT]

Actually I'd like a full scrollbar available that could be used for absolute positioning. Inertial scrolling is all well and good, but it sucks for really long
webpages/documents - in special for low CPU devices such as the Nokia
N800 series.

To alleviate this problem, you could provide a scrollbar hidden at
the right side. To save screen space, this scrollbar could be 
outside the right edge of the screen, just like the command panels are now.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Updated patch. Could be more efficient with the getBoundingClientRect() and the indicators display over chrome and content.

See:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-scroll-indicators.png
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-scroll-indicators-2.png

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

Do as little as possible while dragging. Move more of the static code to dragStart

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Patch 3 is much better, performance-wise, on the n810. But still not as good as without any indicator. Not sure we will be able to get that good.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-1

A little dirty wip which shows indicators for content and xul scrollboxes (bug 515166) with the same mechanism for both.

I have not test it on my n810 for now so i'm a bit unsure of the perfs issue.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 410548 [details] [diff] [review]
wip-1

>diff -r 244fcb70db93 chrome/content/bindings.xml
>-      <xul:vbox class="autocomplete-box" flex="1">
>+      <xul:vbox pannable="true" class="autocomplete-box" flex="1">

Let's flip this so that all scrollboxes are pannable by default and you need to disable it explicitly

>+  <binding id="scrollbox" extends="chrome://global/content/bindings/scrollbox.xml#scrollbox">
>+      <xul:stack xbl:inherits="flex">

Putting stuff all content another stack might be a performance issue

>+        <xul:scrollbox anonid="scrollbox" xbl:inherits="orient,align,pack,dir" flex="1">
>+          <children />
>+        </xul:scrollbox>

Kinda weirdly recursive and will be tough to do if we drop "pannable" attribute you use to do the -moz-binding

>+      <field name="_pannableW">0</field>
>+      <field name="_pannableH">0</field>
>+
>+      <field name="_containerW">0</field>
>+      <field name="_containerH">0</field>
>+      <field name="_containerL">0</field>

This 5 fields are only used in _updateScrollers. Might not need to be fields. Just make them local variables, if needed at all.

>+              // XXX dirty
>+              let [leftvis,rightvis] = Browser.computeSidebarVisibility();
>+              if (leftvis > 0.02 || rightvis > 0.02 || (rect.width == w.value))
>+                this._vscroller.hidden = true;
>+            }

What's this about? I don't like this kind of tight coupling.


>+      <field name="_height">0</field>
>+      <field name="_width">0</field>

Not needed?

>+      <property name="_hidden" onset="this._hscroller.hidden = this._vscroller.hidden = val;" onget="return this._hscroller.hidden"/>

Not needed? and I don't like properties with leading underscores - that's the mark of a private field, which 'hidden' shouldn't be. And 'hidden' is already a property consider using a function, isHidden() ?

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-2

This wip did not address comments, I've just do a bit of refactoring mostly for performances purposes.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-3 - better perfs

Ok, this wip has better perfs. (tested on a n810)

XPConnect is one of our caveats here, so on my local builds I'm quickstubing 'nsIDOMXULElement.top',
'nsIDOMXULElement.left',
'nsIDOMNSElement.scrollTop',
'nsIDOMNSElement.scrollLeft',

but I'm not really sure of the downside of this approach.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-4

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Ben, I've tried to look at which area is repainted, it looks like we don't repaint the entire frame:


With scrollers:
-1210718512[b7b21320]: sending expose event [b1fb6b00] b1fd9760 0x2c000b3 (rects follow):
-1210718512[b7b21320]: 	791 125 7 188
#repaint of the vertical scroller
-1210718512[b7b21320]: Invalidate (rect) [b1fb6b00]: 791 120 7 193 (sync: 0)
#repaint of the window
-1210718512[b7b21320]: Invalidate (rect) [b1fb6b00]: 0 492 800 8 (sync: 0)
-1210718512[b7b21320]: Update [b1fb6b00] b1fd9760

Without scrollers:
-1210935600[b7b21320]: sending expose event [b20b6b00] b20d9760 0x2c000b3 (rects follow):
-1210935600[b7b21320]: 	0 0 800 500
-1210935600[b7b21320]: Update [b18f8d00] b20d0760
-1210935600[b7b21320]: Invalidate (rect) [b20b6b00]: 0 470 800 30 (sync: 0)

Comment 19

[Fabrice Desré]

Attachment: new try

New try using two canvases to draw the scrollbars. Hopefully fast enough on a device.

Comment 20

[Fabrice Desré]

Attachment: new try v2

Better look and feel using a solid stroke for the rectangles.

Comment 21

[Fabrice Desré]

Attachment: new try v3

Much better version, also using two canvases, one for each scrollbar. Each canvas takes the full scrollbar space, and we draw as needed when panning. This means that there are no DOM or style changes during panning.

Comment 22

[Fabrice Desré]

Attachment: screenshot

Comment 23

[Madhava Enros [:madhava]]

I like the blue, but I think a grey indicator would more reliably not clash given that the page beneath could be any color.

Comment 24

[Fabrice Desré]

I did tests also with grey, and I'm not sure it's better than blue. The blue is usually more visible.

Comment 25

[Tim Samoff]

(In reply to comment #24)
> I did tests also with grey, and I'm not sure it's better than blue. The blue is
> usually more visible.

Imho, the scrollbar color should be a theming option, not a default value. In Maemo, the scrollbars are gray by default, but (afaik) they can bet set to different colors using the ThemeMaker app [1].

[1] http://konttoristhoughts.blogspot.com/2009/12/theme-maker-125-out-fremantle-beta.html

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

I agree the these should be themed, but so should the menu/homescreen button and the close button and they are not going to be for 1.0.  Grey would be the most consistent with our theme.

However, its very possible that any color you pick would not show up against the background of some site, so may I suggest putting a border around the box with some other shade of grey?  I'd assume that at least one of the two shades of grey will show up.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

I was using a white border in one of the early patches.

Comment 28

[Fabrice Desré]

So who is authoritative on this? It's a no brainer to update the patch with another color scheme once we take a decision.

Comment 29

[Tim Samoff]

(In reply to comment #28)
> So who is authoritative on this? It's a no brainer to update the patch with
> another color scheme once we take a decision.

That depends. Do we want the official Maemo recommendation, or a general "mobile" one?

If Maemo, I can get one of the UI guys to respond.

Comment 30

[Fabrice Desré]

I found this in the "Fremantle Master Layout Guide v1.1" :

"The thickness of the panning indicator is 8px, positioned in the centre of a 16px area
(HILDON_MARGIN_HALF at both sides), and coloured as SecondaryTextColor. This applies to both vertical
and horizontal scroll indicators."

We can at least support the placement, if not the color of the current theme.

Comment 31

[Fabrice Desré]

Attachment: new patch

New version, with a grey color theme and a 4px margin to follow Hildon guidelines. I checked MicroB and they have a very thin grey indicator (they don't strictly follow the guidelines).

Comment 32

[Fabrice Desré]

Attachment: new screenshot with grey indicators

Comment 33

[Tuomas Kuosmanen]

Hello :-)

The content displayed inside the scrolled area (the web page) can vary wildly in style and color, thus it would in my opinion make no sense to mix in the "system theme" colors, unless you only visit web sites that use the same color scheme :-)

I would keep the indicators neutral - gray fits with any other color.

The example screenshot also has has good visibility on different background lightness, because of the light outline of the dark area.

Comment 34

[Fabrice Desré]

Attachment: updated patch

Updated version to show scroll indicators in HTML scrollable divs and xul-in-content scrollable elements.

Comment 35

[Fabrice Desré]

Attachment: scroll indicator for pannable <div>s

Comment 36

[Fabrice Desré]

Attachment: scroll indicator for xul-in-content

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip full css

This patch use the native scrollbar and just use some css to skin them to look better. Basically they are looking like Fabrice's screenshot.

This patch works for both this patch and bug 515166. I need to clean it up a bit though.

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have an updated patch for that who handle scrollable div and iframes. I'm still working on support for nested iframes :/

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip-2

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch add scroll indicators for both chrome list and web content.
It also show scroll indicators on pannable divs but not for iframe for the moment.

A big part of the patch is some code cleanup and some css styling. I think it would be nice if we can land a basic support and handle the css colors for the indicators and the nested iframes indicators in some followups.

Basically the iframes indicators support is working, but because of some rendering bug on the main content, i've been obliged to removed them.

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 427357 [details] [diff] [review]
Patch

I'm canceling review because I've just found a little UI regression with this patch.

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

There is a perf regression with the scrollbars on desktop but not so huge. I need to gather numbers from devices

The command use is xresponse -m 800x500+1+77 -d 180x560,180x100 -d 180x560,180x100 -d 180x160,180x560 -d 180x160,180x560 -d 180x160,180x560 with prefetching code disabled.

My testing website is: http://dsl.org/cookbook/cookbook_6.html

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Data

Results given by xresponse

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #43)
> Created an attachment (id=428456) [details]
> Data
> 
> Results given by xresponse

on N900?

Comment 45

[Mark Finkle (:mfinkle) (use needinfo?)]

Vivien's data test 5 different scroll/pan actions and repeats the tests 3 times. This is done for 3 cases: No scroll indicators, Scroll indicators without transparency and Scroll indicators with transparency.

The rolled up averages (in ms):

No Scroll     Scroll w/o trans    Scroll w/ trans
---------     ----------------    ---------------
1368          1421                1381
1302          1378                1343
1041          1125                1098
1077          1127                1121
439           489                 432

Based on this data, I'd say we are OK to land this patch. But I'd like to know for sure that the data is from a device and not desktop.

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #45)
> Vivien's data test 5 different scroll/pan actions and repeats the tests 3
> times. This is done for 3 cases: No scroll indicators, Scroll indicators
> without transparency and Scroll indicators with transparency.
> 
> The rolled up averages (in ms):
> 
> No Scroll     Scroll w/o trans    Scroll w/ trans
> ---------     ----------------    ---------------
> 1368          1421                1381
> 1302          1378                1343
> 1041          1125                1098
> 1077          1127                1121
> 439           489                 432
> 
> Based on this data, I'd say we are OK to land this patch. But I'd like to know
> for sure that the data is from a device and not desktop.

Sorry, I've been unclear, these data came from desktop :/

I hope (hardly) to have something similar on device, but I should wait tomorrow to be able to have it.

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: data for device

Scroll performance data for the device, it looks like we're losing ~8% of performance.
 (I've ignored and replace 4281ms by 1450ms for the calculation because the n900 was going into "sleep")

Comment 48

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: data for non-transparent scrollbars

Weird, this is slower and the median is higher

Comment 49

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

This patch has been tested with performance impact on kinetic scrolling mostly. As suggested by mfinkle during a call, i've removed the indicators during the kinetic scroll to avoid the 6 to 8% of performance loss.

A few bugs has been corrected into this patch. I will try to provide a clean way to test for performance on device tomorrow since mine isn't working as expected (the device).

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

I think this is what we want. Same as v0.2 with a little change for perf reason.

I'm testing the normal panning case (when a user is exploring the web page for an information) with fennecmark (patch for testing coming).

Comment 51

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch fennecmark for perf testing

fennecmark patch, there is a little trick with the 10ms delay because perf are really worse without it, but i really think that a user can go as fast for moving his finger on the screen!

This patch apply on top of fennecmark (for testing purpose only) and replace the current pan case done in fennecmark which assume "mousedown-mouvemove in one direction-mouseup", "mousedown-mouvemove in one direction-mouseup", ... while this approach use a "mousedown-mousemove while exploring-mouseup".

Comment 52

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Roc,
we're using margin for scrollbars in this patch, but support for such a thing has been removed in 1.9.3 because of bug 539331 and reading at the comment it looks like it was for passing a test not because of a real design decision (https://bugzilla.mozilla.org/show_bug.cgi?id=539331#c3).

Could we hope to see the margin for scrollbar landing again, or is it such a weird thing that we don't really want that?

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think, due to trunk changes, that we can just back out the patch in bug 539331. Can you try that and confirm that it passes tests on the try-server? If so, we'll back it out.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In particular, the assertions that we needed to avoid in bug 539331 no longer exist.

Comment 55

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: backing out bug 539331 on 1.9.3

I'll try this patch on tryserver, I think that it is enough.

Comment 56

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 433157 [details] [diff] [review]
backing out bug 539331 on 1.9.3

Everything seems fine on the try server, except winmo which is burning for a few before my test. (http://build.mozilla.org/tryserver-builds/[e-mail]-out539331-2)

I've try to not revert the code cleanup done in the previous patch.

Comment 57

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Tests infrastructure on device

As asked by mfinkle, here the tests infrastructure I've set up to test this on device (or on desktop)

Comment 59

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 432814 [details] [diff] [review]
Patch v0.3

Speed seems acceptable on device. I checked myself and had madhava check too.

We found a bug where the indicator would not appear during apnning (example mid way down google news).

Filing a followup bug.

Comment 60

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I've fill bug 556603 as a followup for the non appearing scrollbars.

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/551a743f9eca

Comment 62

[Mark Finkle (:mfinkle) (use needinfo?)]

We still need to push the backout patch to mozilla-central

Comment 63

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100402 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100402 Namoroka/3.7a4pre Fennec/1.1a2pre

follow-up bugs filed:

https://bugzilla.mozilla.org/show_bug.cgi?id=556772
https://bugzilla.mozilla.org/show_bug.cgi?id=556769
https://bugzilla.mozilla.org/show_bug.cgi?id=556770
https://bugzilla.mozilla.org/show_bug.cgi?id=556774
https://bugzilla.mozilla.org/show_bug.cgi?id=556776

Comment 64

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed backout to m-c:
http://hg.mozilla.org/mozilla-central/rev/fc72a7fdbc64

Comment 65

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out while we look for a performance regression while panning:
http://hg.mozilla.org/mobile-browser/rev/4f6da6a72eec

Comment 67

[Mark Finkle (:mfinkle) (use needinfo?)]

This bug won't make it for Fennec 1.1

Comment 68

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch - Chrome UI only

This patch add scroll indicators for chrome list. It require a platform fix and does not work for Content (platform fix to come)

For content there is probably 3 options to show scroll position indicators now:
 1. Draw scroll indicators on a canvas floating above content and draw it manually
 2. Have xul|scrollbars floating above content and adjust them manually (size, position)
 3. Have a way to draw the native scrollbars from the content (they are into their own layer for top document if I understand correctly the code) to a custom position on the chrome side

I wonder if cjones has any ideas for the last point?

Comment 69

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - Platform fix for the Chrome UI to draw scrollbars above content

Comment 70

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 478782 [details] [diff] [review]
Patch - Platform fix for the Chrome UI to draw scrollbars above content

The patch just move some lines of code below to allow scrollbars to be on top of the display list and then allow them to be properly redraw in case they are positioned above the scrollable content.

I'm not certain about the negative side of moving the code here?

Comment 71

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Chrome UI + Content

This patch add add scrollbars for both the chrome UI and the content and use boxes floating above the content. If needed I can do 2 separate patches.

The chrome UI side still needs the platform patch to work correctly

Comment 72

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Roc,
about the patch on which I've asked a review the patch is just a few lines of code move from one place to another into the BuildDisplayList method of nsGfxScrollFrame.cpp. The patch fix a regression probably because of Layers.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In general we don't want scrollbars to be above content that is later than the scrolled element in the content tree. But you can do this for document viewport scrollbars (mIsRoot) --- is that enough for you?

Comment 74

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #73)
> In general we don't want scrollbars to be above content that is later than the
> scrolled element in the content tree. But you can do this for document viewport
> scrollbars (mIsRoot) --- is that enough for you?

In Fennec we might want scrollbars to float above content for basically all our chrome list which can leave wherever.
Does moving the scrollbar above content hurts performance and should we consider doing fake scrollbars (as html:div for examples) floating over the content?

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What exactly do you mean by "floating over content"? What does the DOM look like?

Comment 76

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #75)
> What exactly do you mean by "floating over content"? What does the DOM look
> like?

The visual result we want looks like this screenshot:
https://bug515166.bugzilla.mozilla.org/attachment.cgi?id=421244

This one can be achieve with the patch above by assigning a "position: relative" and negative margins to the scrollbars, so the DOM is the usual one for a container.

For the alternative solution I do not have a final idea yet but the way chosen for making the scrollbars for content (which are different from the scrollbars for the Chrome UI) is basically <stack><content/><vscrollbar right="0" top="0"/><hscrollbar left="0" bottom="0"/></stack>

I hope this is the response you were expecting?

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't really want to make scrollbars float over content in general. That would lead to unnecessary creation of ThebesLayers (see bug 572612).

But we could add a value to nsILookAndFeel that controls whether scrollbars float over content or not, so you could toggle this on.

Comment 78

[Mark Finkle (:mfinkle) (use needinfo?)]

Just to clarify, if needed, the screenshot in comment 76. The list shown is chrome UI. It's a <richlistbox>, which itself is built upon a <scrollbox>. This is not web content, if that makes any difference.

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure. I've decided Web content won't really be affected by this patch. I'm more worried about performance.

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 478782 [details] [diff] [review]
Patch - Platform fix for the Chrome UI to draw scrollbars above content

see comments

Comment 81

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - Platform fix for the Chrome UI to draw scrollbars above content - v0.2 [checked-in]

(In reply to comment #77)
> I don't really want to make scrollbars float over content in general. That
> would lead to unnecessary creation of ThebesLayers (see bug 572612).
> 
> But we could add a value to nsILookAndFeel that controls whether scrollbars
> float over content or not, so you could toggle this on.

The new patch adressed this comment.

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 484866 [details] [diff] [review]
Patch - Platform fix for the Chrome UI to draw scrollbars above content - v0.2 [checked-in]

Rename AppendScrollbarsTo to AppendScrollPartsTo.

Comment 83

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/0cf0c2f57f29

Comment 84

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - Chrome UI + Content updated on trunk

Updated UI patch on trunk.

Thanks Mounir for pushing and Roc for the fast review.

Comment 85

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - Chrome UI + Content updated on trunk (rebased)

Rebased on trunk

Comment 86

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 487630 [details] [diff] [review]
Patch - Chrome UI + Content updated on trunk (rebased)

snif.

On device the panning performance is regressed while on content and there is strange side effects when panning the chrome UI :'(

Comment 87

[Mark Finkle (:mfinkle) (use needinfo?)]

Chris Jones - What can we do here? Any thoughts?

Comment 88

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I applied https://bugzilla.mozilla.org/attachment.cgi?id=487630 but didn't see scroll indicators for chrome or content frames, and I didn't see unusual layers in the layer tree.  Is there something else I need to apply to get scroll indicators?

Comment 89

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gave this another try, and I now see

XML Parsing Error: undefined entity
Location: chrome://browser/content/browser.xul
Line Number 578, Column 7:      <toolbarbutton class="appmenu-button"
------^

Comment 90

[Matt Brubeck (:mbrubeck)]

(In reply to comment #89)
> Gave this another try, and I now see
> 
> XML Parsing Error: undefined entity
> Location: chrome://browser/content/browser.xul
> Line Number 578, Column 7:      <toolbarbutton class="appmenu-button"
> ------^

That sounds like http://hg.mozilla.org/mobile-browser/rev/4fe8d35b7f59 was not pushed properly, or was affected by a bad merge.

Comment 91

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This new patch add scrollbars only for the main content area to prevent the multiple scrollbars bugs seen while landing the first version, it also use a better positioning of the sidebars to keep them in view as much as possible.

There is still two cases where the scrollbars are not completely visible, they concern the vertical scrollbars and happens when:
 * the scroll thumb is at the top and the page is loading, in this case the toolbar is locked and appear above the thumb
 * the scroll thumb is a the bottom and the form helper/find helper is opened, in this case the scroll thumb can appear above the related UI.

I think these 2 cases will be resolved by implementing a better approach for handling the visibleScreenArea dimensions.

Comment 92

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Enhance version of the patch

Comment 93

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 497794 [details] [diff] [review]
Patch v0.2

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+          let event = document.createEvent("Events");
>+          event.initEvent("ContentNavigatorSizeChanged", true, false);
>+          this.dispatchEvent(event);

"SizeChanged"? and listen for it explicitly on the content navigator

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+  handleEvent: function handleEvent(aEvent) {

>+        // Allow a small margin on both sides to prevent adding scrollbars
>+        // on small viewport approximation
>+        let allowedMargin = 5;
>+        let scrollcornerSize = 8;

const ALLOWED_MARGIN = 5;
const SCROLL_CORNER_SIZE = 8;

>+      // right scrollbar is out of view when showing the left sidebar,
>+      // the 'solution' for now is to reposition it if needed
>+      if (Browser.floatedWhileDragging) {
>+        let [leftVis,,leftW,] = Browser.computeSidebarVisibility();
>+        this._verticalScrollbar.setAttribute("right", Math.max(2, leftW * leftVis + 2));
>+      }
>+      else if (this._verticalScrollbar.getAttribute("right") != 2) {
>+        this._verticalScrollbar.setAttribute("right", 2);

Add a const for the '2'


>diff --git a/chrome/content/input.js b/chrome/content/input.js

>       this._dragger.dragStop(0, 0, this._targetScrollInterface);
>       this._dragger = null;
>+      // XXX why is PanFinished not dispatched on the same target as PanBegin
>       let event = document.createEvent("Events");
>       event.initEvent("PanFinished", true, false);
>       document.dispatchEvent(event);

File a bug? Update this comment with a bug number and add a blank line above the comment

r+ with nits fixed

Comment 94

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Address comments

Mark,

do you mind checking if the ContentNavigatorSizeChanged -> SizeChanged change is correct? Thanks.

Comment 95

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/d9822f1768d5

yeah!

Comment 96

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20101227 Namoroka/4.0b9pre Fennec/4.0b4pre

Comment 97

[Ayan Shah[:ashah]]

https://litmus.mozilla.org/show_test.cgi?id=15030