Closed Bug 1355331 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Tracking Flags:
Tracking Status
relnote-firefox --- 55+
firefox55 --- verified

People

(Reporter: adw, Assigned: bgrins)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [photon-structure])

Attachments

(1 file)

Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
Left edge for RTL of course.
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
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.
(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 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 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.
(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`.
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.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/673ad58aecac
Move sidebar to the right edge of the window;r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/673ad58aecac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1358770
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
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: --- → ?
¡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)
As othere, I also think this should be optional to keep Firefox in hands of users as it always is (was)??
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
...also, there should be a pref / #ifdef / something so that this change is Nightly-only until the 57 train.
https://hg.mozilla.org/mozilla-central/rev/181a89c580fd
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox55: fixed → ---
Target Milestone: Firefox 55 → ---
Assignee: bgrinstead → nobody
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.
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
Attachment #8860087 - Attachment is obsolete: true
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8860087 - Attachment is obsolete: false
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 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 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)
(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)
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?
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?
Blocks: 1366851
(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 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-
(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.
(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.
(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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/1475d108174d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.6 - May 29
Depends on: 1367767
Depends on: 1368317
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+
Blocks: 1387512
Blocks: 1405382
Depends on: 1403965
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: