1213894 - Progress bar doesn't appear for pinned chrome

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect, P3)

Description

[Ben Francis [:benfrancis]]

STR:
* Pin a site
* Load the site
* Click on a link to navigate to another page within the pinned site

Expected:
* Progress bar appears

Actual:
* Progress bar disappears

Note: Sometimes the progress briefly appears but then disappears before the page has finished loading.

Comment 1

[Gregor Wagner [:gwagner]]

[Blocking Requested - why for this release]:

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] jedireza:pinned-progress-bar > mozilla-b2g:master

Comment 3

[Reza Akhavan [:jedireza]]

Comment on attachment 8675472 [details] [review]
[gaia] jedireza:pinned-progress-bar > mozilla-b2g:master

I made a small change to a css selector and it seemed to have fixed the issue for me.

I tested by navigating regular windows, private windows and pinned sites. In all types of windows I scrolled up and down to show and hide the `.maximized` app chrome styling while the loading bar was showing and animated.

I see the last time this selector changed was just over a year ago [1]. Maybe my fix isn't the right place to correct this. Maybe something else changed with `.collapsible` `appWindow`s.

[1] https://github.com/mozilla-b2g/gaia/commit/9c07d84b6f8fd7d400d074ad97fd0bb31226a356#diff-201b9947c6f450ef0c1389fda2e6ddc8L388

Comment 4

[Reza Akhavan [:jedireza]]

Comment on attachment 8675472 [details] [review]
[gaia] jedireza:pinned-progress-bar > mozilla-b2g:master

The previous solution caused an integration test to fail. I've updated the PR with an alternative solution that doesn't break the integration test locally.

Comment 5

[Reza Akhavan [:jedireza]]

Comment on attachment 8675472 [details] [review]
[gaia] jedireza:pinned-progress-bar > mozilla-b2g:master

Tests are all green. When the treeherder links work again the test results should be viewable here:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=01c86d3f4e6fb1002b6f33332fea907d557c9ec1

Comment 6

[Ben Francis [:benfrancis]]

Thanks Reza,

I've had a quick look at this and I'm sure it fixes the bug, but I'd like to dig into this a bit more to understand why we need a class that we didn't need before, this CSS is already plenty complicated enough. Alberto if you get to this before me tomorrow please take a look but I'd like to know where the regression came from.

Comment 7

[Alberto Pastor [:albertopq]]

Did we ever show the progressbar when collapsed? I think we were expanding on every location change. Regarding the new class, probably we can achieve the same removing the :not(.collapsible) from the selector.

Comment 8

[Ben Francis [:benfrancis]]

(In reply to Alberto Pastor [:albertopq] from comment #7)
> Did we ever show the progressbar when collapsed?

Actually yes, you can collapse the browser chrome of a loading page by scrolling the page.

> the same removing the :not(.collapsible) from the selector.

Does that work Reza? It might make the progress bar appear for app windows too, though I'm not sure if that's a bad thing. If it does then let's ask UX about that.

.collapsed sounds like the same thing as not(.maximized) and I think is the wrong thing to set here. Being collapsed/expanded is orthogonal to being pinned/unpinned. Being pinned just means the chrome doesn't expand/collapse on scroll.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] jedireza:pinned-progress-bar-alt > mozilla-b2g:master

Comment 10

[Reza Akhavan [:jedireza]]

(In reply to Ben Francis [:benfrancis] from comment #8)
> Does that work Reza? It might make the progress bar appear for app windows
> too, though I'm not sure if that's a bad thing. If it does then let's ask UX
> about that.

This was my original solution but it causes an integration test failure. I created an alternative PR (above) and locally this integration test fails:

```
$ TEST_FILES="apps/system/test/marionette/browser_fullscreen_navigation_chrome_test.js" make test-integration
```

I'll continue with this approach and see how I can fix the integration test.

Comment 11

[Kevin Grandon :kgrandon]

(In reply to Reza Akhavan [:jedireza] from comment #10)
> I'll continue with this approach and see how I can fix the integration test.

That test is probably there for a reason so please make sure you don't regress the progress bar in fullscreen chrome apps (like The Guardian/Facebook).

Comment 12

[Reza Akhavan [:jedireza]]

Comment on attachment 8677081 [details] [review]
[gaia] jedireza:pinned-progress-bar-alt > mozilla-b2g:master

I found another solution. I added a selector for `:not(.fullscreen-app)`. I'm seeing a progress bar where I'd expect. The PR is updated and treeherder [1] is green (although I'm not sure what's up with Gij28, it's been going for hours).

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b0188278d0ea37d9f7930e28588254ce5b4bb418

Comment 13

[Ben Francis [:benfrancis]]

Still not sure if this is right, did you look at wrapper.css? https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/wrapper/wrapper.css

Isn't it the visibility property we need to change, not top?

Comment 14

[Reza Akhavan [:jedireza]]

(In reply to Ben Francis [:benfrancis] from comment #13)
> Still not sure if this is right, did you look at wrapper.css?
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/wrapper/
> wrapper.css
> 
> Isn't it the visibility property we need to change, not top?

I can double check today but at fist thought, no. The gaia-progress element is visible, but for pinned sites, it's position is off the screen, moving it down using `top` did the trick.

Comment 15

[Reza Akhavan [:jedireza]]

I also noticed that for app windows using the `.fullscreen-app` class, the chrome is completely off screen and can be pulled down (by swiping down from the top edge) to access the controls.

Using the `apps/system/test/apps/fullscreennavapp` test app, I navigate to the second page, pull the chrome down and begin hitting back and forth controls. Although we're not getting a good representation of network delay, the progress bar does blip for a second in the right place.

The unit test that was failing is verifying that the gaia-progress element is not visible for full screen apps, as it shouldn't be. It tests this based on the element's rect coordinates.

We have two proposed solutions that seem to address the symptoms mentioned. One with an explicit class name, which Ben didn't like and another that does it by using a `:not(.fullscreen-app)` selector.

Ben, can you help me understand what I may be overlooking to achieve a more ideal solution?

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 8677081 [details] [review]
[gaia] jedireza:pinned-progress-bar-alt > mozilla-b2g:master

Sorry Reza, it's just this area of CSS is a bit of a mess and every time we change something it causes a new regression! I think the second patch is OK I was just worried the selector might be too broad.

But I've played around with it now and I can't find any obvious regressions so let's get this landed once Treeherder is green and ask QA to double check we didn't break anything.

Sorry it took so long to get this tiny patch reviewed!

Comment 17

[Reza Akhavan [:jedireza]]

Tests are all green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b0003e33fbce77f7a96cd7a1996d2f5a96b4d737

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/3d2e28c2d7ce587626784ff291ce3b049f7d7d0a

Comment 19

[Alison Shiue]

Verified on

[Flame]
Build ID               20151026030217
Gaia Revision          a677ddd3aa3a81058775938bd56008d96dbc78b0
Gaia Date              2015-10-26 04:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5ca03a00d26823ce91ee0eaa2937bed605bd53c1
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151026.070911
Firmware Date          Mon Oct 26 07:09:23 EDT 2015
Bootloader             L1TC000118D0

[Aries]
Build ID               20151026223015
Gaia Revision          b564b21e08ffc3e4962f08850843c7482932ee7b
Gaia Date              2015-10-26 14:22:21
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151026.214940
Firmware Date          Mon Oct 26 21:49:48 UTC 2015
Bootloader             s1