Create an option to move sidebar between the left and right sides of the window

VERIFIED FIXED in Firefox 55

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
2 months ago
15 days ago

People

(Reporter: adw, Assigned: bgrins)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {addon-compat})

unspecified
Firefox 55
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 verified, relnote-firefox 55+)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
Left edge for RTL of course.

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
(Assignee)

Updated

2 months ago
Assignee: nobody → bgrinstead

Updated

2 months ago
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review134928

::: browser/base/content/test/general/browser_documentnavigation.js:148
(Diff revision 2)
>  // Open the sidebar and navigate between the sidebar, content and top-level window
>  add_task(function* () {
>    let sidebar = document.getElementById("sidebar");
>  
>    let loadPromise = BrowserTestUtils.waitForEvent(sidebar, "load", true);
> +  let focusPromise = BrowserTestUtils.waitForEvent(sidebar, "focus", true);

By the way, without waiting for focus the URL bar would become focused before the sidebar finished opening which meant that the sidebar would steal focus then lead the initial F6 to focus the URL bar instead of the content document.  I don't know why this event wasn't necessary before - it seems like it should have been.
(Assignee)

Comment 4

2 months ago
Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f73fb05423fc0a7fd1a828d53e96f6de1ec3e81&selectedJob=93032438
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Here's a try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f73fb05423fc0a7fd1a828d53e96f6de1ec3e81&selectedJob=9
> 3032438

I assume you pushed using artifact mode?

Comment 6

2 months ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review135236

Well, looking at the patch at work, it's kinda obvious it's working as expected :)

Thanks, Brian!

::: browser/base/content/browser.xul:1092
(Diff revision 2)
> +          <label id="sidebar-title" persist="value" flex="1" crop="end" control="sidebar"/>
> +          <image id="sidebar-throbber"/>
> +          <toolbarbutton class="close-icon tabbable" tooltiptext="&sidebarCloseButton.tooltip;" oncommand="SidebarUI.hide();"/>
> +        </sidebarheader>
> +        <browser id="sidebar" flex="1" autoscroll="false" disablehistory="true" disablefullscreen="true"
> +                  style="min-width: 14em; width: 18em; max-width: 36em;" tooltip="aHTMLTooltip"/>

/me curious why this isn't in browser.css...
Attachment #8860087 - Flags: review?(mdeboer) → review+

Comment 7

2 months ago
mozreview-review-reply
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review134928

> By the way, without waiting for focus the URL bar would become focused before the sidebar finished opening which meant that the sidebar would steal focus then lead the initial F6 to focus the URL bar instead of the content document.  I don't know why this event wasn't necessary before - it seems like it should have been.

That must be because the content document is now _before_ the opened sidebar, instead of after it.
(Assignee)

Comment 8

2 months ago
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Here's a try push:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=4f73fb05423fc0a7fd1a828d53e96f6de1ec3e81&selectedJob=9
> > 3032438
> 
> I assume you pushed using artifact mode?

Yes, my usual command is `./mach try -b do -p linux,linux64,macosx64,win32 -u xpcshell,mochitests -t none`, which automatically appends --artifact if the local build is set up with `ac_add_options --enable-artifact-builds`.
(Assignee)

Comment 9

2 months ago
mozreview-review-reply
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review135236

> /me curious why this isn't in browser.css...

Yeah.  I'll make a note to move this if one of the other items off of Bug 1353421 requires some changes to these styles.

Comment 10

2 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/673ad58aecac
Move sidebar to the right edge of the window;r=mikedeboer

Comment 11

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/673ad58aecac
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 12

2 months ago
abusive-reviewed
Anyone care to explain, next to a laughable try at being "trendy", what the real point of this change is ?

Updated

2 months ago
Depends on: 1358770

Comment 13

2 months ago
Please do add an option for the user to choose on which side of the Firefox/Nightly window the sidebar is displayed ! I find it being displayed to the right of the window without such an option disturbs my work flow, and is hardly compatible with Mozilla's desire to make Firefox the most user-friendly and customisable browser of them all....

Henri

Comment 14

2 months ago
yes please add option to move back to the left. 
thanks.
Depends on: 1358854
Depends on: 1358856
I filed bug 1358873, fearing that it was intentional, though I expected at least some kind of explanation why the choice was made. So I'm surprised there isn't one given in this bug.

I'm usually not precious about Firefox's UI -- I had no problem with Australis, for example -- but this one is really messing with my muscle memory and visual expectations. +1 for bug 1358854.
Release Note Request (optional, but appreciated)
[Why is this notable]: The sidebar (history, bookmarks) is now on the right side of the browser window, this is a significant UI change since we have always had that sidebar on the left side
[Affects Firefox for Android]: no
[Suggested wording]: The sidebar (bookmarks, history, synced tabs) now opens to the right edge of the window
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?

Comment 17

2 months ago
¡Hola Brian!

FWIW this one bite me on the form of https://github.com/bwinton/TabCenter/issues/1064 so this might break other add-ons that draw on the sides of the main UI.

It might be worth to have an eye out for fallout like that one...

¡Gracias!
Alex
Flags: needinfo?(bgrinstead)

Comment 18

2 months ago
As othere, I also think this should be optional to keep Firefox in hands of users as it always is (was)??
(Assignee)

Comment 19

2 months ago
FYI: based on tabcenter breakage and feedback here we are going to back out this patch
Flags: needinfo?(bgrinstead)
OK I will tell you a few things about this whole issue.

1.  I think the whole sidebar on right is due to it being so much easier for toch screen for right handed so right thumbed people that that biases all of these surveys.

2.  The actually more scientific study I saw about moving sidebars to right firstlay did not find a significant difference although it did find tabs on right "better".  the issue is

3.  For the one "scientific" survey the issue comes with the biased way of representing the results.  It was a more time spent on site and more pages viewed is better.  Well that makes sense if those who left earlier with tabs on left left out of frustration.  but if they left because they found what they were looking for then the results need to be flipped on their side because tabs on left people spent less time and had to view less pages to find what they were looking for.
In any event, because of the left-right handedness issue this really needs to be a preference, but also becaus the only add-on I found to restore what I wanted was omnisidebar and it had a undock feature which allowed the sidebar to float and also to auto-hide.  I realy think if we want to do something better with the new theme rather than just something different we should try to implement this add-on built in to the product.

Comment 22

2 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/181a89c580fd
Backed out changeset 673ad58aecac in the light of user feedback, r=me,backout

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
...also, there should be a pref / #ifdef / something so that this change is Nightly-only until the 57 train.
Comment hidden (off-topic)
status-firefox55: fixed → ---
Keywords: addon-compat
Target Milestone: Firefox 55 → ---

Comment 25

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/181a89c580fd
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 months ago
status-firefox55: fixed → ---
Target Milestone: Firefox 55 → ---
(Assignee)

Updated

2 months ago
Assignee: bgrinstead → nobody

Updated

2 months ago
Status: REOPENED → NEW
Iteration: 55.4 - May 1 → ---
Priority: P1 → P2
My feeling is this needs a preference because my feeling is this is more of a righthanded versus lefthanded than a LTR versus RTL issue as seems to be the prevailing thought.  I also really like the way the Omnisidebar add-on works which allows you to undock the sidebar so that is will auto-hide.
My feeling is make it better, rather than make it different.

Comment 28

2 months ago
Please leave this decision to the users, clearly exposed in the settings UI. I'm the author of the Vertical Tabs Reloaded add-on and against all odds I just got back a bit of hope that the add-on ecosystem will not go to totally break. I'm considering forcing the sidebars to the right just as another deal breaker.

When it's about add-on's own sidebars maybe even leave the positon decision to the add-on. But definitely to the user.

Comment 29

2 months ago
It is with some trepidation that I post this, as I fear what has happened might just turn out to be an accident and could be «corrected» by the developers, but the last few updates to Nightly have returned (on my machines, both those running Linux Mint 18.1 and those running Windows 10 (v 1703)), the sidebar to the left of the Nightly window. If, indeed, this is not an accident, I wish to congratulate the developers for heeding the concerns, as expressed on this forum, of us users. That is what Firefox is supposed to be, a web browser that allows its users to configure it as best suits their needs and desires....

Henri
(In reply to M Henri Day from comment #29)
> ...
> concerns, as expressed on this forum, of us users.

This is not a forum by any means. This is a bug and work tracker. Please leave your thoughts and concerns - especially when they've already been voiced by other community members - on public forums and mailing lists as they're more accessible and more suited for that purpose.

Comment 31

2 months ago
That some people seem to have regarded the fact that the side bar in Nightly opened to the left of the FF window as a bug, gave rise to concerns among users, which were then reflected in posts here and elsewhere as well. I consider that at least as appropriate for this «bug and work tracker», Mr de Boer, as your snide comment above, which has nothing, so far as I can see, with tracking anything. So it goes....

Henri
(In reply to M Henri Day from comment #31)
> That some people seem to have regarded the fact that the side bar in Nightly
> opened to the left of the FF window as a bug, gave rise to concerns among
> users, which were then reflected in posts here and elsewhere as well. I
> consider that at least as appropriate for this «bug and work tracker», Mr de
> Boer, as your snide comment above, which has nothing, so far as I can see,
> with tracking anything. So it goes....
> 
> Henri

Look you have been told this is not a forum nor a soapbox.  Please cease and desist.
(Assignee)

Comment 33

2 months ago
Updating the bug title to match the updated plan, which is to add a menu item to move the sidebar from one side to another in the sidebar switcher popup
Depends on: 1355324
Summary: Move sidebar to the right edge of the window → Create an option to move sidebar between the left and right sides of the window
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1358854
(Assignee)

Updated

2 months ago
Attachment #8860087 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Updated

a month ago
Priority: P2 → P1
(Assignee)

Updated

a month ago
Attachment #8860087 - Attachment is obsolete: false
Comment hidden (mozreview-request)
(Assignee)

Comment 36

a month ago
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

mozreview carried over the r+ but this is a new patch
Attachment #8860087 - Flags: review+ → review?(mdeboer)

Comment 37

a month ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review143440

::: browser/app/profile/firefox.js:1278
(Diff revision 3)
>  pref("image.mem.max_decoded_image_kb", 256000);
>  
>  pref("social.sidebar.unload_timeout_ms", 10000);
>  
> +// Is the sidebar is moved into the reversed position (default = end, reversed = start).
> +pref("sidebar.reverse_position", false);

Please rename this to explicitly mention end or start, such that we can potentially change the default without reversing the meaning of the pref.

::: browser/base/content/browser-sidebar.js:132
(Diff revision 3)
> +    let isRTL = getComputedStyle(browser).direction == "rtl";
> +    let label;
> +    if (Services.prefs.getBoolPref(this.REVERSE_POSITION_PREF)) {
> +      label = isRTL ? gNavigatorBundle.getString("sidebar.moveToRight") :
> +                      gNavigatorBundle.getString("sidebar.moveToLeft");
> +      browser.setAttribute("dir", "reverse");

Please move the DOM nodes instead of setting the dir attribute, as #browser could have other content that doesn't want to be affected by this.

Comment 38

a month ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review143446

Well, Dão said all the good parts. Can you implement those requested changes and re-request review from me? Thanks!
Attachment #8860087 - Flags: review?(mdeboer)

Comment 39

a month ago
(In reply to Dão Gottwald [::dao] from comment #37)
> ::: browser/base/content/browser-sidebar.js:132
> (Diff revision 3)
> > +    let isRTL = getComputedStyle(browser).direction == "rtl";
> > +    let label;
> > +    if (Services.prefs.getBoolPref(this.REVERSE_POSITION_PREF)) {
> > +      label = isRTL ? gNavigatorBundle.getString("sidebar.moveToRight") :
> > +                      gNavigatorBundle.getString("sidebar.moveToLeft");
> > +      browser.setAttribute("dir", "reverse");
> 
> Please move the DOM nodes instead of setting the dir attribute, as #browser
> could have other content that doesn't want to be affected by this.

That would re-init the contents, presumably, and cause significant flickering, and make it hard for users to go back to where they were, if they e.g. had a search open in the history sidebar, or had explored a tree structure. If they had a web page open in the sidebar, it would cause dataloss of any data in form fields or other state in the page. All of those seem bad enough to avoid manipulating the DOM if we can help it.

In fact, reordering the other nodes seems desirable to me - you wouldn't want the sidebar to be at the same side as the devtools (which can be anchored on the side of the window as well). So I think this is the right solution.

If this is really unacceptable for some reason I don't understand, perhaps we can use -moz-box-ordinal-group (based on an attribute) to move the sidebar and splitter instead, without changing the overall direction of this particular element?
Flags: needinfo?(dao+bmo)
(In reply to :Gijs from comment #39)
> > Please move the DOM nodes instead of setting the dir attribute, as #browser
> > could have other content that doesn't want to be affected by this.
> 
> That would re-init the contents, presumably, and cause significant
> flickering,

Shouldn't be any worse than opening the sidebar in the first place?

> and make it hard for users to go back to where they were, if
> they e.g. had a search open in the history sidebar, or had explored a tree
> structure. If they had a web page open in the sidebar, it would cause
> dataloss of any data in form fields or other state in the page. All of those
> seem bad enough to avoid manipulating the DOM if we can help it.

It moves #browser-border-start to the wrong side, for instance. It just happens that we style #browser-border-start and #browser-border-end the same way.

Our support for loading web content in the side bar is pretty rudimentary. I wouldn't worry about having to reload it. Generally it seems that this will be a one-time switch for most users, and it seems odd to optimize this for a user starting to do a complex task in the side bar and then decide to move it to the other side.

> In fact, reordering the other nodes seems desirable to me - you wouldn't
> want the sidebar to be at the same side as the devtools (which can be
> anchored on the side of the window as well). So I think this is the right
> solution.

It's actually not clear to me that a user asking for this side bar to the left wants devtools on the right. Maybe that's the only way to do this (I guess the resizers might stop working otherwise?), but then we better make this explicit in devtools code rather than a side effect of how we move this sidebar.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 41

a month ago
Note that none of these changes will affect where devtools opens when side docked, as it is loaded inside #appcontent. Had a quick chat with shorlander, the plan for now is to leave devtools on the right regardless of splitter location (with devtools being closer to page content).

I think moving the sidebar-box element would cause state to be lost in the ways mentioned in Comment 39, so I'd prefer to do something with CSS instead.  Using -moz-box-ordinal-group instead of dir would add in a couple of hardcoded positions (sidebar-box and sidebar-splitter), but wouldn't be too big a change if you'd be happy with that.

I take the point about browser-border-start and browser-border-end location being swapped as an unintended side effect.  Just an idea - maybe we could change the styling of those two elements to be applied via a `browser-border-side` class instead.  There are a fairly limited number of references to these elements from addons, and also the IDs could be removed once we are supporting webextension only:
* https://dxr.mozilla.org/addons/search?q=browser-border-start&redirect=true
* https://dxr.mozilla.org/addons/search?q=browser-border-&redirect=false
Flags: needinfo?(dao+bmo)
(In reply to Brian Grinstead [:bgrins] from comment #41)
> I take the point about browser-border-start and browser-border-end location
> being swapped as an unintended side effect.  Just an idea - maybe we could
> change the styling of those two elements to be applied via a
> `browser-border-side` class instead.

We could do that, except that I brought this up as an example of how using dir="reverse" on a container that's not exclusively yours can have unintended side-effects. I'd prefer if we avoided this problem.
Flags: needinfo?(dao+bmo)
I have not really looked at the patch but is would seem that from the toolbar icon to toggle sidebar visibility in the new theme this would need to be flipped also.  Does the patch here do that?

I still think I prefer the auto-hide feature of omnisidebar to any of this.  Why waste space for an icon to do what auto-hide can do so nicely?

Comment 44

a month ago
Yeah, having the icon reflect which side the sidebar is set to is a nice touch. We should do this! Can we flip the icon depending on the setting, Brian?
(Assignee)

Updated

a month ago
Blocks: 1366851
(Assignee)

Comment 45

a month ago
(In reply to Aaron Benson from comment #44)
> Yeah, having the icon reflect which side the sidebar is set to is a nice
> touch. We should do this! Can we flip the icon depending on the setting,
> Brian?

Yes, filed bug 1366851 for that
Comment hidden (mozreview-request)

Comment 47

a month ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review145476

::: browser/base/content/browser-sidebar.js:48
(Diff revision 4)
>      this._switcherTarget = document.getElementById("sidebar-switcher-target");
>      this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
>  
>      this._switcherTarget.addEventListener("command", () => {
>        this.toggleSwitcherPanel();
>      });

I'd recommend using the following:

```js
// in 'init() {'
XPCOMUtils.defineLazyPreferenceGetter(this, "_positionStart", this.POSITION_START_PREF,
  true, this.setPosition.bind(this));
  
// in 'uninit()' there's no real need to remove the observer.
```

::: browser/base/content/browser-sidebar.js:107
(Diff revision 4)
>        button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));
>      }
>    },
>  
> +  _addedPositionObserver: false,
> +  _ensurePositionObserverAdded() {

...you can then remove these lines and...

::: browser/base/content/browser-sidebar.js:121
(Diff revision 4)
> +
> +  /**
> +   * Change the pref that will trigger a call to setSidebarPosition
> +   */
> +  reversePosition() {
> +    Services.prefs.setBoolPref(this.POSITION_START_PREF,

And this will then change to
```js
Services.prefs.setBoolPref(this.POSITION_START_PREF, !this._positionStart);
```

::: browser/base/content/browser-sidebar.js:129
(Diff revision 4)
> +
> +  /**
> +   * Read the positioning pref and position the sidebar and the splitter
> +   * appropriately within the browser container.
> +   */
> +  setSidebarPosition() {

I think you can rename this to simply 'setPosition', no?

::: browser/base/content/browser-sidebar.js:130
(Diff revision 4)
> +  /**
> +   * Read the positioning pref and position the sidebar and the splitter
> +   * appropriately within the browser container.
> +   */
> +  setSidebarPosition() {
> +    // First reset all ordinals to match DOM ordering

If you add `this._currentlyAtStart: null,` above and
```js
// Bail out early if we're already in the correct position.
if (this._currentlyAtStart === this._startPosition)
  return;
this._currentlyAtStart = this._startPosition;

// Do stuff...
```

::: browser/base/content/browser-sidebar.js:132
(Diff revision 4)
> +   * appropriately within the browser container.
> +   */
> +  setSidebarPosition() {
> +    // First reset all ordinals to match DOM ordering
> +    let browser = document.getElementById("browser");
> +    [...browser.childNodes].forEach((node, i) => {

Is resetting the ordinal values here when you're calling this again intentional?
Wouldn't this cause jank when you're calling this method a second or third time?

Perhaps you want
```js
let count = 0;
for (let node of [...browser.childNodes]) {
  ++count;
  if (node.ordinal)
    continue;
  node.ordinal = count + 1;
}
```
?

Or would that cause other unwanted side-effects?

::: browser/base/content/browser-sidebar.js:137
(Diff revision 4)
> +    [...browser.childNodes].forEach((node, i) => {
> +      node.ordinal = i + 1;
> +    });
> +
> +    // Combine start/end position with ltr/rtl to set the label in the popup appropriately.
> +    let isRTL = getComputedStyle(browser).direction == "rtl";

Please cache the value here, because querying this causes an (unnecessary) extra reflow.

::: browser/base/content/browser-sidebar.js:139
(Diff revision 4)
> +    });
> +
> +    // Combine start/end position with ltr/rtl to set the label in the popup appropriately.
> +    let isRTL = getComputedStyle(browser).direction == "rtl";
> +    let label;
> +    if (Services.prefs.getBoolPref(this.POSITION_START_PREF)) {

How does this look?
```js
let label = gNavigatorBundle.getString("sidebar.moveTo" + (this._currentlyAtStart || isRTL ? "Right" : "Left"));
```

::: browser/base/content/browser-sidebar.js:213
(Diff revision 4)
>  
>    /**
>     * If loading a sidebar was delayed on startup, start the load now.
>     */
>    startDelayedLoad() {
> +    this._ensurePositionObserverAdded();

add the following here and below:
```js
this.setPosition();
```
Attachment #8860087 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 48

a month ago
(In reply to Mike de Boer [:mikedeboer] from comment #47)
> ::: browser/base/content/browser-sidebar.js:132
> (Diff revision 4)
> > +   * appropriately within the browser container.
> > +   */
> > +  setSidebarPosition() {
> > +    // First reset all ordinals to match DOM ordering
> > +    let browser = document.getElementById("browser");
> > +    [...browser.childNodes].forEach((node, i) => {
> 
> Is resetting the ordinal values here when you're calling this again
> intentional?
> Wouldn't this cause jank when you're calling this method a second or third
> time?
> 
> Perhaps you want
> ```js
> let count = 0;
> for (let node of [...browser.childNodes]) {
>   ++count;
>   if (node.ordinal)
>     continue;
>   node.ordinal = count + 1;
> }
> ```
> ?
> 
> Or would that cause other unwanted side-effects?

Resetting the ordinal in all cases is intentional.  The default behavior is ordinal=1, in which case they layout in DOM order.  In my testing you go through and set custom ordinals to get the right side layout to work, and then go through and remove ordinals or reset them all to 1 the elements don't move back.  The most straightforward way I came up with to deal with this was to reset all children's ordinal to their index, and then if needed do a swap between sidebar-box and appcontent.
(Assignee)

Comment 49

a month ago
(In reply to Mike de Boer [:mikedeboer] from comment #47)
> Comment on attachment 8860087 [details]
> Bug 1355331 - Create an option to move sidebar between the left and right
> sides of the window;
> 
> https://reviewboard.mozilla.org/r/132104/#review145476
> 
> ::: browser/base/content/browser-sidebar.js:48
> (Diff revision 4)
> >      this._switcherTarget = document.getElementById("sidebar-switcher-target");
> >      this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
> >  
> >      this._switcherTarget.addEventListener("command", () => {
> >        this.toggleSwitcherPanel();
> >      });
> 
> I'd recommend using the following:
> 
> ```js
> // in 'init() {'
> XPCOMUtils.defineLazyPreferenceGetter(this, "_positionStart",
> this.POSITION_START_PREF,
>   true, this.setPosition.bind(this));
>   
> // in 'uninit()' there's no real need to remove the observer.
> ```
> 
> ::: browser/base/content/browser-sidebar.js:107
> (Diff revision 4)
> >        button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));
> >      }
> >    },
> >  
> > +  _addedPositionObserver: false,
> > +  _ensurePositionObserverAdded() {
> 
> ...you can then remove these lines and...
> 
> ::: browser/base/content/browser-sidebar.js:121
> (Diff revision 4)
> > +
> > +  /**
> > +   * Change the pref that will trigger a call to setSidebarPosition
> > +   */
> > +  reversePosition() {
> > +    Services.prefs.setBoolPref(this.POSITION_START_PREF,
> 
> And this will then change to
> ```js
> Services.prefs.setBoolPref(this.POSITION_START_PREF, !this._positionStart);
> ```

Nice, I didn't know defineLazyPreferenceGetter allowed a callback to be passed in, that will get rid of a lot of boilerplate! I think it'll need a little bit of adjustment due to the fact that we want to add it in on the first call of either startDelayedLoad or init, but that shouldn't be a problem.

> ::: browser/base/content/browser-sidebar.js:130
> (Diff revision 4)
> > +  /**
> > +   * Read the positioning pref and position the sidebar and the splitter
> > +   * appropriately within the browser container.
> > +   */
> > +  setSidebarPosition() {
> > +    // First reset all ordinals to match DOM ordering
> 
> If you add `this._currentlyAtStart: null,` above and
> ```js
> // Bail out early if we're already in the correct position.
> if (this._currentlyAtStart === this._startPosition)
>   return;
> this._currentlyAtStart = this._startPosition;
> 
> // Do stuff...
> ```

As it is now, I don't think this will help perf since setSidebarPosition is called once at startup and then only when the pref changes. As for the first call we need to run the logic regardless of position to at least set the label text.  Although maybe we could delay setting the label text until showSwitcherPanel is called..  I'll look into that.
(Assignee)

Comment 50

a month ago
(In reply to Mike de Boer [:mikedeboer] from comment #47)
> How does this look?
> ```js
> let label = gNavigatorBundle.getString("sidebar.moveTo" +
> (this._currentlyAtStart || isRTL ? "Right" : "Left"));
> ```

I'd prefer to keep the property keys in tact to make it easier to grep for usages if you don't feel strongly about it. I also don't think this logic is correct - it would always return "Move to Right" in RTL locales.
Comment hidden (mozreview-request)
(Assignee)

Comment 52

a month ago
Thanks for the feedback Mike, it should mostly be addressed in the latest version. Just a couple notes where it differs:
1) I did end up simplifying the `label` value after all but with a little different logic
2) Instead of calling setPosition() in startDelayedLoad and init, I pulled out the code that actually unhides the box and splitter into a new function, and am calling setPosition there if needed.  This seemed more straightforward and will prevent calling it at all on startup if the sidebar is going to be hidden, but let me know if you think that should change

Comment 53

a month ago
mozreview-review
Comment on attachment 8860087 [details]
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;

https://reviewboard.mozilla.org/r/132104/#review145912

Very nice!! I know there were a considerable amount of iterations, but it looks rock-solid to me know :) I hope you didn't find the process frustrating.

Looking forward to your next work!

::: browser/base/content/browser-sidebar.js:84
(Diff revision 5)
>      this._switcherPanel.addEventListener("popuphiding", () => {
>        this._switcherTarget.classList.remove("active");
>      }, {once: true});
> +
> +    // Combine start/end position with ltr/rtl to set the label in the popup appropriately.
> +    let label = this._positionStart == this.isRTL ?

Apology for my bad example earlier. This is much better, thanks!

::: browser/base/content/browser-sidebar.js:122
(Diff revision 5)
> +  /**
> +   * Read the positioning pref and position the sidebar and the splitter
> +   * appropriately within the browser container.
> +   */
> +  setPosition() {
> +    // First reset all ordinals to match DOM ordering

nit: missing dot.

::: browser/base/content/browser-sidebar.js:421
(Diff revision 5)
> +// Add getters related to the position here, since we will want them
> +// available for both startDelayedLoad and init.
> +XPCOMUtils.defineLazyPreferenceGetter(SidebarUI, "_positionStart",
> +  SidebarUI.POSITION_START_PREF, true, SidebarUI.setPosition.bind(SidebarUI));
> +XPCOMUtils.defineLazyGetter(SidebarUI, "isRTL", () => {
> +  return getComputedStyle(document.getElementById("browser")).direction == "rtl";

nit: You can also use `document.documentElement` here.
Attachment #8860087 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)

Comment 55

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1475d108174d
Create an option to move sidebar between the left and right sides of the window;r=mikedeboer

Comment 56

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1475d108174d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months agoa month ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a month ago
Iteration: --- → 55.6 - May 29
(Assignee)

Updated

29 days ago
Depends on: 1367767

Updated

27 days ago
Depends on: 1368317
Depends on: 1368624
Depends on: 1369675
adding to the 55beta release notes
relnote-firefox: ? → 55+
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.