1392468 - [Onboarding] replace overlay icon to fox icon

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P2)

Description

[[e-mail]]

based on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit

Icon can be found as https://bugzilla.mozilla.org/show_bug.cgi?id=1390306#c4

Comment 1

[[e-mail]]

Attachment: Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review commit: https://reviewboard.mozilla.org/r/171986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171986/

Comment 2

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/1-2/

Comment 3

[Fischer [:Fischer]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

https://reviewboard.mozilla.org/r/171986/#review178390

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2db66a67c944
[Onboarding] replace icon and change to the new position;r=Fischer

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing clipboard's browser/base/content/test/newtab/browser_newtab_undo.js:

https://hg.mozilla.org/integration/autoland/rev/d5b6d113cf17f4c91b574eaa2d077a233bc4bc69

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2db66a67c9446e31bbed8d359a5214addde35fcc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&filter-searchStr=clipboard
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=126397878&repo=autoland

03:51:45     INFO - TEST-PASS | browser/base/content/test/newtab/browser_newtab_undo.js | grid status = 5p,0,1,2,3,4,6,7,8 - ["5p","0","1","2","3","4","6","7","8"] == "5p,0,1,2,3,4,6,7,8" - 
03:51:45     INFO - Buffered messages logged at 03:51:02
03:51:45     INFO - TEST-PASS | browser/base/content/test/newtab/browser_newtab_undo.js | grid status = 5p,0,1,2,6,7,8 - ["5p","0","1","2","6","7","8"] == "5p,0,1,2,6,7,8" - 
03:51:45     INFO - Buffered messages finished
03:51:45     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_undo.js | Test timed out -

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

For the record, this passed on Linux, but failed on OS X and Windows.

Comment 7

[[e-mail]]

Attachment: overlaped the restore all link

new Icon position will overlap the `restore all` message button, and cause test fail (because the mouse can't click the `restore all` link).

The under message is `Thumbnail removed, Undo / Restore All` and take about 20px height.


Verdi, any suggestion?

Comment 8

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/2-3/

Comment 9

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/3-4/

Comment 10

[[e-mail]]

the updated patch removed position change, so the new tab restore test will pass.
The position change should handle by a separate bug when we have the decision.

The update also
* add 2x resolution image for notification-tour-icon.
* keep using icon64.png for overlay-icon, which is 2x larger for origin size.

We can do matchMedia in JS and pick icon32.png/icon64.png accordingly since resize bigger image may take some time. 
But since onboarding is running in nextTick, the operation should not effect page loading time.

Comment 11

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/4-5/

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

https://reviewboard.mozilla.org/r/171986/#review178916

::: browser/extensions/onboarding/content/onboarding.css:514
(Diff revision 5)
>    }
>  }
> +
> +@media (min-resolution: 2dppx) {
> +  #onboarding-notification-tour-icon {
> +    background-image: url("chrome://branding/content/icon128.png");

browser/base/content/test/static/browser_all_files_referenced.js | unused whitelist entry: chrome://branding/content/icon128.png

Did you checked whether this fail is releated?

Comment 13

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/5-6/

Comment 14

[[e-mail]]

Thanks for finding that. Icon128.png seems not used anywhere before this patch, I'd remove icon128.png from whitelist and ni in bug 1339420 to make sure the icon is still exist in resource.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The test was implemented in bug 722234. It's part of the "new tab page" replaced by activity streams. I guess we could trun around and disable onboarding on that test, since we do not intend for the onboarding 57 to work with it.

This can be figured out by checking the commit log ... thanks verdi for giving me the idea to look it up.

Comment 16

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/6-7/

Comment 17

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/7-8/

Comment 18

[[e-mail]]

Per comment 7 and comment 15, the update PR add position change back (to the top left edge) and disabled onboarding tour in `browser/base/content/test/newtab/browser_newtab_undo.js` so the fox icon won't show on the undo test.

Disable newtab undo test with the new onboarding icon position might miss the test if activity-stream is not default-on on v57, but we may bear that small chance...
Dao, please raise if you have any concern on that, thanks.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fred Lin [:gasolin] from comment #18)
> Per comment 7 and comment 15, the update PR add position change back (to the
> top left edge) and disabled onboarding tour in
> `browser/base/content/test/newtab/browser_newtab_undo.js` so the fox icon
> won't show on the undo test.

Please disable onboarding on the entire browser/base/content/test/newtab directory, i.e. in browser/base/content/test/newtab/head.js

> Disable newtab undo test with the new onboarding icon position might miss
> the test if activity-stream is not default-on on v57, but we may bear that
> small chance...
> Dao, please raise if you have any concern on that, thanks.

We'll just have to remember to raise this if we turned out to need to go backward.

Comment 20

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/8-9/

Comment 21

[Dão Gottwald [:dao]]

(In reply to Fred Lin [:gasolin] from comment #18)
> Per comment 7 and comment 15, the update PR add position change back (to the
> top left edge) and disabled onboarding tour in
> `browser/base/content/test/newtab/browser_newtab_undo.js` so the fox icon
> won't show on the undo test.
> 
> Disable newtab undo test with the new onboarding icon position might miss
> the test if activity-stream is not default-on on v57, but we may bear that
> small chance...
> Dao, please raise if you have any concern on that, thanks.

Why should we bear that chance?

I don't think we should disable onboarding in that test but just make it work properly such that the different page features don't conflict.

Comment 22

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/9-10/

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Dão Gottwald [::dao] from comment #21)
> Why should we bear that chance?
> 
> I don't think we should disable onboarding in that test but just make it
> work properly such that the different page features don't conflict.

I am not entirely sure asking for a design update just for the sake of that possibility is productive. But yeah we can circle back to designers and activity streams engineers if you feel that we need to.

Comment 24

[Dão Gottwald [:dao]]

Not solving that issue now just means it needs to be taken care of later. That doesn't seem more productive overall, and there's a risk that we'd ship with the problem unsolved.

You could also just spin off the position change to a separate bug. It doesn't seem like the icon change needs to be blocked on that.

Comment 25

[[e-mail]]

I'd switch back to comment 10 change and moving the icon change part to the review process.

Please comment the position change related issue in bug 1395065

Comment 26

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/10-11/

Comment 27

[[e-mail]]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171986/diff/11-12/

Comment 28

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8900585 [details]
Bug 1392468 - [Onboarding] replace overlay icon to fox icon;

https://reviewboard.mozilla.org/r/171986/#review179406

Comment 29

[KM Lee [:rexboy] (inactivate after Sep. 22)]

The patch itself looks good to me but we're still waiting for the decision whether we should remove icon128.png in bug 1339420. We should land it after confirming the file will be kept.

Comment 30

[[e-mail]]

Thanks for review, I think Florian means we should keep icon128.png since it has dependency now. I'd set bug 1339420 as wontfix and land this PR if all test green

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54c1e69db5b3
[Onboarding] replace overlay icon to fox icon;r=Fischer,rexboy

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54c1e69db5b3

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This and bug 1393564 both added a "z-index:10;" to #onboarding-overlay-button, just with different comments, so I just included both comments when merging them together. Feel free to work out if one/both of them should be dropped and do that.

Comment 34

[[e-mail]]

thanks for the notice, we will only uplift bug 1393564 to beta, thanks

Comment 35

[Saddam Hossain]

I can see the feature "overlay icon to fox icon" implemented in latest Nightly 57.0a1 on Windows 10, 64-bit  

Build ID :   20170831100258
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 37

[Abe - QA (:Abe_LV)]

verified fixed.