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

NEW
Unassigned

Status

()

Firefox
Toolbars and Customization
2 years ago
2 years ago

People

(Reporter: JuliaC, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(e10s-, firefox45 affected, firefox46 affected, firefox47 affected, firefox48 affected, firefox49 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Created attachment 8743284 [details]
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.
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?
Flags: needinfo?(iulia.cristescu)
(Reporter)

Comment 2

2 years ago
(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.
Keywords: regression, regressionwindow-wanted
Summary: Tools does not arange propperly in pallette after New Non-e10s tool is gone from panel menu → Tools does not arrange properly in palette after New Non-e10s tool is gone from panel menu
(Reporter)

Comment 3

2 years ago
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
Flags: needinfo?(iulia.cristescu)
Keywords: regressionwindow-wanted

Updated

2 years ago
tracking-e10s: --- → -
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. :-\
status-firefox45: unaffected → affected
status-firefox46: unaffected → affected
status-firefox49: --- → affected
Keywords: regression
Summary: Tools does not arrange properly in palette after New Non-e10s tool is gone from panel menu → Buttons do not wrap properly in the main menu panel when we have to move wide widgets over/across non-existing widgets
You need to log in before you can comment on or make changes to this bug.