1266089 - Buttons do not wrap properly in the main menu panel when we have to move wide widgets over/across non-existing widgets

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Attachment: screenshot showing the issue.png

[Affected versions]:
- latest Nightly 
- latest Aurora

[Affected platforms]:
- Windows 10 x64
- Windows 7 x64
- Windows 8.1 x86
- Ubuntu 14.04 x86
- Mac OS X 10.10.5

[Steps to reproduce]:
1. Launch Firefox with a new profile.
2. Visit about:preferences and disable e10s. 
3. After Firefox restarts, go to Hamburger Menu > Customize.
4. Drag one of the Navigation Toolbar buttons to the Panel Menu, to the second row.
5. Click the Exit Customize button.
6. Click again the Hamburger Menu 
- inspect the Panel Menu

[Expected result]:
- All the Panel Menu items are properly arranged.

[Actual result]:
- The second row from the Panel Menu isn't completely populated.

[Regression range]:
- To be investigated as soon as possible.

[Additional notes]:
- See the attached screenshot.

Comment 1

[:Gijs (he/him)]

How sure are you that this is a regression, and not just an issue only on aurora/nightly because of build-time switches? As in, have you checked this really didn't happen in earlier versions of Nightly?

Comment 2

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to :Gijs Kruitbosch from comment #1)
> How sure are you that this is a regression, and not just an issue only on
> aurora/nightly because of build-time switches? As in, have you checked this
> really didn't happen in earlier versions of Nightly?

Hello! I didn't yet have the time to see whether this is a regression or not, but I'll check it as soon as possible. I'm leaving the ni? flag in place as a reminder.

Comment 3

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Here's the regression range for this issue:
 * Last good revision: f48352b311eb120a09347efce60ae9045401858c
 * First bad revision: 19876a153a009b457900c82f27efbc398ad19413
 * Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f48352b311eb120a09347efce60ae9045401858c&tochange=19876a153a009b457900c82f27efbc398ad19413
 * Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1223573

Comment 4

[:Gijs (he/him)]

Some notes:

1) Hello being an add-on triggered this, presumably because it triggered issues with saving the CUI state because of how it registers buttons etc. Otherwise we wouldn't have saved the state after a clean run of Firefox, but if you try the following, you should get a different window, if you get a window at all - I expect it isn't really a regression:

a) clean profile
b) open hamburger menu, right click any button you click, click "remove from menu"
c) turn off e10s
d) after the restart, go to customize mode
e) move a button from the palette between the zoom and edit controls


2) you can reproduce this also by running this from a browser console on a clean profile:

CustomizableUI.createWidget({id: "some-non-existing-widget", label: "foo"});
CustomizableUI.addWidgetToArea("some-non-existing-widget", "PanelUI-contents", 3);
CustomizableUI.destroyWidget("some-non-existing-widget");
CustomizableUI.addWidgetToArea("open-file-button", "PanelUI-contents", 1);

3) This means it will affect add-ons that get removed and the like. We should fix it even though it doesn't affect non-e10s for this specific (builtin) issue.

The code here:

  // This function is called whenever an item gets moved in the menu panel. It
  // adjusts the position of widgets within the panel to prevent "gaps" between
  // wide widgets that could be filled up with single column widgets
  adjustPosition: function(aWidgetId, aMoveForwards) {
    // Make sure that there are n % columns = 0 narrow buttons before the widget.
    let placementIndex = gPanelPlacements.indexOf(aWidgetId);
    let prevSiblingCount = 0;
    let fixedPos = null;
    while (placementIndex--) {
      let thisWidgetId = gPanelPlacements[placementIndex];
      if (gWideWidgets.has(thisWidgetId)) {
        continue;
      }
      let widgetStatus = this.checkWidgetStatus(thisWidgetId);
      if (!widgetStatus) {
        continue;
      }
      if (widgetStatus == "public-only") {
        fixedPos = !fixedPos ? placementIndex : Math.min(fixedPos, placementIndex);
        prevSiblingCount = 0;
      } else {
        prevSiblingCount++;
      }
    }

    if (fixedPos !== null || prevSiblingCount % CustomizableUI.PANEL_COLUMN_COUNT) {
      let desiredPos = (fixedPos !== null) ? fixedPos : gPanelPlacements.indexOf(aWidgetId);
      let desiredChange = -(prevSiblingCount % CustomizableUI.PANEL_COLUMN_COUNT);
      if (aMoveForwards && fixedPos == null) {
        // +1 because otherwise we'd count ourselves:
        desiredChange = CustomizableUI.PANEL_COLUMN_COUNT + desiredChange + 1;
      }
      desiredPos += desiredChange;
      CustomizableUI.moveWidgetWithinArea(aWidgetId, desiredPos);
    }


breaks in this case because it evaluates the amount by which to move based on the widgets *before* the wide widget, and then moves the widget forward. While the code that determines the amount by which to move correctly deals with widgets that are only visible in non-private-browsing mode, and widgets that are gone, it then doesn't take that into account when actually moving the widget, as it needs to still increment the amount of change by the number of dead/non-existing widgets to move over.


Finally... it doesn't look like we do anything when a widget is destroyed or created/restored. So even if we fix the code to deal with this at the point where things are moved, we'll then get it wrong again if either some of the widgets get destroyed or some previously-destroyed widgets are recreated. :-\

Comment 5

[:Gijs (he/him)]

We got rid of this version of the hamburger panel.