1078859 - [Rocketbar] Only outline of current page displayed when selecting "Show windows"

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20141006_ShowWindow.txt

+++ This bug was initially created as a clone of Bug #1071295 +++

Description:
Only the outline of the current page is displayed when selecting "Show Windows"

Repro Steps:
1: Update a Flame to 20140922063004
2: From the homescreen, tap on the rocketbar and conduct a search
3: Wait until the google search page completely loads
4: Tap on (...) and select "Show Windows"

Actual:
Only the outline of the current page is displayed.

Expected:
The entire web page is displayed

Flame 2.1 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141006000205
Gaia: 778ebac47554e1c4b7e9a952d73e850f58123914
Gecko: c4a4b04c617c
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Repro frequency: 8/10
See attached: logcat, screenshot

Comment 1

[Yeojin Chung [:YeojinC]]

This issue does NOT reproduce on Flame 2.2:

Flame 2.2 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141006040204
Gaia: 470826d13ae130a5c3d572d1029e595105485fb0
Gecko: e0d714f43edc
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

The entire web page is displayed properly on Show windows screen.
====================================================
This issue occurs on New windows screen, which is a new feature on Browser Chrome. Not implemented on Flame 2.0:

Flame 2.0 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.0
BuildID: 20141006000202
Gaia: 092d2b7678774c8b0b06dca0e0a8119e9eafdec3
Gecko: 69ca61f7edf3
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Yeojin Chung [:YeojinC]]

Attachment: FrameOnly.png

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

Broken new feature. Window should be visible nominating 2.1?

Comment 4

[Marcia Knous [:marcia]]

qawanted to see if this reproduces only after a fresh flash, or consistently after that in subsequent browsing sessions.

Comment 5

[Rob MacDonald [:robmac]]

Considering the timeline, if a fallback is required, can we add a light grey background to make it look slightly less buggy? 10% grey perhaps? NI'ing Eric as an FYI.

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Marcia Knous [:marcia - use needinfo] from comment #4)
> qawanted to see if this reproduces only after a fresh flash, or consistently
> after that in subsequent browsing sessions.

Once the user starts seeing this issue, all subsequent attempts to 'Show Windows' will result in bug reproing.

The problem is the issue will NOT repro if one follows only the original STR. I'm unable to come up with an 100% repro STR without writing some 15 repetitive steps. It takes some play-around on multiple rocketbar/browser URL bar searches, killing Browser windows via 'Show windows' functionality, and eventually this issue can be reliably hit. I can get the issue to occur within 3 minutes on 317MB memory Flame 2.1 with full or shallow flash.

Basic idea to repro the bug:
1) Search via Rocketbar or Browser URL bar. Note that searching via Homescreen rocketbar will make another window, as opposed to searching on Browser URL bar will get the resultant search on the same window.
2) Make three windows displaying search results
3) On one of the browser window, tap on '...' icon > Show windows, and kill all browser windows
4) Repeat steps 2-3 two more times and bug will occur

Once bug occurs, killing all browser windows via Show windows dialog will result in all subsequent attempts to display 'Show window' to reproduce the bug.

Comment 7

[Eric Pang [:epang] UX]

(In reply to Rob MacDonald [:robmac] from comment #5)
> Considering the timeline, if a fallback is required, can we add a light grey
> background to make it look slightly less buggy? 10% grey perhaps? NI'ing
> Eric as an FYI.

If a fallback is needed we can use a light grey at full opacity #f4f4f4.

thanks!

Comment 8

[Gregor Wagner [:gwagner]]

Does this happen all the time or just the first time you open the page?

Comment 9

[Marcia Knous [:marcia]]

(In reply to Gregor Wagner [:gwagner] from comment #8)
> Does this happen all the time or just the first time you open the page?

It looks as if it was tested and the answer in is Comment 6 - continues happening according to the comment.

Comment 10

[Gregor Wagner [:gwagner]]

Aus, can you take a look if we can fix the underlying issue or do the workaround from comment 7 to close this out by friday?

Comment 11

[Gregor Wagner [:gwagner]]

Bad UX

Comment 12

[Aus Lacroix [:aus]]

Yep, I can take a look at this.

Comment 13

[Aus Lacroix [:aus]]

This issue occurs because we end up trying to grab a screenshot before the page has finished loading. Without the page being loaded there is nothing to screenshot. As soon as the page finishes loading and you move the card, the screenshot will appear (because there is one now, yay!)

I'm guessing 'doing it 2 or 3 times' helps because one gets more and more fed up by repeating the process as to do it faster and faster until finally you do it fast enough that it happens. :P

I can make this happen on Flame v2.1 v180 firmware w/1G and 319M on the first attempt.

Comment 14

[Aus Lacroix [:aus]]

Attachment: Pull Request - Force getScreenshot for system browser window instances displaying a web page.

Will add tests if this looks like the correct approach.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

Left some comments on the PR, not sure if they help much. It might be I'm just too clueless to give useful feedback on this area. More thoughts FWIW here: 

As the Card instances are just consumers of the screenshot-related API that AppWindow/BrowserMixin provides, it seems a little odd to put this isBrowser and special-casing check there. Does the edge gesture/sheets transition not have the same issue? (and if not, why not?). As to forcing getScreenshot being the right approach - I dont fully understand why it is necessary or why this fixes it. Do we not risk getting screenshots of half-rendered pages? (Some pages are never done loading there's no right time to screenshot really...) I'm not opposed to the approach, I just don't have the context to evaluate it I guess. 

It would be nice if we had something other than nothing/white to show on the card until the screenshot was available. At one point we had the icons centered in there by default and we removed that class when we determined we were going to show a screenshot instead. That maybe is a note for future releases though.

Comment 16

[Aus Lacroix [:aus]]

For others interested in why the changes are how they are:

auswerk			sfoster: the transition is to smooth the appearance of the screenshot			3:15
auswerk			however, in 2.2 we just show it			3:16
auswerk			because we share the element via moz-element			3:16
auswerk			we also show the app chrome while we are fetching the screenshot			3:16
auswerk			getting a screenshot is asynchronous because in the case where the content hasn't fully loaded it will wait for it to load before taking the screenshot.			3:16
auswerk			I could remove the isBrowser() check and the code would effectively be the same because this case *only* happens for browser windows			3:17
auswerk			because applications *always* display something so they always have a screenshot			3:17
auswerk			hope that all makes sense.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

So I took a look and I think the code looks fine. I'm not too happy about this impacting apps if it's only needed for browser frames though, it might be worthwhile to do some fastpath on an appWindow.isBrowser() check.

Since Sam has already provided some feedback, I'd like to delegate the review to him if he is comfortable with that. Sam - let me know if you can't review this and I should be able to take a more in-depth look first thing tomorrow.

Comment 18

[Aus Lacroix [:aus]]

:kgrandon, I had a check there before, but it's essentially pointless. Applications will never hit this because they will always have fully loaded content in the mozbrowser element and therefore always have a screenshot blob available. It'll only be triggered by browser windows that have content that isn't fully loaded yet.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

I'd rather Alive take a look at this - he's been reviewing all the other AppWindow/Task-Manager screenshot stuff. I'm adding him, but I can look at it tomorrow if he's out and we need this landed before the weekend.

Comment 20

[Aus Lacroix [:aus]]

:sfoster -- I would much rather have this land this week than the next as I have another blocker to take care of over the weekend already and getting this one out of the way would be immensely helpful.

Comment 21

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

I'm going to r+ this with reservations. We have already rewritten this in master so while I can see areas for improvement, they would all introduce further risk. 

I am going to file a follow-up for a related issue (not caused by this patch)  if you holdhome from a still-loading browser window, the time it takes for the task manager to show is directly correlated with the page load time (network, server latency). So loading a big page from a slow server over a bad connection is ... bad. 

Can you fix the nits Etienne pointed out?

Comment 22

[Kevin Grandon :kgrandon]

(In reply to Sam Foster [:sfoster] from comment #21)
> Comment on attachment 8502779 [details] [review]
> I am going to file a follow-up for a related issue (not caused by this
> patch)  if you holdhome from a still-loading browser window, the time it
> takes for the task manager to show is directly correlated with the page load
> time (network, server latency). So loading a big page from a slow server
> over a bad connection is ... bad. 

Is there no way to show a fallback and swap the image when it's done loading? Wouldn't that be a better experience?

Comment 23

[Aus Lacroix [:aus]]

(In reply to Kevin Grandon :kgrandon from comment #22)
> (In reply to Sam Foster [:sfoster] from comment #21)
> > Comment on attachment 8502779 [details] [review]
> > I am going to file a follow-up for a related issue (not caused by this
> > patch)  if you holdhome from a still-loading browser window, the time it
> > takes for the task manager to show is directly correlated with the page load
> > time (network, server latency). So loading a big page from a slow server
> > over a bad connection is ... bad. 
> 
> Is there no way to show a fallback and swap the image when it's done
> loading? Wouldn't that be a better experience?

We did get a placeholder color from UX to use in lieu of an image. I've gone ahead and updated my patch to use that as the default card 'image' which gets replaced with the real screenshot asap. Cards that already have a screenshot are unaffected by this change.

See updated PR.

Comment 24

[Aus Lacroix [:aus]]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): New feature.
[User impact] if declined: Blank card with only a title to discern the content they are looking at.
[Testing completed]: On Flame (319M v184) -- Latest 2.1 Gaia (Eng Build)
[Risk to taking this patch] (and alternatives if risky): Low, has unit tests.
[String changes made]: None.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 8502779 [details] [review]
Pull Request - Force getScreenshot for system browser window instances displaying a web page.

Approving this 2.1 specific patch to land on 2.1. Also requesting QA verification once this lands. Aus mentioned this is not needed on 2.2 on due to the new use of new task manager, so it does not need to land on master as its unaffected.

Comment 26

[Aus Lacroix [:aus]]

Commit (v2.1 only!): https://github.com/mozilla-b2g/gaia/commit/3bd3088c890807ebaf69c90b055732ac495991ed

Fixed!

Comment 27

[pete siphantong [:Petes]]

This bug is verified fixed on the Flame 2.1 (319mb) and the Flame 2.2 (319mb)


Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KK (319mb) (Full Flash)

Device: Flame 2.1
BuildID: 20141011000201
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Result: Selecting "Show windows", the entire web page is displayed