CustomizableUI.reset() moves buttons to overflow panel

NEW
Unassigned

Status

()

Firefox
Toolbars and Customization
10 months ago
10 months ago

People

(Reporter: arai, Unassigned)

Tracking

({regressionwindow-wanted})

Trunk
regressionwindow-wanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected)

Details

(Reporter)

Description

10 months ago
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)
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?)
Flags: needinfo?(arai.unmht)
(Reporter)

Comment 2

10 months ago
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
Flags: needinfo?(arai.unmht)
(Reporter)

Comment 3

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

Updated

10 months ago
Blocks: 1322953
Keywords: regression
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.
Flags: needinfo?(dao+bmo)

Comment 5

10 months ago
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).
Flags: needinfo?(dao+bmo)
(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)?
Flags: needinfo?(dao+bmo)

Comment 7

10 months ago
(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
Flags: needinfo?(dao+bmo)

Updated

10 months ago
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Version: unspecified → Trunk

Comment 8

10 months ago
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.
Blocks: 1323001
status-firefox50: unaffected → ---
status-firefox51: unaffected → ---
status-firefox52: unaffected → ---
Keywords: regression → regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.