835152 - [Browser] Show the address bar whenever the user scrolls up

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In the Gaia browser, I can't access the URL bar or the tab switching UI without scrolling to the top of the page.  This makes using that UI quite difficult in many contexts.

Steps to reproduce:
 1. load http://en.m.wikipedia.org/wiki/Main_Page in the Gaia browser
 2. scroll down a bit
 3. long-press a link, and choose open in a new tab
 4. Look for a way to switch to that tab

Actual results: The only way to switch to the tab appears to scroll all the way back up to the top to access the "2>" UI in the upper right.

Expected results:  Perhaps, as in Firefox for Android, the "2>" UI should stay on the screen as the browser is scrolled?

Comment 1

[Josh Carpenter [:jcarpenter]]

Sympathy UX nod. We didn't have much time to iterate and test our address bar implementation for v1, and the result—despite dev best efforts—is pretty glitchy in practice, popping in and out awkwardly. UX is tracking this for improvement in future versions. There's no time frame, but we absolutely want to improve this and several other aspects of our Browser experience.

Comment 3

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

FTR, there's a simple suggestion in bug 836610 that's UX-r-.

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

fyi  kgrandon's suggestion : https://bugzilla.mozilla.org/show_bug.cgi?id=843693#c1

Comment 5

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

(That needs to be made more specific before being implementable.)

Comment 6

[Chris Lord [:cwiiis]]

I've made a suggestion in bug 868038, but I'll reiterate it here for convenience;

I think we should make the top bar permanently visible and use the same bottom toolbar that wrapped pages get. This way we get approximately the same amount of real-estate, wrapped and browser pages are still differentiated (by the toolbar), but there's none of the issues presented by the hiding toolbar.

Comment 7

[Ben Francis [:benfrancis]]

I'd like to solve this in the same was as fennec by making the address bar appear whenever the user scrolls up, not just when they reach the top.

Comment 8

[Justin Lebar (not reading bugmail)]

Ben, we should sync up wrt what platform changes you need in order to make this work.

Comment 9

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548819

Comment 10

[ffos-browser]

Karen Rudnitski deleted the linked story in Pivotal Tracker

Comment 12

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55923384

Comment 13

[Botond Ballo [:botond]]

Attachment: bug835152.patch

Moving patch from bug 860812 since it more properly belongs here. Not for review or committing until the platform changes are in place.

Comment 14

[[e-mail]]

Hi ben,

I've some time in weekend and try to implement this feature.

I try to keep current conditions, but add the scroll threshold to detect scroll up conditions in current branch.
So I can reuse show/hide AddressBar, which cause smaller impact to current code base.

https://github.com/mozilla-b2g/gaia/pull/11879

Is it sufficient to solve this issue?

Comment 15

[Ben Francis [:benfrancis]]

Hi Fred,

You should talk to Botond who is refactoring how this feature works at both the platform and Gaia level for 1.2.

Your patch would probably successfully implement the feature described in this bug, but is likely to be obsolete once bug 860812 is fixed and would need to be written again.

Comment 16

[Botond Ballo [:botond]]

(In reply to Ben Francis [:benfrancis] from comment #15)
> Hi Fred,
> 
> You should talk to Botond who is refactoring how this feature works at both
> the platform and Gaia level for 1.2.
> 
> Your patch would probably successfully implement the feature described in
> this bug, but is likely to be obsolete once bug 860812 is fixed and would
> need to be written again.

I'm not sure whether my changes will make 1.2. There's quite a few platform changes involved, and I'm currently working on some higher-priority displayport-related issues. So, if we can put a workaround in place before my changes land, I think that would be good.

Regarding Fred's patch - I think whether it works depends on how fast the user is scrolling and possibly the hardware. Since scrolling up is detected by the difference between the current scroll position's y coordinate and the previous scroll position's y coordinate being greater than 10 pixels, if scroll events are sent at a rate faster than 1 event per 10 pixels (which could be because the user is scrolling slowly or because the hardware can generate touch events faster), then nothing will be detected.

I should mention that I tried to test the patch, but I'm finding that with current gaia (with or without the patch), the address bar never scrolls off screen... not sure why that is.

Comment 17

[[e-mail]]

Thanks, I've test my patch on b2g18 and gecko master. 
It works in b2g18, but with gecko master branch, the browser can't get 'window.top' while scroll. I think it might be an oop issue?

Comment 18

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

Same issue as bug 911195 probably.

Comment 19

[mr.avgp]

Well, I had the problem in 1.2 and I fixed it without looking into Bugzilla. My bad.

However, I wonder if this looks good for you guys to make a PR into the mozilla repository: 
https://github.com/AVGP/gaia/compare/mozilla-b2g:v1.2...address_bar_on_scroll_up

Let me know what you think.

Comment 20

[mr.avgp]

Attachment: Possible fix based on v1.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: User has to scroll ALL the way up to get the address bar back :(
[Testing completed]: 
[Risk to taking this patch] (and alternatives if risky): -
[String changes made]:

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 1 - Remove JS code for hiding/showing the address bar

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 1 - Remove JS code for hiding/showing the address bar

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 2 - Implement hiding/showing the address bar using a scroll-grabbing div

Comment 24

[Botond Ballo [:botond]]

The updated patch completes the Gaia portion of the initial version of the dynamic toolbar implementation. The notable addition is using the 'scrollgrab' attribute for the div containing the toolbar and the content iframe. This attribute is implemented in the platform in bug 912666 (not landed yet).

I also rebased the patch and split it into two parts (removing the JS method for showing/hiding the address bar, and implementing the scroll-grabbing method).

With these patches + the patches in bug 912657 and bug 912666, one can now see the initial dynamic toolbar implementation working. There are some more platform bugs that needs to be fixed before these patches can land - I will track them as dependencies of bug 912666.

Comment 25

[Botond Ballo [:botond]]

Attachment: Part 2 - Implementing hiding/showing the address bar using a scroll-grabbing div

Updated patch in accordance with changes to the platform patch (bug 912666) - 'mozscrollgrab' is now a DOM property rather than an HTML attribute.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 2 - Implementing hiding/showing the address bar using a scroll-grabbing div

In the review of the platform part of this change, I was asked to rename the 'mozscrollgrab' property to just 'scrollgrab'. Updated the gaia patch accordingly.

Comment 27

[Botond Ballo [:botond]]

The platform changes this depended on (bug 912657, bug 912666) have just landed! Flagging gaia patches for review.

Comment 28

[Ben Francis [:benfrancis]]

Comment on attachment 823552 [details] [diff] [review]
Part 1 - Remove JS code for hiding/showing the address bar

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

It looks like this patch needs rebasing as it doesn't apply cleanly.

Would you mind rolling the two Gaia patches into one, rebasing them against master and sending a pull request as per https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Hacking?redirectlocale=en-US&redirectslug=Mozilla%2FFirefox_OS%2FGaia%2FHacking#Submitting_a_patch

Thanks!

Comment 29

[Ben Francis [:benfrancis]]

Attachment: Botond's patches rolled up into a single pull request

I got impatient and did that myself, botond please verify you're happy I did it correctly :)

Comment 30

[Ben Francis [:benfrancis]]

This certainly works, but I'm seeing lots of weird graphics issues when using this patch with yesterday's b2g_ril/latest-hamachi-eng-mozilla-central Gecko build.

I'm going to do some more testing to see whether this is specific to this patch or is a more general issue, and figure out how many edge cases we're missing from the UX patch.

Comment 31

[Ben Francis [:benfrancis]]

I meant "UX spec"

Comment 32

[Ben Francis [:benfrancis]]

Comment on attachment 8337714 [details] [review]
Botond's patches rolled up into a single pull request

Having tested this further it seems that applying this patch does seem to cause lots of rendering issues:
* Kinetic scrolling doesn't seem to work
* If I do a Google search I can't click on any of the hyperlinks for search results
* There seem to be a lot of repainting issues with black rectangles shown at the top and bottom of the screen
* The responsiveness of the tab tray seems to suffer, as well as lots of dropped frames on the tab tray opening animation
* A scroll bar is visible when dragging an open web page back over the tab tray to close it which looks odd (it also doesn't render very well)

as well as causing some regressions in the front end:
* It's possible to show the awesomescreen when the awesomebar is partially hidden, resulting in a broken looking UI
* The address bar won't always show when a page is loading, like it used to

Botond, do you have any idea why this patch is causing the rendering issues I describe to be exposed? Are you able to reproduce them or am I just using a bad build?

Comment 33

[Botond Ballo [:botond]]

(In reply to Ben Francis [:benfrancis] from comment #32)
> Having tested this further it seems that applying this patch does seem to
> cause lots of rendering issues:
> * Kinetic scrolling doesn't seem to work

I can reproduce this, and I know what's causing it - scrolling the content now relies on scroll handoff, and we only implemented scroll handoff for pans, not for flings.

Do you think we should implement scroll handoff for flings in general? That is, if you have an inner scrollable element, and you do a fling on it, and the inner element has scrolled as far as possible, and there is an enclosing outer scrollable element, should the rest of the fling be handed off to the outer element? This should be relatively straightforward to implement, and it will solve this problem.

If we don't want fling hand-off in general, only for this use case, that may be more difficult.

> * If I do a Google search I can't click on any of the hyperlinks for search
> results

I can reproduce this. I'll have to debug it to see what's causing it.

> * There seem to be a lot of repainting issues with black rectangles shown at
> the top and bottom of the screen

I haven't seen this - is there an example webpage that shows this?

> * The responsiveness of the tab tray seems to suffer, as well as lots of
> dropped frames on the tab tray opening animation

Is the "tab tray opening animation" just the browser contents sliding out to the left when you click on the tabs button? I didn't see any frames dropped when that happens, although it happens so quickly that it's hard to tell.

> * A scroll bar is visible when dragging an open web page back over the tab
> tray to close it which looks odd (it also doesn't render very well)

I'm not sure I understand this one. Do you mean when you're viewing the tab tray, panning left on a tab to close it causes a scroll bar to appear? I didn't see that. Where does the scroll bar appear?

> * It's possible to show the awesomescreen when the awesomebar is partially
> hidden, resulting in a broken looking UI

This can be fixed by calling scrollTo(0, 0) on the main-screen element when the awesomescreen is shown. We may have to do this for some other screens as well.

> * The address bar won't always show when a page is loading, like it used to

If a new page being loaded can be detected in the frontend, then calling scrollTo(0, 0) on the main-screen element in that case can fix that as well.

Comment 34

[Ben Francis [:benfrancis]]

Hi Botond,

Did you make any progress with the Gecko issues?

In our sprint planning meeting yesterday it was decided by Product that this work may now be too risky to land and was marked as a "no go" for 1.3 https://etherpad.mozilla.org/fxos-systems-frontend1-3

We are being discouraged from landing any big changes or features in the last sprint before feature complete.

Unless all the Gecko problems are now fixed and only trivial Gaia changes are required I think it's very unlikely we're going to be allowed to land this :(

We would have to have all the patches in place and convince Fabrice (who is acting as sherriff) that it's safe to land.

Comment 35

[Botond Ballo [:botond]]

(In reply to Ben Francis [:benfrancis] from comment #34)
> Did you make any progress with the Gecko issues?

Working on them now. I've been busy with gaia-apzc (bug 909877) issues.

Can you take a look at what I wrote about flings and tell me your opinion about it?

Comment 36

[Ben Francis [:benfrancis]]

(In reply to Botond Ballo [:botond] from comment #33)
> Do you think we should implement scroll handoff for flings in general? That
> is, if you have an inner scrollable element, and you do a fling on it, and
> the inner element has scrolled as far as possible, and there is an enclosing
> outer scrollable element, should the rest of the fling be handed off to the
> outer element? This should be relatively straightforward to implement, and
> it will solve this problem.
> 
> If we don't want fling hand-off in general, only for this use case, that may
> be more difficult.

I don't think I'm qualified to judge this but it sounds sensible. Would you be able to achieve continuous kinetic scrolling across both elements though? That sounds tricky.

> I haven't seen this - is there an example webpage that shows this?

google.com results page
 
> Is the "tab tray opening animation" just the browser contents sliding out to
> the left when you click on the tabs button?

Yes that's it, it has a really bad frame rate for me.

> > * A scroll bar is visible when dragging an open web page back over the tab
> > tray to close it which looks odd (it also doesn't render very well)
> 
> I'm not sure I understand this one. Do you mean when you're viewing the tab
> tray, panning left on a tab to close it causes a scroll bar to appear? I
> didn't see that. Where does the scroll bar appear?

No sorry, I mean when the tab drawer is open you can close it again by pulling the content back over the top of the tab drawer.

I would recommend talking to Peter (Product manager for Systems Front End) before spending too much more time on this as it's already been marked as a "no go" for 1.3 :(

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's wrong with continuing the work so that it can at least make 1.4?

Comment 38

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #33)
> > * If I do a Google search I can't click on any of the hyperlinks for search
> > results
> 
> I can reproduce this. I'll have to debug it to see what's causing it.

I looked into this a bit and filed bug 944521 for it.

Comment 39

[Ben Francis [:benfrancis]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #37)
> What's wrong with continuing the work so that it can at least make 1.4?

The browser app is unlikely to exist in 1.4.

That isn't to say that the Gecko work will be wasted, it may well be useful in other areas like the new system browser. But if these particular Gaia changes don't make 1.3 they are likely to never land. Unfortunately right now I think turning it on results in a worse experience than we have now so I wouldn't want to turn it on.

I've discussed this with Botond before so hopefully he was aware.

Comment 40

[Robert Kaiser]

(In reply to Ben Francis [:benfrancis] from comment #39)
> The browser app is unlikely to exist in 1.4.

I would be estranged if there would be nothing where I can see and edit a URL bar - as much as I understand where Haida comes from.

Comment 41

[Ben Francis [:benfrancis]]

(In reply to Robert Kaiser (:[e-mail]) from comment #40)
> (In reply to Ben Francis [:benfrancis] from comment #39)
> > The browser app is unlikely to exist in 1.4.
> 
> I would be estranged if there would be nothing where I can see and edit a
> URL bar - as much as I understand where Haida comes from.

Don't worry, there will be an address bar :)

Comment 42

[Robert Kaiser]

(In reply to Ben Francis [:benfrancis] from comment #41)
> Don't worry, there will be an address bar :)

Phew, OK. ;-)
So, does the patch here apply to that still or is the concept for it so radically different on Haida that it can't? If so, are there any mockups that can make us understand why that is?

Comment 43

[Ben Francis [:benfrancis]]

There are some old mockups but they're still in flux

https://mozilla.app.box.com/s/s3xx046zxhxyxci971ur
https://mozilla.app.box.com/s/oiahvxjubsd7h6kgtkc2

There's one idea I've seen where the address bar basically shrinks into the status bar and the title is shown on the status bar but I don't have a link for that.

We just don't have a final UX spec for it yet so we just don't know.

Comment 44

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #38)
> (In reply to Botond Ballo [:botond] from comment #33)
> > > * If I do a Google search I can't click on any of the hyperlinks for search
> > > results
> > 
> > I can reproduce this. I'll have to debug it to see what's causing it.
> 
> I looked into this a bit and filed bug 944521 for it.

The fix for this bug also fixed the issue with flings (since touch events themselves now go to the innermost scrollable frame even if an outer one is scroll-grabbing, it's always the innermost scrollable frame that flings (and only that, as fling handoff isn't implemented)). There is a minor implementation artefact where, if your pan turns into a fling while the address bar is partially off-screen, the fling will apply to the page content instead of first bringing the address bar fully on- or off-screen, but I think that's a fairly minor issue and shouldn't block this.

Comment 45

[Botond Ballo [:botond]]

> * There seem to be a lot of repainting issues with black rectangles shown at
> the top and bottom of the screen
> * The responsiveness of the tab tray seems to suffer, as well as lots of
> dropped frames on the tab tray opening animation

I still can't reproduce these issues.

> * A scroll bar is visible when dragging an open web page back over the tab
> tray to close it which looks odd (it also doesn't render very well)

Is the same scroll bar not visible when dragging the address bar into and out of view? (I agree it shouldn't be there, just wanted to establish that this is not specific to dragging an open web page back over the tab tray.)

Comment 46

[Gregor Wagner [:gwagner]]

Should we consider taking this now since the browser stays in 1.4?

Comment 47

[Ben Francis [:benfrancis]]

This is a frequently requested feature in the browser app, and we haven't added any features to the browser app for several releases now! But as I understand it the feature complete date for 1.4 has been brought forward and we are no longer allowed to land new features for 1.4.

The last time I looked at this patch it still caused some regressions so it would need more work.

Comment 49

[Botond Ballo [:botond]]

I haven't worked on this in the past months, and I'm not sure when I will. Untaking to reflect this.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[Blocking Requested - why for this release]:  This is a major usability problem in the browser; it interferes with many use cases involving use of tabs.  For example, if you go to a newspaper website and open a few articles in tabs, read one of the articles to the end, and then want to read another, the the only way to do that is to scroll all the way back to the top in order to close the tab.

Comment 51

[Kevin Grandon :kgrandon]

We are actually going to have this functionality with rocketbar in 2.1. It will work with scroll-grab, and is a much better experience IMO. I'm not sure if we need to block given that the browser app is going away in 2.1, and will be replaced by the global rocketbar experience.

Comment 52

[Gregor Wagner [:gwagner]]

(In reply to Kevin Grandon :kgrandon from comment #51)
> We are actually going to have this functionality with rocketbar in 2.1. It
> will work with scroll-grab, and is a much better experience IMO. I'm not
> sure if we need to block given that the browser app is going away in 2.1,
> and will be replaced by the global rocketbar experience.

Is this already working on master or can we dup it to the bug that implements this?

Comment 54

[Kevin Grandon :kgrandon]

This is already implemented in the 2.1 system browser through a variety of bugs and patches. Should probably be feature-b2g: 2.1+.

Comment 55

[Sue]

Attachment: video of issue verify

This issue has been verified successfully on Flame 2.1
STR:
1. Load http://en.m.wikipedia.org/wiki/Main_Page in the Gaia browser
2. Scroll down a bit
3. Long-press a link, and choose open link in new tab
4. Slide down and then slide up the webpage.
**When you slide up, the menu icon "..." will show up.
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.040617
FW-Date         Tue Nov 25 04:06:28 EST 2014
Bootloader      L1TC00011880