Is there any place we can learn more about the reason behind this? (turn the unique Back button to a normal one that we can see in any other browser.) --- I'm actually looking for the "plan" on this "visual refresh" activity for a while, see a lot of results (like this bug) but usually with no reason explained. As a volunteer, it's really hard for me in this situation to know when or where I should provide feedback, or help you explain (promote) the idea to local community members. I do think it would be better to publish something, on wiki or blog or any place, to tell the story about "why." (Or perhaps you already did? please share the URL?)

(In reply to Po-chiang Chao [:bobchao] from comment #4) > Is there any place we can learn more about the reason behind this? (turn the > unique Back button to a normal one that we can see in any other browser.) > > --- > > I'm actually looking for the "plan" on this "visual refresh" activity for a > while, see a lot of results (like this bug) but usually with no reason > explained. As a volunteer, it's really hard for me in this situation to know > when or where I should provide feedback, or help you explain (promote) the > idea to local community members. > > I do think it would be better to publish something, on wiki or blog or any > place, to tell the story about "why." > (Or perhaps you already did? please share the URL?) Your question would be better suited for the firefox-dev mailing list. Bugzilla's primary purpose is issue tracking and organizing engineering work.

I forwarded the review request to Gijs since I'm not sure (1) if we need to add some profile migration code for such changes these days and (2) if it's sensible to land this before 57's nightly cycle.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review157748 The invision spec suggests that the back/forward/refresh items should be 1 block of items, and as-written this code will result in the refresh/stop button dropping out of the toolbar for people with an existing profile (because it won't be in their saved navbar placements and, unlike the back/fwd buttons, it is removable). (In reply to Dão Gottwald [::dao] from comment #9) > I forwarded the review request to Gijs since I'm not sure (1) if we need to > add some profile migration code for such changes these days Yes. > and (2) if it's > sensible to land this before 57's nightly cycle. I don't know enough about how you're restructuring the navbar to comment on this.

(In reply to :Gijs from comment #10) > Comment on attachment 8881124 [details] > Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the > location bar container to the start of the toolbar along with the Home button > > https://reviewboard.mozilla.org/r/152424/#review157748 > > The invision spec suggests that the back/forward/refresh items should be 1 > block of items ... together with the home button, even. FWIW, I am not convinced either way whether or not this is a good idea, but as-is there's no discussion in the bug that clarifies that the spec is wrong, or, if they're not supposed to be used together, why the back/fwd buttons *do* stay together and/or why we're not moving the home button while we're here. I suggest you discuss it with UX and come to explicit agreement and log it in the bug.

(In reply to :Gijs from comment #10) > > and (2) if it's > > sensible to land this before 57's nightly cycle. > > I don't know enough about how you're restructuring the navbar to comment on > this. There's no "restucturing the navbar" beyond this bug as far as photon-visual is concerned. (In reply to :Gijs from comment #11) > > The invision spec suggests that the back/forward/refresh items should be 1 > > block of items > > ... together with the home button, even. FWIW, I am not convinced either way > whether or not this is a good idea, but as-is there's no discussion in the > bug that clarifies that the spec is wrong, or, if they're not supposed to be > used together, why the back/fwd buttons *do* stay together and/or why we're > not moving the home button while we're here. I suggest you discuss it with > UX and come to explicit agreement and log it in the bug. I can only assume this is a mistake. Making the home button irremovable seems like a nonstarter, I don't know why anybody would want that. Stephen, can you please clarify?

UK92, FYI: 1. The invision spec is here: https://mozilla.invisionapp.com/share/MRBK1MZF7#/screens/229940647 At least I think this is what Gijs was referring to, and more specifically he was probably referring to Back/Forward/Reload/Home being labeled "Navigation Group" there. I am not interpreting this in the way Gijs did; I don't think that label in the mockup has anything to do with customization. For now let's assume that you did the right thing in your patch. 2. The migration, i.e. adding the buttons to the right place in existing profiles, will need to happen in _migrateUI in browser/components/nsBrowserGlue.js.

(In reply to :Gijs from comment #11) > (In reply to :Gijs from comment #10) > > Comment on attachment 8881124 [details] > > Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the > > location bar container to the start of the toolbar along with the Home button > > > > https://reviewboard.mozilla.org/r/152424/#review157748 > > > > The invision spec suggests that the back/forward/refresh items should be 1 > > block of items > > ... together with the home button, even. FWIW, I am not convinced either way > whether or not this is a good idea, but as-is there's no discussion in the > bug that clarifies that the spec is wrong, or, if they're not supposed to be > used together, why the back/fwd buttons *do* stay together and/or why we're > not moving the home button while we're here. I suggest you discuss it with > UX and come to explicit agreement and log it in the bug. Sorry for the confusion. The labels on that page aren't intended to be an indication of fixed structural groupings; they are just there to describe the default state and position of things.

(In reply to Stephen Horlander [:shorlander] from comment #14) > Sorry for the confusion. The labels on that page aren't intended to be an > indication of fixed structural groupings; they are just there to describe > the default state and position of things. Can any of the items be removed from the nav bar entirely? I assume at least the back forward button can't be, but it's not clear to me what should happen with refresh/stop. If the home button stays separate, I guess it can stay removable, too? I'm bringing this up because the customization spec ( https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252091_Customization ) explicitly says "Disabled icons indicate those that cannot be removed from the toolbar", and the back/fwd/refresh/stop buttons are disabled in that image (the home button isn't). I see 2 issues with this: 1) it seems people who don't click refresh/stop a lot were happier with the previous placement and would like to be able to remove it. Unlike back/forward, I think these are non-critical, so removing them doesn't "break" the user's browser. 2) items that can't be removed cannot currently be rearranged between themselves. What I mean is, while the user can move other items around non-removable items (and thus alter the visual position of the non-removable item by rearranging the removable items around them), you can't drag non-removable items and so it isn't possible (within the user interface) to change the order of 2 non-removable items (like the urlbar and the back/fwd buttons). As a consequence, if the back/fwd button and refresh/stop button were both non-removable, then the user would not be able to change the order of the back/fwd, refresh/stop and url bar items. They could put loads of stuff in between them, but not change their order. I don't think that's a shippable state, and so I would prefer that the refresh/stop buttons become removable (which is what the current patch does), at least for now.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158042 ::: browser/base/content/browser.xul:781 (Diff revision 1) > - <toolbaritem id="stop-reload-button" > + <toolbaritem id="stop-reload-button" > - removable="false" overflows="false" > + removable="true" overflows="false" > - class="chromeclass-toolbar-additional"> > + class="chromeclass-toolbar-additional"> If this item is removable, there needs to be a label of some kind, so that it displays a label in customize mode when you move it to the panel or palette. I suppose whether or not the item should overflow into the overflow button on narrow windows is another question for UX...

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158228 Notes: 1) please remove the unrelated whitespace fixes from the l10n file 2) labeling the button 'refresh' permanently when it's in the panel probably doesn't really make sense for the cases where it displays the 'stop' icon. I expect we can make the code that swaps the icon around also swap the label. 3) the migration code is tricky... how this works is, we increment the counter and we simply check on each startup if the profile has seen a build with a counter >= the value in the code we're running right now, and then we run any and all migrations that apply. But this is a once-only process. We don't want to do this migration until 57, except maybe in Nightly. We can't just add another migration at the end right now, because it will also run when MOZ_PHOTON_THEME is not defined (ie when 56 comes off nightly), and then the ids you're adding won't exist as customizable items and it won't work right if the user resets to default before 57. I think we need a new photon-specific thing that somehow checks whether this is the first time the profile is on photon (and removes the pref it checks when it's run without MOZ_PHOTON_THEME defined) -- either that or we postpone all these changes until the start of the 57 cycle. For inspiration about the logic here, perhaps https://hg.mozilla.org/releases/mozilla-release/rev/2e786892906e is helpful - though because we no longer use the XUL rdf store, the actual changes to the persisted state will have to happen via CUI and/or its pref rather than the rdf code. ::: browser/components/nsBrowserGlue.js:2059 (Diff revision 3) > + CustomizableUI.addListener({ > + onAreaNodeRegistered: function(aArea, aContainer) { > + if(aArea == CustomizableUI.AREA_NAVBAR) { > + CustomizableUI.addWidgetToArea("back-forward-button", CustomizableUI.AREA_NAVBAR, 0); > + CustomizableUI.addWidgetToArea("stop-reload-button", CustomizableUI.AREA_NAVBAR, 1); > + CustomizableUI.addWidgetToArea("home-button", CustomizableUI.AREA_NAVBAR, 2); > + > + CustomizableUI.removeListener(this); > + } > + } This is clever, but unfortunately this code runs after the toolbar is 'built' (so nodes are rearranged) by CustomizableUI. This will result in first removing the nodes, and then re-adding them. Admittedly, only on the run where we migrate, but still... Can you check if we can instead check for, and if present parse and manipulate the browser.uiCustomization.state pref? ::: browser/components/nsBrowserGlue.js:2061 (Diff revision 5) > } > > + if (currentUIVersion < 49) { > + CustomizableUI.addListener({ > + onAreaNodeRegistered: function(aArea, aContainer) { > + if(aArea == CustomizableUI.AREA_NAVBAR) { Nit: space after 'if' ::: browser/components/nsBrowserGlue.js:2067 (Diff revision 5) > + try { > + CustomizableUI.addWidgetToArea("back-forward-button", CustomizableUI.AREA_NAVBAR, 0); > + CustomizableUI.addWidgetToArea("stop-reload-button", CustomizableUI.AREA_NAVBAR, 1); > + CustomizableUI.addWidgetToArea("home-button", CustomizableUI.AREA_NAVBAR, 2); > + } catch (ex) { > + // Some widgets are MOZ_PHOTON_THEME only How does this throw, exactly?

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158242 ::: browser/base/content/browser.xul:1074 (Diff revision 5) > label="&downloads.label;" > removable="true" > cui-areatype="toolbar" > tooltip="dynamic-shortcut-tooltip"/> > > <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional" Unfortunately, during Australis we found that moving nodes around was not free (mostly because of XBL bindings being destroyed + recreated). I think we should ifdef this copy of the home-button out, and add another ifdef'd photon copy before the urlbar container, just like we've done for the other buttons.

(In reply to :Gijs from comment #21) > 2) labeling the button 'refresh' permanently when it's in the panel probably > doesn't really make sense for the cases where it displays the 'stop' icon. I > expect we can make the code that swaps the icon around also swap the label. Note that we should not use "refresh" at all. We've called this "reload" for ages (e.g. it's still used in tooltips, context menus, and on support.mozilla.org) and should stick to that. > 3) the migration code is tricky... how this works is, we increment the > counter and we simply check on each startup if the profile has seen a build > with a counter >= the value in the code we're running right now, and then we > run any and all migrations that apply. But this is a once-only process. We > don't want to do this migration until 57, except maybe in Nightly. We can't > just add another migration at the end right now, because it will also run > when MOZ_PHOTON_THEME is not defined (ie when 56 comes off nightly), and > then the ids you're adding won't exist as customizable items and it won't > work right if the user resets to default before 57. Let's just land this at the beginning of the 57 cycle (August 7). We can work on getting this patch ready to land and then let it sit here for a few weeks before actually landing it.

(In reply to :Gijs from comment #15) > 1) it seems people who don't click refresh/stop a lot were happier with the > previous placement and would like to be able to remove it. Unlike > back/forward, I think these are non-critical, so removing them doesn't > "break" the user's browser. > 2) items that can't be removed cannot currently be rearranged between > themselves. What I mean is, while the user can move other items around > non-removable items (and thus alter the visual position of the non-removable > item by rearranging the removable items around them), you can't drag > non-removable items and so it isn't possible (within the user interface) to > change the order of 2 non-removable items (like the urlbar and the back/fwd > buttons). Yeah, I agree. I don't see any reason why we should not allow removal (or movement) of the reload/stop button. We do want to anchor what we consider core components (i.e. stuff that would break the core browsing experience if you removed it). Which is: [Back][Forward][URL Bar][Overflow][Menu]. I will ask Aaron to update the spec to reflect that.

I split up commit for easier testing, and also add photon specific migrate code based on Gijs idea.

(In reply to Stephen Horlander [:shorlander] from comment #24) > We do want to anchor what we consider core components (i.e. stuff that would > break the core browsing experience if you removed it). Which is: > [Back][Forward][URL Bar][Overflow][Menu]. Should Back/Forward be separate items or a grouped one? I.e. should moving other items between Back and Forward be allowed?

They should be separate items, but that's not currently reflected in the spec for customize mode. I'll update.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158642 (In reply to UK92 from comment #30) > I split up commit for easier testing, and also add photon specific migrate > code based on Gijs idea. I'm sorry to go back and forth on this, but if Dão thinks we should just postpone landing this until the 57 cycle then we can probably go with the previous approach, which will be simpler (though we might need to bitrot the exact ui migration number/version/thingy if other things land between now and then). Based on the recent comments we could also just permanently move the home button and remove some of the other ifdefs (for back/forward/refresh/stop) while we're here anyway, and make the back/forward buttons individual items. This will also simplify the changes in CustomizableUI.jsm. Finally, you will also need to update BrowserUITelemetry.jsm to match the new navbar layout.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158772 ::: browser/base/content/browser.xul (Diff revision 8) > <hbox id="nav-bar-customization-target" flex="1"> > - <toolbaritem id="urlbar-container" flex="400" persist="width" > - removable="false" > - class="chromeclass-location" overflows="false"> > - <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > + <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > - removable="false" overflows="false" We should keep the back/fwd buttons as removable=false and overflows=false to prevent them disappearing / being movable, see comment 24. ::: browser/base/content/browser.xul:777 (Diff revision 8) > - command="Browser:ForwardOrForwardDuplicate" > + command="Browser:ForwardOrForwardDuplicate" > - onclick="checkForMiddleClick(this, event);" > + onclick="checkForMiddleClick(this, event);" > - tooltip="forward-button-tooltip" > + tooltip="forward-button-tooltip" > - context="backForwardMenu"/> > + context="backForwardMenu"/> > - <toolbaritem id="stop-reload-button" > - removable="false" overflows="false" > + <toolbaritem id="stop-reload-button" class="chromeclass-toolbar-additional" > + title="&reloadButton.label;" I think you need to use 'label' rather than 'title' - right now there's no label when you pin this button into the overflow panel. I also still think we should update the button label to say 'stop' when that is the correct label (again, for the benefit of the panel - of course, in the customize mode palette it won't matter very much). You can do this from within the CombinedStopReload object defined at https://dxr.mozilla.org/mozilla-central/rev/d9a144b7b6d994fc9a497c53b13f51a2a654d85e/browser/base/content/browser.js#4909 .

Comment on attachment 8882210 [details] Bug 1363485 - Add migration to new navbar layout https://reviewboard.mozilla.org/r/153316/#review158774 This looks alright for what it does, but can you clarify whether you tried the pref route I described in comment #21? Does that not work?

Comment on attachment 8882551 [details] Bug 1363485 - Update BrowserUITelemetry.jsm https://reviewboard.mozilla.org/r/153662/#review158776 ::: browser/modules/BrowserUITelemetry.jsm:81 (Diff revision 1) > if (AppConstants.MOZ_PHOTON_THEME) { > result["nav-bar"].push("sidebar-button"); > } > Looks like you also need to remove this?

(In reply to :Gijs from comment #37) > > + <toolbaritem id="stop-reload-button" class="chromeclass-toolbar-additional" > > + title="&reloadButton.label;" > > I think you need to use 'label' rather than 'title' - right now there's no > label when you pin this button into the overflow panel. It's not clear to me how putting the label on the toolbaritem would do the right thing in the overflow panel. I think instead we need to set labels on reload-button and stop-button. > I also still think we should update the button label to say 'stop' when that > is the correct label (again, for the benefit of the panel - of course, in > the customize mode palette it won't matter very much). What I said above should do this automatically.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review159018 ::: browser/base/content/browser.xul:778 (Diff revision 8) > - onclick="checkForMiddleClick(this, event);" > + onclick="checkForMiddleClick(this, event);" > - tooltip="forward-button-tooltip" > + tooltip="forward-button-tooltip" > - context="backForwardMenu"/> > + context="backForwardMenu"/> > - <toolbaritem id="stop-reload-button" > - removable="false" overflows="false" > - class="chromeclass-toolbar-additional"> > + <toolbaritem id="stop-reload-button" class="chromeclass-toolbar-additional" > + title="&reloadButton.label;" > + removable="true" overflows="false"> You need to keep the chromeclass-toolbar-additional class.

(In reply to Dão Gottwald [::dao] from comment #41) > > - <toolbaritem id="stop-reload-button" > > - removable="false" overflows="false" > > - class="chromeclass-toolbar-additional"> > > + <toolbaritem id="stop-reload-button" class="chromeclass-toolbar-additional" > > + title="&reloadButton.label;" > > + removable="true" overflows="false"> > > You need to keep the chromeclass-toolbar-additional class. Oh, I see now that you kept it but moved it to a different line :/

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158228 > This is clever, but unfortunately this code runs after the toolbar is 'built' (so nodes are rearranged) by CustomizableUI. This will result in first removing the nodes, and then re-adding them. Admittedly, only on the run where we migrate, but still... Can you check if we can instead check for, and if present parse and manipulate the browser.uiCustomization.state pref? i tried currentset modification and manipulate the browser.uiCustomization.state pref and both didn't work.

Comment on attachment 8882210 [details] Bug 1363485 - Add migration to new navbar layout https://reviewboard.mozilla.org/r/153316/#review158774 I tried to modified 'nav-bar' list in 'browser.uiCustomization.state' and updating pref didn't work. This is the code: ```javascript let cState = JSON.parse(Services.prefs.getCharPref("browser.uiCustomization.state")); let cStateNavbar = cState.placements["nav-bar"]; log.info(JSON.stringify(cStateNavbar)); // ["urlbar-container","search-container","bookmarks-menu-button","downloads-button","sidebar-button","pocket-button","screenshots_mozilla_org-browser-action"] let cStateNavbarNew = ["back-button", "forward-button", "stop-reload-button", "home-button"] for (let widget of cStateNavbar) { if (!cStateNavbarNew.includes(widget)) { cStateNavbarNew.push(widget); } } log.info(JSON.stringify(cStateNavbarNew)); // ["back-button","forward-button","stop-reload-button","home-button","urlbar-container","search-container","bookmarks-menu-button","downloads-button","sidebar-button","pocket-button","screenshots_mozilla_org-browser-action"] cState.placements["nav-bar"] = cStateNavbarNew.slice(0); log.info(JSON.stringify(cState.placements["nav-bar"])); // ["back-button","forward-button","stop-reload-button","home-button","urlbar-container","search-container","bookmarks-menu-button","downloads-button","sidebar-button","pocket-button","screenshots_mozilla_org-browser-action"] Services.prefs.setCharPref("browser.uiCustomization.state", JSON.stringify(cState)); let cStateNew = JSON.parse(Services.prefs.getCharPref("browser.uiCustomization.state")); log.info(JSON.stringify(cStateNew.placements["nav-bar"])); // ["back-button","forward-button","stop-reload-button","home-button","urlbar-container","search-container","bookmarks-menu-button","downloads-button","sidebar-button","pocket-button","screenshots_mozilla_org-browser-action"] ``` then i check "browser.uiCustomization.state" in about:config: ```json "nav-bar": ["urlbar-container","search-container","bookmarks-menu-button","downloads-button","sidebar-button","pocket-button","screenshots_mozilla_org-browser-action","forward-button","back-button"] ``` Maybe i do something wrong in this code?

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review158772 > I think you need to use 'label' rather than 'title' - right now there's no label when you pin this button into the overflow panel. > > I also still think we should update the button label to say 'stop' when that is the correct label (again, for the benefit of the panel - of course, in the customize mode palette it won't matter very much). You can do this from within the CombinedStopReload object defined at https://dxr.mozilla.org/mozilla-central/rev/d9a144b7b6d994fc9a497c53b13f51a2a654d85e/browser/base/content/browser.js#4909 . i added labels on 'stop-button' and 'reload-button' and it also work, label on 'stop-reload-button' is for customization mode palette... but switching between stop and reload button in overflow panel doesn't work right, it works only after you exit customization mode.

Comment on attachment 8881124 [details] Bug 1363485 - Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button https://reviewboard.mozilla.org/r/152424/#review159168 ::: browser/base/content/browser.xul:779 (Diff revision 9) > - command="Browser:ForwardOrForwardDuplicate" > + command="Browser:ForwardOrForwardDuplicate" > - onclick="checkForMiddleClick(this, event);" > + onclick="checkForMiddleClick(this, event);" > - tooltip="forward-button-tooltip" > + tooltip="forward-button-tooltip" > - context="backForwardMenu"/> > + context="backForwardMenu"/> > - <toolbaritem id="stop-reload-button" > - removable="false" overflows="false" > + <toolbaritem id="stop-reload-button" class="chromeclass-toolbar-additional" > + label="&reloadButton.label;" Sorry, I was confused - with the labels on the individual toolbarbuttons, I guess using `title` on the containing toolbaritem is better than `label`. (In reply to UK92 from comment #48) > but switching between stop and reload button in overflow panel doesn't work > right, it works only after you exit customization mode. I don't really understand what you mean in this comment. Testing this patch, it seems to work like this?

(In reply to UK92 from comment #47) > Comment on attachment 8882210 [details] > Bug 1363485 - Add migration to new navbar layout > > https://reviewboard.mozilla.org/r/153316/#review158774 > > I tried to modified 'nav-bar' list in 'browser.uiCustomization.state' and > updating pref didn't work. > Maybe i do something wrong in this code? No, you're not doing anything wrong. The problem is that something else (I expect one of the add-ons) already loads CustomizableUI.jsm, and it loads its saved state immediately, so subsequent changes to its pref are ignored. We can either keep this as you wrote it, or use the UI customization state in CustomizableUI itself. Maybe that's better... the pattern is basically the same as in nsBrowserGlue.js. You can increment kVersion here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#63-67 and add code here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#362-415 to add the new buttons, probably something like: if (currentVersion < 7 && gSavedState && gSavedState.placements && gSavedState.placements[CustomizableUI.AREA_NAVBAR]) { let placements = gSavedState.placements[CustomizableUI.AREA_NAVBAR]; let insertions = 0; for (let button of ["back-button", "forward-button", "stop-reload-button", "home-button"]) { if (!placements.includes(button)) { placements.splice(insertions++, 0, button); } } if (!placements.includes("sidebar-button")) { placements.push("sidebar-button"); } } (we need the nullchecks in the if statement because on new profiles the nav bar might not have any customized state, in which case it'll pick up the default state you've already changed, which should be fine)

(In reply to :Gijs from comment #49) > (In reply to UK92 from comment #48) > > but switching between stop and reload button in overflow panel doesn't work > > right, it works only after you exit customization mode. > > I don't really understand what you mean in this comment. Testing this patch, > it seems to work like this? When reload button is in overflow panel, it doesn't change to stop button, when page is loading (always showing reload button). When you enter and leave customization mode, switching between reload and stop button start working.

Comment on attachment 8882210 [details] Bug 1363485 - Add migration to new navbar layout https://reviewboard.mozilla.org/r/153316/#review159932 ::: browser/components/customizableui/CustomizableUI.jsm:432 (Diff revision 5) > + > + if (!newPlacements.includes("sidebar-button")) { > + newPlacements.push("sidebar-button"); > + } > + > + gSavedState.placements[CustomizableUI.AREA_NAVBAR] = newPlacements.slice(0); Nit: I don't think you need to use slice(0) here, as nothing else will change the array from this point on.

(In reply to UK92 from comment #54) > (In reply to :Gijs from comment #49) > > (In reply to UK92 from comment #48) > > > but switching between stop and reload button in overflow panel doesn't work > > > right, it works only after you exit customization mode. > > > > I don't really understand what you mean in this comment. Testing this patch, > > it seems to work like this? > > When reload button is in overflow panel, it doesn't change to stop button, > when page is loading (always showing reload button). > When you enter and leave customization mode, switching between reload and > stop button start working. OK, I can reproduce this after a restart after moving the item to the panel. I think we can tackle this in a separate bug. I've filed bug 1378819 for this.

I rebased commits, and make small change, i used existing button labels `reloadCmd.label` and `stopCmd.label` instead of create new one.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c62afb1941d1faa4ed5b2ccd8e16892981f1ec Failing tests: browser/components/customizableui/test/browser_884402_customize_from_overflow.js browser/components/extensions/test/browser/browser_ext_user_events.js browser/components/customizableui/test/browser_914138_widget_API_overflowable_toolbar.js browser/modules/test/browser/browser_BrowserUITelemetry_defaults.js

I couldn't reproduce 'browser_884402_customize_from_overflow.js' test failure.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=577eebf49ddd7c10b41d1bc4d85cb2472574bbd0

(In reply to Dão Gottwald [::dao] from comment #67) > New try run: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=577eebf49ddd7c10b41d1bc4d85cb2472574bbd0 browser/components/extensions/test/browser/browser_ext_user_events.js still fails on Windows :(

(In reply to Dão Gottwald [::dao] from comment #68) > (In reply to Dão Gottwald [::dao] from comment #67) > > New try run: > > > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=577eebf49ddd7c10b41d1bc4d85cb2472574bbd0 > > browser/components/extensions/test/browser/browser_ext_user_events.js still > fails on Windows :( It looks like at least part of the reason for this is the now-default sidebar button taking up space and pushing things into the overflow menu at the window size that that test runs with on infra. I expect we could just remove extraneous buttons from the navbar for the test, and registerCleanupFunction(() => CustomizableUI.reset()) to restore order at the end of it.

I hope this will fix 'browser_ext_user_events.js' test.

(In reply to UK92 from comment #71) > I hope this will fix 'browser_ext_user_events.js' test. looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4cfbb89dcb10d6073b940b92808b9c4ff8ce637

(In reply to Dão Gottwald [::dao] from comment #72) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4cfbb89dcb10d6073b940b92808b9c4ff8ce637 I can move items between "back" and "forward", but I can't place them in this order: [......flexible spacer......] [back] [......locationbar......] [forward] [......flexible spacer......] | overflow | menu Please just make everything moveable, except the overflow icon and the menu icon. Even for some kind of kiosk mode it would be interesting to remove the locationbar and place there some special navigation buttons with a webextension.

(In reply to Darkspirit from comment #73) > (In reply to Dão Gottwald [::dao] from comment #72) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4cfbb89dcb10d6073b940b92808b9c4ff8ce637 > > I can move items between "back" and "forward", but I can't place them in > this order: > [......flexible spacer......] [back] [......locationbar......] [forward] > [......flexible spacer......] | overflow | menu I would take a fix that lets you do this (specifically, that lets you drag non-removable items within their container in customize mode). It would be a separate bug, and it isn't really related to this specific bug. > Please just make everything moveable, except the overflow icon and the menu > icon. No. This opens up lots of footguns in ways that we don't want to do, and in ways that the relevant code no longer supports. Various pieces of code expect the URL bar and some of these buttons to always be present, and that code will break if those assumptions are incorrect. As a result, the cost/benefit burden of making all these items removable is not worth it. (To be clear, there is not currently a halfway-house "movable" in the middle between "removable" and "non-removable" -- non-removable items cannot be reordered in the UI at all, and it is not possible with a simple attribute or somesuch to make items "movable" but not "removable". A fix would be, as specified above, to make it possible to drag non-removable items within their containers only. Doing this is not a photon requirement, but as said, I will review patches if people want to contribute them, in a separate bug.)

Comment on attachment 8891813 [details] Bug 1363485 - Update tests https://reviewboard.mozilla.org/r/162852/#review168718

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1e6aea26ec16 Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button r=Gijs https://hg.mozilla.org/integration/autoland/rev/69063acda8eb Add migration to new navbar layout r=Gijs https://hg.mozilla.org/integration/autoland/rev/0b1f1dbc1b5a Update BrowserUITelemetry.jsm r=Gijs https://hg.mozilla.org/integration/autoland/rev/f3e224764ca8 Update tests r=dao

Backed out for failing browser_urlbar_search_reflows.js on OSX debug with e10s and Windows: https://hg.mozilla.org/integration/autoland/rev/ba31ce1322a6056156297c26c7eb1bb963b4a31e https://hg.mozilla.org/integration/autoland/rev/e817cdb00e03824d272d99bfe0c357a10dc6fcbe https://hg.mozilla.org/integration/autoland/rev/b0e73d46ba355dfbd3d945711f733cdc07d68bdf https://hg.mozilla.org/integration/autoland/rev/c8818a7de36c34967c7316a709fc40f584becef8 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f3e224764ca8bdeb669bad39d36bc67a1cd60cdd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120360143&repo=autoland 06:33:31 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_urlbar_search_reflows.js | unexpected uninterruptible reflow 06:33:31 INFO - [ 06:33:31 INFO - "_handleOverflow@chrome://global/content/bindings/autocomplete.xml:2405:26", 06:33:31 INFO - "handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml:2477:11", 06:33:31 INFO - "_onChanged@chrome://global/content/bindings/autocomplete.xml:2086:15", 06:33:31 INFO - "_appendCurrentResult/<@chrome://global/content/bindings/autocomplete.xml:1412:17", 06:33:31 INFO - "setTimeout handler*_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1411:15", 06:33:31 INFO - "_invalidate@chrome://global/content/bindings/autocomplete.xml:1195:11", 06:33:31 INFO - "invalidate@chrome://global/content/bindings/autocomplete.xml:1164:11", 06:33:31 INFO - "testFn/popup.invalidate@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:252:7", 06:33:31 INFO - "notifyResults@jar:file:///Users/cltbld/tasks/task_1501679851/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/UnifiedComplete.js:2216:5", 06:33:31 INFO - "_addMatch@jar:file:///Users/cltbld/tasks/task_1501679851/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/UnifiedComplete.js:1834:5", 06:33:31 INFO - "_onResultRow@jar:file:///Users/cltbld/tasks/task_1501679851/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/UnifiedComplete.js:1728:5", 06:33:31 INFO - "handleResult@resource://gre/modules/Sqlite.jsm:779:13", 06:33:31 INFO - "" 06:33:31 INFO - ] 06:33:31 INFO - - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reflow :: line 104 06:33:31 INFO - Stack trace: 06:33:31 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reflow:104 06:33:31 INFO - chrome://global/content/bindings/autocomplete.xml:_handleOverflow:2405 06:33:31 INFO - chrome://global/content/bindings/autocomplete.xml:handleOverUnderflow:2477 06:33:31 INFO - chrome://global/content/bindings/autocomplete.xml:_onChanged:2086 06:33:31 INFO - chrome://global/content/bindings/autocomplete.xml:_appendCurrentResult/<:1412

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #78) > Backed out for failing browser_urlbar_search_reflows.js on OSX debug with > e10s and Windows: mconley, since you recently added that test, could you please take a quick look? What's your best guess: Did these patches actually add a new layout flush (seems unlikely to me), is this just some weird side-effect (should we add it to the whitelist?) or could the test itself have issues (should we disable it?).

Looking...

This might be related: Without patch: https://screenshots.firefox.com/ZyhWxITF3Z9ICgj8/i.imgur.com With patch: https://screenshots.firefox.com/MnTOh3rHEmzRDuaO/i.imgur.com Notice that with the patch, the layout of the popup has changed somewhat. There's some kind of collapsing going on so that the icons and entries are not aligned with the URL bar like they used to. Was that intentional? Do we know how the urlbarBindings calculates how to align the entries?

I believe the popup layout is intentionally different depending on how much space there's between the location bar and the window border. I'll try to find the code responsible for this, but I think it means we need to add the layout flush to the whitelist.

(In reply to Dão Gottwald [::dao] from comment #82) > I believe the popup layout is intentionally different depending on how much > space there's between the location bar and the window border. http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/browser/base/content/urlbarBindings.xml#1871-1892

Here's where I suspect we're falling over: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/toolkit/content/widgets/autocomplete.xml#2078-2089 I suspect that before your patch, iconChanged was being set to "false" from adjustSiteIconStart, so handleOverUnderflow was not being called.

Comment on attachment 8893002 [details] Bug 1363485 - Update urlbarBindings.xml https://reviewboard.mozilla.org/r/163996/#review169308 Works for me. I just submitted a more radical patch in bug 1386741 but we can take this one.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a0b559db94ff Move the Back, Forward, Reload and Stop buttons outside of the location bar container to the start of the toolbar along with the Home button r=Gijs https://hg.mozilla.org/integration/autoland/rev/83f8ca380201 Add migration to new navbar layout r=Gijs https://hg.mozilla.org/integration/autoland/rev/8d17fed26aaa Update BrowserUITelemetry.jsm r=Gijs https://hg.mozilla.org/integration/autoland/rev/393c431b0186 Update tests r=dao https://hg.mozilla.org/integration/autoland/rev/a91caf422971 Update urlbarBindings.xml r=dao

(In reply to :Gijs from comment #21) > 1) please remove the unrelated whitespace fixes from the l10n file This never happened, and it's far from clear from Mercurial's diff. Had to go in mozreview to figure what happened to that file. This screws up badly hg blame in that file.

https://hg.mozilla.org/mozilla-central/rev/a0b559db94ff https://hg.mozilla.org/mozilla-central/rev/83f8ca380201 https://hg.mozilla.org/mozilla-central/rev/8d17fed26aaa https://hg.mozilla.org/mozilla-central/rev/393c431b0186 https://hg.mozilla.org/mozilla-central/rev/a91caf422971

(In reply to Francesco Lodolo [:flod] from comment #90) > (In reply to :Gijs from comment #21) > > 1) please remove the unrelated whitespace fixes from the l10n file > > This never happened, and it's far from clear from Mercurial's diff. Had to > go in mozreview to figure what happened to that file. > This screws up badly hg blame in that file. This is unfortunate, but there is no way to fix that anymore short of doing repository surgery, which isn't warranted by this problem. :-(

I verified this on Mac OS X 10.10, Windows 10 x64, Windows 7 x64 and Ubuntu 16.04 with the latest Nightly 57.0a1(2017-08-15) and I can confirm that the Back, Forward, Reload and Stop buttons are moved outside of the location bar along with the Home button. I have a question regards to this.

Please see the attachment from comment 93. I see that there is an empty space between the Back, Forward, Reload and Stop buttons and the URL bar. This is intended? Thanks

(In reply to ovidiu boca[:Ovidiu] from comment #94) > Please see the attachment from comment 93. I see that there is an empty > space between the Back, Forward, Reload and Stop buttons and the URL bar. > This is intended? Thanks That's the result of bug 1383009.