corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1393574 - fix flexible spacers not being removable in some circumstances, r=jaws

Browse Source 
The goal of this patch is to ensure that:
- in default placements, specials have no unique ids
- in actual placements as stored by CUI, they do
- we reset the counter for those unique ids on reset.
- we re-number specials when building an area (like on startup, or when resetting),
  ensuring that the actual nodes always match the placements for a given area.
- we force saves after resetting, to ensure that the gNewElementCount is always persisted correctly.

This last part will also fix bug 1393661

MozReview-Commit-ID: HAS5J5ZSgB5
This commit is contained in:
Gijs Kruitbosch
2017-09-06 10:02:44 +01:00
parent 6a65edd6e0
commit 7bcdebbdbc
7 changed files with 97 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
browser/base/content/browser.xul
View File

@@ -784,6 +784,7 @@
onclick="BrowserGoHome(event);"
cui-areatype="toolbar"
aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
<toolbarspring cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>
<toolbaritem id="urlbar-container" flex="400" persist="width"
removable="false"
class="chromeclass-location" overflows="false">
@@ -937,6 +938,7 @@
flex="100" persist="width" removable="true">
<searchbar id="searchbar" flex="1"/>
</toolbaritem>
<toolbarspring cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>
<!-- This is a placeholder for the Downloads Indicator. It is visible
during the customization of the toolbar, in the palette, and before

53
browser/components/customizableui/CustomizableUI.jsm
View File

@@ -532,13 +532,7 @@ var CustomizableUIInternal = {
props.get(key) != aProperties[key]) {
throw new Error("An area cannot change the property for '" + key + "'");
}
// XXXgijs for special items, we need to make sure they have an appropriate ID
// so we aren't perpetually in a non-default state:
if (key == "defaultPlacements" && Array.isArray(aProperties[key])) {
props.set(key, aProperties[key].map(x => this.isSpecialWidget(x) ? this.ensureSpecialWidgetId(x) : x ));
} else {
props.set(key, aProperties[key]);
}
props.set(key, aProperties[key]);
}
// Default to a toolbar:
if (!props.has("type")) {
@@ -740,8 +734,12 @@ var CustomizableUIInternal = {
currentNode = currentNode.nextSibling;
}
if (currentNode &&
(currentNode.id == id || this.matchingSpecials(id, currentNode.id))) {
// Fix ids for specials and continue, for correctly placed specials.
if (currentNode && (!currentNode.id || CustomizableUI.isSpecialWidget(currentNode)) &&
this.matchingSpecials(id, currentNode)) {
currentNode.id = id;
}
if (currentNode && currentNode.id == id) {
currentNode = currentNode.nextSibling;
continue;
}
@@ -801,9 +799,10 @@ var CustomizableUIInternal = {
// if one exists. If no palette exists, we just remove the node. If the
// node is not removable, we leave it where it is. However, we can only
// safely touch elements that have an ID - both because we depend on
// IDs, and because such elements are not intended to be widgets
// (eg, titlebar-placeholder elements).
if (node.id && node.getAttribute("skipintoolbarset") != "true") {
// IDs (or are specials), and because such elements are not intended to
// be widgets (eg, titlebar-placeholder elements).
if ((node.id || this.isSpecialWidget(node)) &&
node.getAttribute("skipintoolbarset") != "true") {
if (this.isWidgetRemovable(node)) {
if (palette && !this.isSpecialWidget(node.id)) {
palette.appendChild(node);
@@ -1253,7 +1252,21 @@ var CustomizableUIInternal = {
return !!panels && panels.has(node);
},
_getSpecialIdForNode(aNode) {
if (typeof aNode == "object" && aNode.localName) {
if (aNode.id) {
return aNode.id
}
if (aNode.localName.startsWith("toolbar")) {
return aNode.localName.substring(7);
}
return "";
}
return aNode;
},
isSpecialWidget(aId) {
aId = this._getSpecialIdForNode(aId);
return (aId.startsWith(kSpecialWidgetPfx) ||
aId.startsWith("separator") ||
aId.startsWith("spring") ||
@@ -1261,6 +1274,9 @@ var CustomizableUIInternal = {
},
matchingSpecials(aId1, aId2) {
aId1 = this._getSpecialIdForNode(aId1);
aId2 = this._getSpecialIdForNode(aId2);
return this.isSpecialWidget(aId1) &&
this.isSpecialWidget(aId2) &&
aId1.match(/spring|spacer|separator/)[0] == aId2.match(/spring|spacer|separator/)[0];
@@ -2567,8 +2583,9 @@ var CustomizableUIInternal = {
gSeenWidgets.add(widgetId);
}
}
if (gSeenWidgets.size) {
if (gSeenWidgets.size || gNewElementCount) {
gDirty = true;
this.saveState();
}
gResetting = false;
@@ -2581,6 +2598,7 @@ var CustomizableUIInternal = {
gUIStateBeforeReset.uiDensity = Services.prefs.getIntPref(kPrefUIDensity);
gUIStateBeforeReset.autoTouchMode = Services.prefs.getBoolPref(kPrefAutoTouchMode);
gUIStateBeforeReset.currentTheme = LightweightThemeManager.currentTheme;
gUIStateBeforeReset.newElementCount = gNewElementCount;
} catch (e) { }
this._resetExtraToolbars();
@@ -2590,6 +2608,7 @@ var CustomizableUIInternal = {
Services.prefs.clearUserPref(kPrefUIDensity);
Services.prefs.clearUserPref(kPrefAutoTouchMode);
LightweightThemeManager.currentTheme = null;
gNewElementCount = 0;
log.debug("State reset");
// Reset placements to make restoring default placements possible.
@@ -2661,6 +2680,7 @@ var CustomizableUIInternal = {
let currentTheme = gUIStateBeforeReset.currentTheme;
let uiDensity = gUIStateBeforeReset.uiDensity;
let autoTouchMode = gUIStateBeforeReset.autoTouchMode;
gNewElementCount = gUIStateBeforeReset.newElementCount;
// Need to clear the previous state before setting the prefs
// because pref observers may check if there is a previous UI state.
@@ -2705,7 +2725,12 @@ var CustomizableUIInternal = {
if (typeof aWidget == "string") {
widgetId = aWidget;
} else {
widgetId = aWidget.id;
if (!aWidget.id &&
!["toolbarspring", "toolbarspacer", "toolbarseparator"].includes(aWidget.nodeName)) {
throw new Error("No nodes without ids that aren't special widgets should ever come into contact with CUI");
}
// Use "spring" / "spacer" / "separator" for special widgets without ids
widgetId = aWidget.id || aWidget.nodeName.substring(7 /* "toolbar".length */);
widgetNode = aWidget;
}
let provider = this.getWidgetProvider(widgetId);

2
browser/components/customizableui/content/toolbar.xml
View File

@@ -197,7 +197,7 @@
}
}
let orderedPlacements = CustomizableUI.getWidgetIdsInArea(this.id);
return orderedPlacements.filter((x) => currentWidgets.has(x)).join(",");
return orderedPlacements.filter(w => currentWidgets.has(w)).join(",");
]]></getter>
<setter><![CDATA[
// Get list of new and old ids:

2
browser/components/customizableui/test/browser.ini
View File

@@ -137,6 +137,7 @@ skip-if = os == "mac"
[browser_1161838_inserted_new_default_buttons.js]
[browser_allow_dragging_removable_false.js]
[browser_bootstrapped_custom_toolbar.js]
[browser_currentset_post_reset.js]
[browser_customizemode_contextmenu_menubuttonstate.js]
[browser_customizemode_uidensity.js]
[browser_exit_background_customize_mode.js]
@@ -150,6 +151,7 @@ skip-if = os == "mac"
[browser_panelUINotifications_fullscreen_noAutoHideToolbar.js]
tags = fullscreen
[browser_panelUINotifications_multiWindow.js]
[browser_remove_customized_specials.js]
[browser_switch_to_customize_mode.js]
[browser_synced_tabs_menu.js]
[browser_backfwd_enabled_post_customize.js]

1
browser/components/customizableui/test/browser_1096763_seen_widgets_post_reset.js
View File

@@ -14,7 +14,6 @@ add_task(async function() {
ok(bsPass.gSeenWidgets.has(BUTTONID), "Widget should be seen after createWidget is called.");
CustomizableUI.reset();
ok(bsPass.gSeenWidgets.has(BUTTONID), "Widget should still be seen after reset.");
ok(!Services.prefs.prefHasUserValue(kPrefCustomizationState), "Pref shouldn't be set right now, because that'd break undo.");
CustomizableUI.addWidgetToArea(BUTTONID, CustomizableUI.AREA_NAVBAR);
gCustomizeMode.removeFromArea(document.getElementById(BUTTONID));
let hasUserValue = Services.prefs.prefHasUserValue(kPrefCustomizationState);

25
browser/components/customizableui/test/browser_currentset_post_reset.js Normal file
View File

@@ -0,0 +1,25 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */
"use strict";
function checkSpacers() {
let navbarWidgets = CustomizableUI.getWidgetIdsInArea("nav-bar");
let currentSetWidgets = document.getElementById("nav-bar").currentSet.split(",");
navbarWidgets = navbarWidgets.filter(w => CustomizableUI.isSpecialWidget(w));
currentSetWidgets = currentSetWidgets.filter(w => CustomizableUI.isSpecialWidget(w));
Assert.deepEqual(navbarWidgets, currentSetWidgets, "Should have the same 'special' widgets in currentset and placements");
}
/**
* Check that after a reset, the currentset property correctly deals with flexible spacers.
*/
add_task(async function() {
await startCustomizing();
checkSpacers();
CustomizableUI.addWidgetToArea("spring", "nav-bar", 4 /* Insert before the last extant spacer */);
await gCustomizeMode.reset();
checkSpacers();
await endCustomizing();
});

28
browser/components/customizableui/test/browser_remove_customized_specials.js Normal file
View File

@@ -0,0 +1,28 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */
"use strict";
/**
* Check that after a reset, we can still drag special nodes in customize mode
*/
add_task(async function() {
await startCustomizing();
CustomizableUI.addWidgetToArea("spring", "nav-bar", 5);
await gCustomizeMode.reset();
let springs = document.querySelectorAll("#nav-bar toolbarspring");
let lastSpring = springs[springs.length - 1];
let expectedPlacements = CustomizableUI.getWidgetIdsInArea("nav-bar");
info("Placements before drag: " + expectedPlacements.join(","));
let lastItem = document.getElementById(expectedPlacements[expectedPlacements.length - 1]);
simulateItemDrag(lastSpring, lastItem);
expectedPlacements.splice(expectedPlacements.indexOf(lastSpring.id), 1);
expectedPlacements.splice(expectedPlacements.length - 1, 0, lastSpring.id);
let actualPlacements = CustomizableUI.getWidgetIdsInArea("nav-bar");
// Log these separately because Assert.deepEqual truncates the stringified versions...
info("Actual placements: " + actualPlacements.join(","));
info("Expected placements: " + expectedPlacements.join(","));
Assert.deepEqual(expectedPlacements, actualPlacements, "Should be able to move spring");
await gCustomizeMode.reset();
await endCustomizing();
});