1249577 - Hello FTU panel cut off and some glitches can be seen

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Bogdan Maris, Desktop QA]

Attachment: Gif showing the issue

[Affected versions]:
- latest Developer Edition 46.0a2
- latest Nightly 47.0a1

[Affected platforms]:
- Mac OS X 10.10.5
- Ubuntu 14.04 32-bit
- Windows 10 32-bit

[Steps to reproduce]:
1. Start Firefox
2. Click Hello icon
3. Repeat step 2 twice
if the issue does not reproduce an two additional steps are required
4. Click the grey area where 'See how it works' is shown (not on the button)
5. Repeat step 2 twice

[Expected result]:
- FTU panel is properly shown.

[Actual result]:
- FTU panel is cut off and glitches can be seen while opening and closing Hello panel.

[Regression range]:
- Not sure if this regression range is accurate but Good build did not display FTU even though I had 'loop.gettingStarted.latestFTUVersion' pref set to '1'

M-C:
Last good revision: e355cacefc881ba360d412853b57e8e060e966f4 (2016-02-15)
First bad revision: 6ea654cad929c9bedd8a4161a182b6189fbeae6a (2016-02-16)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e355cacefc881ba360d412853b57e8e060e966f4&tochange=6ea654cad929c9bedd8a4161a182b6189fbeae6a
M-I:
Last good revision: ea39d4a6232c278dd8d805608a07cf9f4cc4c76b
First bad revision: fb7ceaf4b009243423cdaf1077adbb6c609b22e5
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ea39d4a6232c278dd8d805608a07cf9f4cc4c76b&tochange=fb7ceaf4b009243423cdaf1077adbb6c609b22e5


[Additional notes]:
- Firefox Beta and Release does not have the FTU enabled.

Comment 1

[Romain Testard [:RT]]

I experienced this on Windows too, pretty bad since it's the first thing the user sees and the button is hidden, therefore preventing him from doing anything in the panel. SOmething we should fix before launch, marking a a rank 5 to reflect this.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Turns out the switching the order DOMContentLoaded message handler attachment was not enough to fix it reliably.

I've done a bunch more debugging, and currently, it's looking like the problem may be related to firing socialFrameShow at inappropriate times.  I've found that ensuring it never gets called more than once seems to make the problem go away.  I need to dig through some more code and verify more details to see whether that's the right fix.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

After spending lots of quality time with window.dump, this is now looking like it could be a logic error introduced by bug 1087934 around <http://hg.mozilla.org/mozilla-central/diff/0387607be3e8/browser/modules/Social.jsm#l1.41>.  I'm gonna try to mind-meld with mixedpuppy Thursday to see if he agrees and discuss possible fixes (I have a couple of prototype fixes that appear to work, but it's not yet obvious to me which (if either) is correct).

Comment 4

[Shane Caraveo (:mixedpuppy)]

a) you should get rid of the use of socialframeshow/hide, I'd like to remove that from the code (bug 1032398).  use the visibility api.
b) that sizing code is incredibly tricky and hard to know changes are right.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: diff

WIP with logging...

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Fix Social/Loop dynamic panel resizing glitchiness

I have yet to do the manual testing that we discussed, I'm gonna start a try run and then 
dig into that.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f27b8d5e695b

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Excitingly, after working with Shane on this today, the revised patch turned out to have some issues.  Since we need something for 45 (which can't get an uplift) very soon, and it isn't yet clear how much effort this will be to fix in correctly in core, I'm going to experiment with an add-on only strategy for 45 on Friday, with the hopes that we can use it for the other releases as well until we can make the right thing happen in core.

My current theory is to attach a loop-specific popupshown handler to the panel (in addition to the main one attached in Social.jsm), and then using a simpler resize algorithm (since we will only ever be loading the one page) there at the appropriate moment.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Another thing that has occurred to me to try is to open (and/or tweak a setting, or re-open) the panel in statically sized mode when the FTU needs to run, and open it in dynamically-sized mode the rest of the time.

Comment 10

[GitHub Autolander Bot]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/250

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/250

This appears to basically work, and should work on all versions of the add-on without needing core fixes.  Thanks to Shane for the suggestion.

Still needs cleanup and automated tests.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

The excitement continues.  Although the above patch seems to work perfectly in 47, it doesn't work at all in 46, for completely unclear reasons, given that the code involves hasn't changed.  Some platform-dependency, I'm guessing.

Current theory looks like this: add the option to pass in a resizer class to PanelFrame.jsm, and uplift that through to 46 (Shane's idea, and he's cool with this).  For 45, some basic testing suggests that I should, in theory, be able to simply set the showPopup function on the loopNotificationPanel object to a forked version of showPopup that also calls a forked copy of the resizer specific to 45.  We also would need to convince ourselves that the 46 upgrade will do something reasonable if the (monkey-patched) system update to 45 is still installed.  Standard8, do you have any insight on that?

Yuck.  But it could work.  I think.

The saga continues tomorrow.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Excitingly, on my machine, at least, the FTEView fix setting explicit style on the body element seems to work in 45 at least, though it doesn't work in the German version of 46.

Separately from that here's a PR that fixes the problem in 45 that forks PanelFrame, DynamicResizer and sizeSocialPanelToContent and applies some variants of the fix that mixedpuppy and I were looking at in comment 6.  Unfortunately, this fix now breaks dynamic sizing within the panel itself in the case covered by the functional test.  

https://github.com/mozilla/loop/pull/264/files

Happily, we have the functional test to detect this, and also happily, we can mess with the fork without putting any of the Social API providers in the wild at risk, since only our add-on uses the fork.

The very next step will be:

* graft the extensive logging that I had before into the forked copy

After that, we can do one or many of the following:

* convince ourselves that the FTEView fix in 45 will be reliable, perhaps by...
* figuring out what causes this not to work in 46

and/or

* debug why this forked "fix" regresses the functional test in 45

I'll go for the FTEView plan first, since if it works, it'll be way less gross.

We'll still need to do something like the forked version (and debug it) for 48, 47, and 46.

Good times!

Comment 15

[GitHub Autolander Bot]

Attachment: [loop] dmose:fte-simple-fix-1249577 > mozilla:master

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/268

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

This came from the most extreme of extreme programming, with ~5 engineers chiming.  Thanks in particular to ianb and crafuse who offered the key insights leading us to figure out why this was happening.

There will be a followon patch removing the width as well, and a mochitest too, assuming that's practical.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8728583 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/268

Yep, tested this earlier, it looked good :-)

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Pushed.  Another patch forthcoming, probably Thursday.

Comment 21

[Jim Mathies [:jimm]]

any update here?

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

The bug itself is fixed; this is still open for a few details to be spin off: cleaning up the width, and writing a mochitest.  If you'd prefer, I can spin those off to another bug.

Comment 23

[GitHub Autolander Bot]

Attachment: [loop] dmose:ftu-width-cleanup > mozilla:master

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8735987 [details] [review]
[loop] dmose:ftu-width-cleanup > mozilla:master

After discussion with Standard8, we decided that it wasn't worth testing against an exact width, as that's very fragile in the face of UI changes.  This patch just removes the bogus width constraint (which, unlike the bogus height constraint doesn't seem to be causing any problems at the moment).  The patch doesn't visually change anything.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

CSS width cleanup pushed:

https://github.com/mozilla/loop/commit/493e71adf4d51ec23ff379c163255197ff1a3341

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Was this fixed in the last Hello push? Or is it still an issue in 46?

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Comment 28

[Mark Banner (:standard8)]

This was shipped in the update to 1.1.12 which then became 1.1.14 which was shipped to users via the update system for 45.