1447043 - Selection UI positioning errors in latest Screenshots

Status: VERIFIED FIXED

(Firefox :: Screenshots, defect)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

We've seen multiple reports of selection errors since the last export to Nightly:

https://github.com/mozilla-services/screenshots/issues/4217
https://github.com/mozilla-services/screenshots/issues/3907
https://github.com/mozilla-services/screenshots/issues/3972

The fix is simple and low risk for uplift, so we will export it directly:

https://github.com/mozilla-services/screenshots/issues/4218

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1447043 - Calculate top and left selection margin using left and top;

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

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1447043 - Export Screenshots 30.1.0 to Firefox;

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

Comment 3

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8960250 [details]
Bug 1447043 - Export Screenshots 30.1.0 to Firefox;

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

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Bug 1447043 - Calculate top and left selection margin using left and top;

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

Comment 5

[Ian Bicking (:ianbicking)]

Comment on attachment 8960250 [details]
Bug 1447043 - Export Screenshots 30.1.0 to Firefox;

https://reviewboard.mozilla.org/r/229006/#review234754

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8960254 [details]
Bug 1447043 - Calculate top and left selection margin using left and top;

https://reviewboard.mozilla.org/r/229016/#review234752

Comment 7

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Try build looks good[1], landing this patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7a69c81998ac89e38a7f817680d945b3f619ae

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc8d54d3b19c
Calculate top and left selection margin using left and top; r=_6a68
https://hg.mozilla.org/integration/autoland/rev/1db369ac3b20
Export Screenshots 30.1.0 to Firefox; r=ianbicking

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cc8d54d3b19c
https://hg.mozilla.org/mozilla-central/rev/1db369ac3b20

Comment 10

[Cosmin Muntean [:cmuntean], Ecosystem QA - PTO until 18 Sept - ping mcoman for any question]

I have retested this issue on latest Nightly (61.0a1, build from 2018-03-21) build and the issue is no longer reproducible. The Screenshots selection is working as expected on the reported websites:
- https://www.ghacks.net/2018/03/12/firefox-59-firefox-screenshots-gets-image-editing-functionality/
- https://www.reddit.com/r/iamverysmart/comments/84b7dn/message_for_all_the_people_who_think_iq_matters/
- https://brightside.me/
- https://www.npr.org/

I can also confirm that the issue is reproducible on older Nightly (eg: build from 2018-03-19) builds. In order to reproduce issue I have followed the provided STR from the GitHub issue: https://github.com/mozilla-services/screenshots/issues/4217

In order to make sure if there are no new issues or regressions, I have verified if the Screenshots selection works as expected on the first 50 websites from https://www.alexa.com/topsites page and also a few more that we consider popular (eg: cnn.com, nytimes.com, stackoverflow.com, imdb.com, pinterest.com etc). During testing I haven't found any new issues.

Tested this on Windows 7 x64, Mac 10.13 and Arch Linux.

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8960250 [details]
Bug 1447043 - Export Screenshots 30.1.0 to Firefox;

Approval Request Comment
[Feature/Bug causing the regression]: On a few websites, when the user starts the screenshot flow by clicking to select a region, the selection will be drawn in the wrong place, sometimes off screen. For more details, see [1], [2], [3].
[User impact if declined]: Shot creation flow will seem broken in some cases. 
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, already tested by SV.
[List of other uplifts needed for the feature/fix]: Both patches attached to this bug (one to fix the bug, one to increment the addon version number).
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: The patch is very small, is confined to webextension code, and only affects the selection step in the screenshot creation flow.
[String changes made/needed]: None


[1] https://github.com/mozilla-services/screenshots/issues/4217
[2] https://github.com/mozilla-services/screenshots/issues/3907
[3] https://github.com/mozilla-services/screenshots/issues/3972

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8960250 [details]
Bug 1447043 - Export Screenshots 30.1.0 to Firefox;

screenshots bug fix, beta60+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/760683bdddb4
https://hg.mozilla.org/releases/mozilla-beta/rev/67b517c61b1f

Comment 14

[Cosmin Muntean [:cmuntean], Ecosystem QA - PTO until 18 Sept - ping mcoman for any question]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Firefox	60.0b6, Build ID: 20180322152034

I can confirm that the issue is fixed in latest Beta 60.0b6 build. Tested this on Windows 10 x64, Windows 7 x64, Mac 10.12 and Ubuntu 14.04 x64.