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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: urbankrajnc92)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(6 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Blocks: 1363486
(Reporter)

Updated

2 years ago
Blocks: 1363487
(Reporter)

Updated

2 years ago
Flags: qe-verify+
Priority: -- → P1
Priority: P1 → P2
QA Contact: brindusa.tot
(Reporter)

Updated

2 years ago
Depends on: 1363840
(Reporter)

Updated

2 years ago
Summary: Move the back and forward buttons outside of the location bar to the start of the toolbar → Move the back, forward, reload and stop buttons outside of the location bar container
(Reporter)

Updated

2 years ago
Depends on: 1364090
(Reporter)

Updated

2 years ago
No longer blocks: 1363486
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1363486
(Reporter)

Updated

2 years ago
Summary: Move the back, forward, reload and stop buttons outside of the location bar container → 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
(Reporter)

Updated

2 years ago
No longer blocks: 1363487
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1363487
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1364951
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?)
(Reporter)

Comment 5

2 years ago
(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.

Updated

2 years ago
Duplicate of this bug: 1373893
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1373652
(Assignee)

Updated

2 years ago
Assignee: nobody → urbankrajnc92
Status: NEW → ASSIGNED
Priority: P2 → P1
(Reporter)

Updated

2 years ago
Attachment #8881124 - Flags: review?(dao+bmo) → review?(gijskruitbosch+bugs)
(Reporter)

Comment 9

2 years ago
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 10

2 years ago
mozreview-review
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.
Attachment #8881124 - Flags: review?(gijskruitbosch+bugs)

Comment 11

2 years ago
(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.
(Reporter)

Comment 12

2 years ago
(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?
Flags: needinfo?(shorlander)
(Reporter)

Comment 13

2 years ago
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.
Flags: needinfo?(shorlander)

Comment 15

2 years ago
(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.
Flags: needinfo?(shorlander)

Comment 16

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
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?
Attachment #8881124 - Flags: review?(gijskruitbosch+bugs)

Comment 22

2 years ago
mozreview-review
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.
(Reporter)

Comment 23

2 years ago
(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.
Flags: needinfo?(shorlander) → needinfo?(abenson)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8882209 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
I split up commit for easier testing, and also add photon specific migrate code based on Gijs idea.
(Reporter)

Comment 31

2 years ago
(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?
Flags: needinfo?(shorlander)

Comment 32

2 years ago
They should be separate items, but that's not currently reflected in the spec for customize mode. I'll update.
Flags: needinfo?(shorlander)
Flags: needinfo?(abenson)

Comment 33

2 years ago
mozreview-review
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.
Attachment #8881124 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Blocks: 1366555

Comment 37

2 years ago
mozreview-review
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 .
Attachment #8881124 - Flags: review?(gijskruitbosch+bugs)

Comment 38

2 years ago
mozreview-review
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?
Attachment #8882210 - Flags: review?(gijskruitbosch+bugs)

Comment 39

2 years ago
mozreview-review
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?
Attachment #8882551 - Flags: review?(gijskruitbosch+bugs) → review+
(Reporter)

Comment 40

2 years ago
(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.
(Reporter)

Comment 41

2 years ago
mozreview-review
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.
(Reporter)

Comment 42

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8882210 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 46

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 47

2 years ago
mozreview-review-reply
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?
(Assignee)

Comment 48

2 years ago
mozreview-review-reply
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 49

2 years ago
mozreview-review
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?
Attachment #8881124 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 50

2 years ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 54

2 years ago
(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.

Updated

2 years ago
Blocks: 1375080

Comment 55

2 years ago
mozreview-review
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.
Attachment #8882210 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

2 years ago
Blocks: 1378819

Comment 56

2 years ago
(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.
(Reporter)

Updated

2 years ago
Blocks: 1382898
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

2 years ago
I rebased commits, and make small change, i used existing button labels `reloadCmd.label` and `stopCmd.label` instead of create new one.
(Reporter)

Comment 61

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 66

2 years ago
I couldn't reproduce 'browser_884402_customize_from_overflow.js' test failure.
(Reporter)

Comment 68

2 years ago
(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 :(

Comment 69

2 years ago
(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.
Comment hidden (mozreview-request)
(Assignee)

Comment 71

2 years ago
I hope this will fix 'browser_ext_user_events.js' test.
(Reporter)

Comment 72

2 years ago
(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.

Comment 74

2 years ago
(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.)

Updated

2 years ago
Duplicate of this bug: 1385997
(Reporter)

Comment 76

2 years ago
mozreview-review
Comment on attachment 8891813 [details]
Bug 1363485 - Update tests

https://reviewboard.mozilla.org/r/162852/#review168718
Attachment #8891813 - Flags: review?(dao+bmo) → review+
(Reporter)

Updated

2 years ago
Blocks: 1273358

Comment 77

2 years ago
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

Updated

2 years ago
Depends on: 1386611
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
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 79

2 years ago
(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?).
Flags: needinfo?(dao+bmo) → needinfo?(mconley)
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?
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
(Reporter)

Comment 82

2 years ago
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.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 83

2 years ago
(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.
(Reporter)

Updated

2 years ago
Depends on: 1386741
Comment hidden (mozreview-request)
(Reporter)

Comment 86

2 years ago
mozreview-review
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.
Attachment #8893002 - Flags: review?(dao+bmo) → review+
(Reporter)

Updated

2 years ago
No longer depends on: 1386741
Duplicate of this bug: 1386741

Comment 89

2 years ago
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.

Comment 92

2 years ago
(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. :-(
Iteration: --- → 57.1 - Aug 15

Updated

2 years ago
Depends on: 1388990
Depends on: 1390006

Updated

2 years ago
No longer depends on: 1390006
Posted image empty space.png
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
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+ → needinfo?(dao+bmo)
(Reporter)

Comment 95

2 years ago
(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.
Flags: needinfo?(dao+bmo)
Blocks: 1098416
You need to log in before you can comment on or make changes to this bug.