Closed Bug 835152 Opened 7 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(b2g18 affected, b2g-v1.1hd affected, b2g-v1.2 affected, b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 verified)

RESOLVED FIXED
Tracking Status
b2g18 --- affected
b2g-v1.1hd --- affected
b2g-v1.2 --- affected
b2g-v1.3 --- affected
b2g-v1.3T --- affected
b2g-v1.4 --- affected
b2g-v2.0 --- affected
b2g-v2.1 --- verified

People

(Reporter: dbaron, Unassigned)

References

()

Details

(Keywords: feature, Whiteboard: c=browser u=user [sprintready])

Attachments

(2 files, 7 obsolete files)

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?
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.
FTR, there's a simple suggestion in bug 836610 that's UX-r-.
(That needs to be made more specific before being implementable.)
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.
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.
Keywords: feature
Summary: can't access URL bar or tab switch UI without scrolling to the top of the page → [Browser] Show the address bar whenever the browser scrolls up
Whiteboard: c=browser u=user
Summary: [Browser] Show the address bar whenever the browser scrolls up → [Browser] Show the address bar whenever the user scrolls up
Priority: -- → P1
Ben, we should sync up wrt what platform changes you need in order to make this work.
Depends on: 889883
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548819
Whiteboard: c=browser u=user → c=browser u=user [sprintready]
Karen Rudnitski deleted the linked story in Pivotal Tracker
Duplicate of this bug: 908181
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55923384
Attached patch bug835152.patch (obsolete) — Splinter Review
Moving patch from bug 860812 since it more properly belongs here. Not for review or committing until the platform changes are in place.
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?
Flags: needinfo?(bfrancis)
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.
Flags: needinfo?(bfrancis) → needinfo?(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.
Flags: needinfo?(botond)
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?
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.
Attached patch Possible fix based on v1.2 (obsolete) — Splinter Review
[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]:
Attachment #815444 - Flags: review?(mr.avgp)
Assignee: nobody → botond
Attachment #796278 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Updated patch in accordance with changes to the platform patch (bug 912666) - 'mozscrollgrab' is now a DOM property rather than an HTML attribute.
Attachment #823553 - Attachment is obsolete: true
Attachment #8336341 - Attachment description: bug835152-part2.patch → 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.
Attachment #8336341 - Attachment is obsolete: true
The platform changes this depended on (bug 912657, bug 912666) have just landed! Flagging gaia patches for review.
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!
Attachment #823552 - Flags: review?(bfrancis)
Attachment #8336392 - Flags: review?(bfrancis)
I got impatient and did that myself, botond please verify you're happy I did it correctly :)
Attachment #815444 - Attachment is obsolete: true
Attachment #823552 - Attachment is obsolete: true
Attachment #8336392 - Attachment is obsolete: true
Attachment #815444 - Flags: review?(mr.avgp)
Attachment #8337714 - Flags: review?(bfrancis)
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.
I meant "UX spec"
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?
Attachment #8337714 - Flags: review?(bfrancis) → review-
Flags: needinfo?(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.
Flags: needinfo?(botond)
Flags: needinfo?(bfrancis)
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.
Flags: needinfo?(bfrancis)
(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?
(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 :(
What's wrong with continuing the work so that it can at least make 1.4?
(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.
(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.
(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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 :)
(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?
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.
(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.
> * 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.)
Should we consider taking this now since the browser stays in 1.4?
Flags: needinfo?(bfrancis)
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.
Flags: needinfo?(bfrancis)
Duplicate of this bug: 1014630
I haven't worked on this in the past months, and I'm not sure when I will. Untaking to reflect this.
Assignee: botond → nobody
[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.
blocking-b2g: --- → 2.1?
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.
(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?
Duplicate of this bug: 1051505
This is already implemented in the 2.1 system browser through a variety of bugs and patches. Should probably be feature-b2g: 2.1+.
Status: ASSIGNED → RESOLVED
blocking-b2g: 2.1? → ---
Closed: 6 years ago
Resolution: --- → FIXED
Attached video 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
You need to log in before you can comment on or make changes to this bug.