1246455 - mozscreenshots: Some captured images have the browser in an incorrect state

Status: RESOLVED FIXED

(Testing :: mozscreenshots, defect)

Description

[Matthew N. [:MattN]]

Since mozscreenshots changes UI so quickly, not necessarily waiting for the previous change (sometimes because there aren't good events), the browser can get captured and sometimes stuck in unusual states. For example, when toggling the WindowSize and TabsInTitlebar immediately after the other, the window contents can become separated from the window frame. I don't understand all of the causes but they seem like things that would be very rare in the real world with our default UI as users can't toggle these things like mozscreenshots is without returning to the event loop in between.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1246455 - mozscreenshots: Open the tabs to be pinned with the other opening tabs. r=kitcambridge

Opening the two tabs after leads to them animating open with overflow and causes inconsistency.

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

Comment 2

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1246455 - mozscreenshots: Wait for the @inFullscreen change when toggling fullScreen. r=kitcambridge

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

Comment 3

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1246455 - mozscreenshots: Change the lightweight theme colors to be a bit off from black and white. r=kitcambridge

This should make it easier to detect when we're picking up a default black/white value by mistake.

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

Comment 4

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1246455 - mozscreenshots: Add a delay between applying configurations. r=kitcambridge

Avoid conflicts when configurations try to apply at the same time. e.g WindowSize and TabsInTitlebar.
Also bump requestLongerTimeout for the longer duration or this will timeout on OS X.

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

Comment 5

[Matthew N. [:MattN]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48508a0072e2

Comment 6

[Lina Butler (ex-Mozilla)]

Comment on attachment 8716702 [details]
MozReview Request: Bug 1246455 - mozscreenshots: Add a delay between applying configurations. r=kitcambridge

https://reviewboard.mozilla.org/r/33917/#review30633

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:154
(Diff revision 1)
> +      // try to apply at the same time. e.g WindowSize and TabsInTitlebar

This is because animations for those might still be in progress, even though `applyConfig` has returned, right?

Comment 7

[Lina Butler (ex-Mozilla)]

Comment on attachment 8716700 [details]
MozReview Request: Bug 1246455 - mozscreenshots: Wait for the @inFullscreen change when toggling fullScreen. r=kitcambridge

https://reviewboard.mozilla.org/r/33913/#review30637

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/WindowSize.jsm:66
(Diff revision 1)
> +    return wantsFS == browserWindow.document.documentElement.hasAttribute("inFullscreen");

Do we need to observe `fullscreen-painted`, too?

Comment 8

[Lina Butler (ex-Mozilla)]

Comment on attachment 8716699 [details]
MozReview Request: Bug 1246455 - mozscreenshots: Open the tabs to be pinned with the other opening tabs. r=kitcambridge

https://reviewboard.mozilla.org/r/33911/#review30639

Comment 9

[Lina Butler (ex-Mozilla)]

Comment on attachment 8716701 [details]
MozReview Request: Bug 1246455 - mozscreenshots: Change the lightweight theme colors to be a bit off from black and white. r=kitcambridge

https://reviewboard.mozilla.org/r/33915/#review30657

Comment 10

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/33917/#review30633

> This is because animations for those might still be in progress, even though `applyConfig` has returned, right?

Most likely but I can't say for sure.

Comment 11

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/33913/#review30637

> Do we need to observe `fullscreen-painted`, too?

I believe that's only for DOM fullscreen, not native fullscreen.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d46626c3d078
https://hg.mozilla.org/integration/fx-team/rev/b396e91f02a6
https://hg.mozilla.org/integration/fx-team/rev/76b880393d82
https://hg.mozilla.org/integration/fx-team/rev/59049fe7a107

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d46626c3d078
https://hg.mozilla.org/mozilla-central/rev/b396e91f02a6
https://hg.mozilla.org/mozilla-central/rev/76b880393d82
https://hg.mozilla.org/mozilla-central/rev/59049fe7a107