1323607 - CustomizableUI.reset() moves buttons to overflow panel

Status: NEW

(Firefox :: Toolbars and Customization, defect)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1323276.

Steps to Reproduce:

  1. open Nightly
  2. Turn "Enable browser chrome and add-on debugging toolboxes" on
  3. Open Scratchpad
  4. evaluate the following code:

    let {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {});
    let origPlacement = CustomizableUI.getPlacementOfWidget("new-window-button");
    CustomizableUI.addWidgetToArea("new-window-button",
                                   CustomizableUI.AREA_NAVBAR,
                                   0);
    CustomizableUI.ensureWidgetPlacedInWindow("new-window-button", window);
    setTimeout(() => CustomizableUI.reset(), 1000);

Expected result:

  New Window button is added to the beginning of the toolbar, and removed after 1 second

Actual result:

  New Window button is added to the beginning of the toolbar, and removed after 1 second, and all buttons except location bar is moved to overflow panel
  (Search field, Bookmark this page, Downloads, Home, Pocket)



Another steps to reproduce:

  Run the following command at revision 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8:

    ./mach mochitest --keep-open=true browser/base/content/test/general/browser_newWindowDrop.js

  This test adds New Window button at startup, and calls CustomizableUI.reset() while cleanup.
  CustomizableUI.reset() moves buttons to overflow panel, and subsequent test fails because of it (see bug 1323276 comment #0)

Comment 1

[:Gijs (he/him)]

Is this reproducible on non-OS X? Does backing out bug 1322953 and bug 1323001 "fix" this? (ie does it reproduce on a nightly from last week?)

Comment 2

[Tooru Fujisawa [:arai]]

yeah, indeed, the regression range is
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5cb21793896161601a065eb0aa21dbfe570fe4f3&tochange=0c650d7f26091598dc2aa96088b9756914a63ff1

I confirmed this issue also on debian with xcfe with nightly 2016-12-14

will try backing out each changes

Comment 3

[Tooru Fujisawa [:arai]]

ah... apparently it's clear without bisecting between them.
it's from https://hg.mozilla.org/integration/mozilla-inbound/rev/06f516403cf4 on osx.

Comment 4

[:Gijs (he/him)]

This sounds like the CSS changes actually revealed real breakage (that is likely to happen in other circumstances than the neatly reproducible testcase in comment #0), and so I would prefer to back out bug 1322953 until we have an actual fix for this problem, rather than just working around it in the automated tests that showed the problem existed.

Comment 5

[Dão Gottwald [:dao]]

Backing out bug 1322953 won't prevent the breakage from happening in other circumstances given that:
1. Windows and Linux didn't have that padding in the first place but passed the testcase from comment 0,
2. and yet bug 1323001 revealed the same problem on Linux (I had backed it out before I got results on Linux, have to go back to check if it was affected too).

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> Backing out bug 1322953 won't prevent the breakage from happening in other
> circumstances given that:
> 1. Windows and Linux didn't have that padding in the first place but passed
> the testcase from comment 0,

How does this imply that they would be affected by the problem?

> 2. and yet bug 1323001 revealed the same problem on Linux (I had backed it
> out before I got results on Linux, have to go back to check if it was
> affected too).

I'm a bit lost. You backed out bug 1323001 and then relanded it without the changes that you split off into bug 1322953, right? Were there other changes you omitted? If the second time, 1323001 stayed in the tree without breaking on Linux, how are you also claiming you're seeing the same problem on Linux (but also that you'd need to "go back to check" if it was affected)?

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Backing out bug 1322953 won't prevent the breakage from happening in other
> > circumstances given that:
> > 1. Windows and Linux didn't have that padding in the first place but passed
> > the testcase from comment 0,
> 
> How does this imply that they would be affected by the problem?

It implies that #navbar-customization-target having no padding is neither sufficient for revealing the underlying bug nor necessary (my second point). Things are apparently more complex.  

> > 2. and yet bug 1323001 revealed the same problem on Linux (I had backed it
> > out before I got results on Linux, have to go back to check if it was
> > affected too).
> 
> I'm a bit lost. You backed out bug 1323001 and then relanded it without the
> changes that you split off into bug 1322953, right? Were there other changes
> you omitted? If the second time, 1323001 stayed in the tree without breaking
> on Linux, how are you also claiming you're seeing the same problem on Linux

Just like bug 1322953, disabling browser_newWindowDrop.js is what allowed it to reland. See the dependency on bug 1323276.

> (but also that you'd need to "go back to check" if it was affected)?

This was a typo, I meant I had to go back and see if Windows was affected too. I did that now. Both Linux and Windows were affected: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=52e782555b4b

Comment 8

[Dão Gottwald [:dao]]

We don't really know what releases might be affected by this and under what circumstances. A way to test if this is a regression and when it regressed might be to apply the patches from bug 1322953 or bug 1323001 on top of older revisions and see if the STR from comment 0 still apply. Maybe this can also be done with userstyles and the browser console rather than building Firefox.

However I'm not quite sure if that's good use of anyone's time. It might be more effective to dig into  CustomizableUI.reset and try to figure out what's going on there.