1163231 - Test failures on Aurora due to Pocket button causing toolbar overflow

Status: RESOLVED INCOMPLETE

(Firefox :: Pocket, defect, P3)

Description

[Justin Dolske [:Dolske]]

Ran a Try build before uplifting bug 1161881, to enable pocket, and found bc1 failures on Windows 7 (although, interestingly, not Windows 8):

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

Investigation reveals this is a result of the toolbar being in overflow. Aurora is the devtools branch, and has a number of extra buttons on the navtoolbar. One of the tests is explicitly checking for "toolbar not in overflow" at the beginning (and fails), and some of the other tests are broken because test widgets end up in an unexpected place (the overflow panel).

We could fix this by making the window a little wider, but I don't know if that's going to cause other problems. Some of the tests can be fixed by having them add their widgets farther to the left in the toolbar, so that they don't end up in overflow.

Failing tests:

  browser/components/customizableui/test/...
    browser_943683_migration_test.js
    browser_947987_removable_default.js
    browser_962069_drag_to_overflow_chevron.js
    browser_973641_button_addon.js
    browser_976792_insertNodeInWindow.js
    browser_984455_bookmarks_items_reparenting.js
    browser_985815_propagate_setToolbarVisibility.js
    browser_987177_xul_wrapper_updating.js
    browser_988072_sidebar_events.js

Comment 1

[Justin Dolske [:Dolske]]

(This was actually found when uplifting bug 1155521, which adds Pocket to the toolbar when enabled. We're going to temporarily work around this by disabling Pocket on Aurora. This lets us keep up with the flurry of uplifts, and we'll only need to do a trivial pref flip when the tests are fixed.)

Comment 2

[Drew Willcoxon :adw]

I've narrowed down the browser_943683_migration_test.js failure to these three tests:

[browser_932928_show_notice_when_palette_empty.js]
[browser_934951_zoom_in_toolbar.js]
[browser_943683_migration_test.js]

After the first one, the overflow chevron is present, and then in zoom_in_toolbar.js, the zoom buttons actually are not properly added to the toolbar at all, so that test fails too.  I'll keep debugging.

Comment 3

[Drew Willcoxon :adw]

I'm pretty sure the problem is browser_932928_show_notice_when_palette_empty.js.  With that one test disabled, try looks OK.  bc1 on Windows 7 opt was the problem, and disabling that test makes it green:

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

My hunch is that it's a real problem in CustomizableUI and not that test's fault.  I'm still looking into it, but it seems that the navbar erroneously remains in overflowed mode after the test opens customizing mode, moves all the widgets to the navbar, ends customizing mode, and then resets customization.  When I manually click the overflow chevron after that, the overflow panel is empty.  It's as if CustomizableUI.reset() is not ultimately resetting the navbar's overflowed status.

Comment 4

[Drew Willcoxon :adw]

Er, I misread the results.  Windows 7 hasn't finished yet.  I was looking at XP.

Comment 5

[Drew Willcoxon :adw]

Attachment: disable problematic test, enable Pocket on Dev Edition

Well, fortunately Windows 7 looks OK after all.  There were tons of other mostly known failures though.  To make sure my patch wasn't causing them, I pushed another run that makes no changes at all, and it has the same failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf460d5701b

So I think the thing to do right now is disable browser_932928_show_notice_when_palette_empty.js.  I'm still not sure what the problem is other than what I wrote earlier.  But I'm less confident that it has something to do with customization mode, because even when I force the navbar to keep listening to CustomizableUI's notifications inside customizing mode (normally it doesn't), the overflow chevron still appears later, which messes up the following tests.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8609702 [details] [diff] [review]
disable problematic test, enable Pocket on Dev Edition

Review of attachment 8609702 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser.ini
@@ +46,5 @@
>  [browser_923857_customize_mode_event_wrapping_during_reset.js]
>  [browser_927717_customize_drag_empty_toolbar.js]
> +
> +# Bug 1163231 - Causes failures on Developer Edition on Windows 7.
> +# [browser_932928_show_notice_when_palette_empty.js]

I think it's preferred to use:
skip-if = true # Bug … 

so that the test still shows up as being skipped in test runs and makes it easier to find disabled tests to fix.

See https://mxr.mozilla.org/mozilla-central/search?string=skip-if%20%3D%20true

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8609702 [details] [diff] [review]
disable problematic test, enable Pocket on Dev Edition

r+ with the change MattN suggested.

Note that we'll either need to keep this bug open, or fix the underlying cause in a followup before the next uplift. Otherwise we'll need to land this permanently in m-c (so that it doesn't keep regressing on uplift). I'd be open to doing that, as this isn't a critical test in itself (although I'd be more concerned about there being some kind of real overflow bug that we should fix)...

Interesting that this all seems to be the result of just one test causing something to get messed up. I vaguely recall some (fixed?) Australis bugs with the overflow chevron showing up when it isn't needed, Gijs remembers more?

Comment 8

[:Gijs (out for now; he/him)]

I wouldn't really know offhand what's going on here. The difference with devedition will be the extra buttons (for devtools) in the toolbar and the sizes of the window at which things start to overflow.

Can we disable the test selectively (ie per-channel) instead of wholesale?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/027644ad089b

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 8609702 [details] [diff] [review]
disable problematic test, enable Pocket on Dev Edition

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: No Pocket button on Aurora for June 2nd release
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Enables the Pocket button on Aurora, currently enabled on all other channels. Disables a minor test that had been causing other tests to fail.
[String/UUID change made/needed]: n/a

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8609702 [details] [diff] [review]
disable problematic test, enable Pocket on Dev Edition

I reviewed this change with dolske. The Firefox window needs to shrink to a width smaller than ~735px for overflow to occur on a user's system. This is a change from the current overflow width of ~700px. That seems like a reasonable tradeoff for consistency wrt the availability of this feature across the Firefox channels. Aurora+

Comment 12

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3a13b2f6d14

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/027644ad089b

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information