Closed Bug 461843 Opened 11 years ago Closed 9 years ago

Show indication of where on the page you are when scrolling

Categories

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

defect

Tracking

(fennec2.0b4+)

VERIFIED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: madhava, Assigned: vingtetun)

References

Details

(Whiteboard: [has-patch])

Attachments

(3 files, 33 obsolete files)

13.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.10 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
10.18 KB, patch
Details | Diff | Splinter Review
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
Assignee: nobody → mark.finkle
Attached patch patch (obsolete) — Splinter Review
This patch displays floating scroll position indicators.
Attachment #347471 - Flags: review?(gavin.sharp)
> 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.
Target Milestone: --- → Fennec A3
Blocks: 477628
Attachment #347471 - Flags: review?(gavin.sharp)
Depends on: 489225
Assignee: mark.finkle → combee
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0b3+
Duplicate of this bug: 496650
Flags: wanted-fennec1.0?
Target Milestone: B1 → ---
OS: Mac OS X → All
Flags: wanted-fennec1.0? → wanted-fennec1.0+
We now pan in content _and_ in chrome (lists and stuff). I think the first priority here is indicators for scrolling in content.
Status: NEW → ASSIGNED
Flags: wanted-fennec1.0+
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.
The earlier implementation must be redone a bit for tile work.  I'll push this until after we see how Stuart handles the sidebars.
Attachment #347471 - Attachment is obsolete: true
Talked with mfinkle, waiting on tilecache to land to see how to overlay this UI now.
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.
Attached patch patch 2 (obsolete) — Splinter Review
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
Attached patch patch 3 (obsolete) — Splinter Review
Do as little as possible while dragging. Move more of the static code to dragStart
Attachment #395525 - Attachment is obsolete: true
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.
Attached patch wip-1 (obsolete) — Splinter Review
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 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() ?
Attached patch wip-2 (obsolete) — Splinter Review
This wip did not address comments, I've just do a bit of refactoring mostly for performances purposes.
Attachment #410548 - Attachment is obsolete: true
Attached patch wip-3 - better perfs (obsolete) — Splinter Review
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.
Attachment #410718 - Attachment is obsolete: true
Attached patch WIP-4 (obsolete) — Splinter Review
Attachment #410830 - Attachment is obsolete: true
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)
tracking-fennec: 1.0b3+ → 1.0-
Flags: wanted-fennec1.0+
Blocks: 534283
Duplicate of this bug: 534283
Attached patch new try (obsolete) — Splinter Review
New try using two canvases to draw the scrollbars. Hopefully fast enough on a device.
Attachment #417486 - Flags: review?(mark.finkle)
Attached patch new try v2 (obsolete) — Splinter Review
Better look and feel using a solid stroke for the rectangles.
Attachment #417486 - Attachment is obsolete: true
Attachment #417491 - Flags: review?(mark.finkle)
Attachment #417486 - Flags: review?(mark.finkle)
Attached patch new try v3 (obsolete) — Splinter Review
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.
Assignee: combee → fabrice.desre
Attachment #417491 - Attachment is obsolete: true
Attachment #418376 - Flags: review?(mark.finkle)
Attachment #417491 - Flags: review?(mark.finkle)
Attached image screenshot (obsolete) —
I like the blue, but I think a grey indicator would more reliably not clash given that the page beneath could be any color.
I did tests also with grey, and I'm not sure it's better than blue. The blue is usually more visible.
(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
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.
I was using a white border in one of the early patches.
So who is authoritative on this? It's a no brainer to update the patch with another color scheme once we take a decision.
(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.
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.
Attached patch new patch (obsolete) — Splinter Review
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).
Attachment #418376 - Attachment is obsolete: true
Attachment #421229 - Flags: ui-review?(madhava)
Attachment #421229 - Flags: review?(mark.finkle)
Attachment #418376 - Flags: review?(mark.finkle)
Attached image new screenshot with grey indicators (obsolete) —
Attachment #418381 - Attachment is obsolete: true
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.
Attached patch updated patch (obsolete) — Splinter Review
Updated version to show scroll indicators in HTML scrollable divs and xul-in-content scrollable elements.
Attachment #421229 - Attachment is obsolete: true
Attachment #421408 - Flags: ui-review?(madhava)
Attachment #421408 - Flags: review?(mark.finkle)
Attachment #421229 - Flags: ui-review?(madhava)
Attachment #421229 - Flags: review?(mark.finkle)
Attached image scroll indicator for pannable <div>s (obsolete) —
Attached image scroll indicator for xul-in-content (obsolete) —
Attached patch wip full css (obsolete) — Splinter Review
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.
Attachment #411202 - Attachment is obsolete: true
I have an updated patch for that who handle scrollable div and iframes. I'm still working on support for nested iframes :/
Attached patch wip-2 (obsolete) — Splinter Review
Attachment #426697 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: fabrice.desre → 21
Attachment #421408 - Attachment is obsolete: true
Attachment #427243 - Attachment is obsolete: true
Attachment #427357 - Flags: review?(mark.finkle)
Attachment #421408 - Flags: ui-review?(madhava)
Attachment #421408 - Flags: review?(mark.finkle)
Comment on attachment 427357 [details] [diff] [review]
Patch

I'm canceling review because I've just found a little UI regression with this patch.
Attachment #427357 - Flags: review?(mark.finkle)
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
Attached file Data (obsolete) —
Results given by xresponse
(In reply to comment #43)
> Created an attachment (id=428456) [details]
> Data
> 
> Results given by xresponse

on N900?
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.
(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.
Attached file data for device (obsolete) —
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")
Attached file data for non-transparent scrollbars (obsolete) —
Weird, this is slower and the median is higher
Flags: in-litmus?
Attached patch Patch v0.2 (obsolete) — Splinter Review
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).
Attached patch Patch v0.3 (obsolete) — Splinter Review
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).
Attachment #432698 - Attachment is obsolete: true
Attachment #432814 - Flags: review?(mark.finkle)
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".
Attachment #427357 - Attachment is obsolete: true
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?
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.
In particular, the assertions that we needed to avoid in bug 539331 no longer exist.
Attached patch backing out bug 539331 on 1.9.3 (obsolete) — Splinter Review
I'll try this patch on tryserver, I think that it is enough.
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/vnicolas@mozilla.com-out539331-2)

I've try to not revert the code cleanup done in the previous patch.
Attachment #433157 - Flags: review?(roc)
Attached file Tests infrastructure on device (obsolete) —
As asked by mfinkle, here the tests infrastructure I've set up to test this on device (or on desktop)
tracking-fennec: 1.0- → 1.1+
Duplicate of this bug: 515166
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.
Attachment #432814 - Flags: review?(mark.finkle) → review+
I've fill bug 556603 as a followup for the non appearing scrollbars.
pushed:
http://hg.mozilla.org/mobile-browser/rev/551a743f9eca
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
We still need to push the backout patch to mozilla-central
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
Status: RESOLVED → VERIFIED
Backed out while we look for a performance regression while panning:
http://hg.mozilla.org/mobile-browser/rev/4f6da6a72eec
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 484153
This bug won't make it for Fennec 1.1
tracking-fennec: 1.1+ → ?
tracking-fennec: ? → 2.0b2+
tracking-fennec: 2.0b2+ → 2.0+
Priority: -- → P2
Attached patch patch - Chrome UI only (obsolete) — Splinter Review
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?
Attachment #432814 - Attachment is obsolete: true
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?
Attachment #478782 - Flags: review?(roc)
Attached patch Chrome UI + Content (obsolete) — Splinter Review
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
Attachment #478781 - Attachment is obsolete: true
Attachment #481774 - Flags: review?(mark.finkle)
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.
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 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?
What exactly do you mean by "floating over content"? What does the DOM look like?
(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?
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.
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.
Sure. I've decided Web content won't really be affected by this patch. I'm more worried about performance.
Comment on attachment 478782 [details] [diff] [review]
Patch - Platform fix for the Chrome UI to draw scrollbars above content

see comments
Attachment #478782 - Flags: review?(roc) → review-
Hardware: x86 → All
(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.
Attachment #478782 - Attachment is obsolete: true
Attachment #484866 - Flags: review?(roc)
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.
Attachment #484866 - Flags: review?(roc) → review+
Attachment #484866 - Attachment description: Patch - Platform fix for the Chrome UI to draw scrollbars above content - v0.2 → Patch - Platform fix for the Chrome UI to draw scrollbars above content - v0.2 [checked-in]
Updated UI patch on trunk.

Thanks Mounir for pushing and Roc for the fast review.
Attachment #481774 - Attachment is obsolete: true
Attachment #485043 - Flags: review?(mark.finkle)
Attachment #481774 - Flags: review?(mark.finkle)
tracking-fennec: 2.0+ → 2.0b3+
Rebased on trunk
Attachment #485043 - Attachment is obsolete: true
Attachment #487630 - Flags: review?(mark.finkle)
Attachment #485043 - Flags: review?(mark.finkle)
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 :'(
Attachment #487630 - Flags: review?(mark.finkle)
Whiteboard: [has-patch]
Chris Jones - What can we do here? Any thoughts?
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?
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"
------^
(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.
tracking-fennec: 2.0b3+ → 2.0b4+
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #421230 - Attachment is obsolete: true
Attachment #421409 - Attachment is obsolete: true
Attachment #421410 - Attachment is obsolete: true
Attachment #428456 - Attachment is obsolete: true
Attachment #428724 - Attachment is obsolete: true
Attachment #428735 - Attachment is obsolete: true
Attachment #432815 - Attachment is obsolete: true
Attachment #433157 - Attachment is obsolete: true
Attachment #433557 - Attachment is obsolete: true
Attachment #487630 - Attachment is obsolete: true
Attachment #496840 - Flags: review?(mark.finkle)
Attached patch Patch v0.2Splinter Review
Enhance version of the patch
Attachment #496840 - Attachment is obsolete: true
Attachment #497794 - Flags: review?(mark.finkle)
Attachment #496840 - Flags: review?(mark.finkle)
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
Attachment #497794 - Flags: review?(mark.finkle) → review+
Attached patch Address commentsSplinter Review
Mark,

do you mind checking if the ContentNavigatorSizeChanged -> SizeChanged change is correct? Thanks.
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20101227 Namoroka/4.0b9pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Depends on: 623485
You need to log in before you can comment on or make changes to this bug.