Merge branch '22343-honor-user-fixed-layout-pref' into 'master'

Ensure the 'fixed layout' preference is honored whenever possible

## What does this MR do?

Currently, when viewing any Merge Request the user's fixed-layout preference is overridden if they have set "Side-by-Side" view as their preference when viewing diffs.  This makes sense if they are currently viewing a diff, but this is confusing when their layout preference is overridden while they are on another tab (i.e. "Discussion" or "Builds").

This MR moves all responsibility for overriding the fixed layout from the Ruby page layout helper into frontend JavaScript where it is only applied when needed.

## Are there points in the code the reviewer needs to double check?

Check that nothing broke for users which have their layout preference set to "Fluid" as well as those which have it set to "Fixed".  I've already done this but double checking is always good idea :smile:.

## Screenshots (if relevant)

![side-by-side-toggle-fixed](/uploads/033dc73e70b73da5692b75606733c938/side-by-side-toggle-fixed.gif)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #22343 

## Other Notes

As @lbennett noted in #22343, this is something of a band-aid.  It fixed most of the issue, but we still need to have a discussion about whether or not is acceptable to override user layout preferences for "Side-by-Side" views in general.  At least this MR limits the scope of this behavior to a single tab within an MR or merge conflict page.

See merge request !6422

11 files changed, 58 insertions, 33 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 9f871360bf4..d654975d886 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -109,6 +109,7 @@ v 8.12.0
   - Fix long comments in diffs messing with table width
   - Add spec covering 'Gitlab::Git::committer_hash' !6433 (dandunckelman)
   - Fix pagination on user snippets page
+  - Honor "fixed layout" preference in more places !6422
   - Run CI builds with the permissions of users !5735
   - Fix sorting of issues in API
   - Fix download artifacts button links !6407
diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js
index c8634b78f2b..8086c10ad6b 100644
--- a/app/assets/javascripts/diff.js
+++ b/app/assets/javascripts/diff.js
@@ -7,6 +7,9 @@
     function Diff() {
       $('.files .diff-file').singleFileDiff();
       this.filesCommentButton = $('.files .diff-file').filesCommentButton();
+      if (this.diffViewType() === 'parallel') {
+        $('.content-wrapper .container-fluid').removeClass('container-limited');
+      }
       $(document).off('click', '.js-unfold');
       $(document).on('click', '.js-unfold', (function(_this) {
         return function(event) {
@@ -52,6 +55,10 @@
       })(this));
     }
 
+    Diff.prototype.diffViewType = function() {
+      return $('.inline-parallel-buttons a.active').data('view-type');
+    }
+
     Diff.prototype.lineNumbers = function(line) {
       if (!line.children().length) {
         return [0, 0];
diff --git a/app/assets/javascripts/merge_conflict_data_provider.js.es6 b/app/assets/javascripts/merge_conflict_data_provider.js.es6
index cd92df8ddc5..13ee794ba38 100644
--- a/app/assets/javascripts/merge_conflict_data_provider.js.es6
+++ b/app/assets/javascripts/merge_conflict_data_provider.js.es6
@@ -7,13 +7,16 @@ const ORIGIN_BUTTON_TITLE = 'Use theirs';
 class MergeConflictDataProvider {
 
   getInitialData() {
+    // TODO: remove reliance on jQuery and DOM state introspection
     const diffViewType = $.cookie('diff_view');
+    const fixedLayout = $('.content-wrapper .container-fluid').hasClass('container-limited');
 
     return {
       isLoading      : true,
       hasError       : false,
       isParallel     : diffViewType === 'parallel',
       diffViewType   : diffViewType,
+      fixedLayout    : fixedLayout,
       isSubmitting   : false,
       conflictsData  : {},
       resolutionData : {}
@@ -192,14 +195,17 @@ class MergeConflictDataProvider {
   updateViewType(newType) {
     const vi = this.vueInstance;
 
-    if (newType === vi.diffView || !(newType === 'parallel' || newType === 'inline')) {
+    if (newType === vi.diffViewType || !(newType === 'parallel' || newType === 'inline')) {
       return;
     }
 
-    vi.diffView   = newType;
-    vi.isParallel = newType === 'parallel';
-    $.cookie('diff_view', newType); // TODO: Make sure that cookie path added.
-    $('.content-wrapper .container-fluid').toggleClass('container-limited');
+    vi.diffViewType = newType;
+    vi.isParallel   = newType === 'parallel';
+    $.cookie('diff_view', newType, {
+      path: (gon && gon.relative_url_root) || '/'
+    });
+    $('.content-wrapper .container-fluid')
+      .toggleClass('container-limited', !vi.isParallel && vi.fixedLayout);
   }
 
 
diff --git a/app/assets/javascripts/merge_conflict_resolver.js.es6 b/app/assets/javascripts/merge_conflict_resolver.js.es6
index b56fd5aa658..7e756433bf5 100644
--- a/app/assets/javascripts/merge_conflict_resolver.js.es6
+++ b/app/assets/javascripts/merge_conflict_resolver.js.es6
@@ -60,9 +60,8 @@ class MergeConflictResolver {
           $('#conflicts .js-syntax-highlight').syntaxHighlight();
         });
 
-        if (this.vue.diffViewType === 'parallel') {
-          $('.content-wrapper .container-fluid').removeClass('container-limited');
-        }
+        $('.content-wrapper .container-fluid')
+          .toggleClass('container-limited', !this.vue.isParallel && this.vue.fixedLayout);
       })
   }
 
diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js
index 05644b3d03c..02ff5a382e2 100644
--- a/app/assets/javascripts/merge_request.js
+++ b/app/assets/javascripts/merge_request.js
@@ -36,13 +36,10 @@
     };
 
     MergeRequest.prototype.initTabs = function() {
-      if (this.opts.action !== 'new') {
-        // `MergeRequests#new` has no tab-persisting or lazy-loading behavior
-        window.mrTabs = new MergeRequestTabs(this.opts);
-      } else {
-        // Show the first tab (Commits)
-        return $('.merge-request-tabs a[data-toggle="tab"]:first').tab('show');
+      if (window.mrTabs) {
+        window.mrTabs.unbindEvents();
       }
+      window.mrTabs = new MergeRequestTabs(this.opts);
     };
 
     MergeRequest.prototype.showAllCommits = function() {
diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js
index 18bbfa7a459..bec11a198a1 100644
--- a/app/assets/javascripts/merge_request_tabs.js
+++ b/app/assets/javascripts/merge_request_tabs.js
@@ -56,6 +56,8 @@
 
     MergeRequestTabs.prototype.commitsLoaded = false;
 
+    MergeRequestTabs.prototype.fixedLayoutPref = null;
+
     function MergeRequestTabs(opts) {
       this.opts = opts != null ? opts : {};
       this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true;
@@ -70,7 +72,12 @@
 
     MergeRequestTabs.prototype.bindEvents = function() {
       $(document).on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown);
-      return $(document).on('click', '.js-show-tab', this.showTab);
+      $(document).on('click', '.js-show-tab', this.showTab);
+    };
+
+    MergeRequestTabs.prototype.unbindEvents = function() {
+      $(document).off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown);
+      $(document).off('click', '.js-show-tab', this.showTab);
     };
 
     MergeRequestTabs.prototype.showTab = function(event) {
@@ -85,11 +92,15 @@
       if (action === 'commits') {
         this.loadCommits($target.attr('href'));
         this.expandView();
+        this.resetViewContainer();
       } else if (action === 'diffs') {
         this.loadDiff($target.attr('href'));
         if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') {
           this.shrinkView();
         }
+        if (this.diffViewType() === 'parallel') {
+          this.expandViewContainer();
+        }
         navBarHeight = $('.navbar-gitlab').outerHeight();
         $.scrollTo(".merge-request-details .merge-request-tabs", {
           offset: -navBarHeight
@@ -97,11 +108,14 @@
       } else if (action === 'builds') {
         this.loadBuilds($target.attr('href'));
         this.expandView();
+        this.resetViewContainer();
       } else if (action === 'pipelines') {
         this.loadPipelines($target.attr('href'));
         this.expandView();
+        this.resetViewContainer();
       } else {
         this.expandView();
+        this.resetViewContainer();
       }
       if (this.opts.setUrl) {
         this.setCurrentAction(action);
@@ -126,7 +140,7 @@
       if (action === 'show') {
         action = 'notes';
       }
-      return $(".merge-request-tabs a[data-action='" + action + "']").tab('show');
+      $(".merge-request-tabs a[data-action='" + action + "']").tab('show').trigger('shown.bs.tab');
     };
 
     // Replaces the current Merge Request-specific action in the URL with a new one
@@ -209,7 +223,7 @@
             gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'));
             $('#diffs .js-syntax-highlight').syntaxHighlight();
             $('#diffs .diff-file').singleFileDiff();
-            if (_this.diffViewType() === 'parallel') {
+            if (_this.diffViewType() === 'parallel' && _this.currentAction === 'diffs') {
               _this.expandViewContainer();
             }
             _this.diffsLoaded = true;
@@ -308,11 +322,21 @@
 
     MergeRequestTabs.prototype.diffViewType = function() {
       return $('.inline-parallel-buttons a.active').data('view-type');
-    // Returns diff view type
     };
 
     MergeRequestTabs.prototype.expandViewContainer = function() {
-      return $('.container-fluid').removeClass('container-limited');
+      var $wrapper = $('.content-wrapper .container-fluid');
+      if (this.fixedLayoutPref === null) {
+        this.fixedLayoutPref = $wrapper.hasClass('container-limited');
+      }
+      $wrapper.removeClass('container-limited');
+    };
+
+    MergeRequestTabs.prototype.resetViewContainer = function() {
+      if (this.fixedLayoutPref !== null) {
+        $('.content-wrapper .container-fluid')
+          .toggleClass('container-limited', this.fixedLayoutPref);
+      }
     };
 
     MergeRequestTabs.prototype.shrinkView = function() {
diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb
index 22387d66451..7d4d049101a 100644
--- a/app/helpers/page_layout_helper.rb
+++ b/app/helpers/page_layout_helper.rb
@@ -92,12 +92,8 @@ module PageLayoutHelper
     end
   end
 
-  def fluid_layout(enabled = false)
-    if @fluid_layout.nil?
-      @fluid_layout = (current_user && current_user.layout == "fluid") || enabled
-    else
-      @fluid_layout
-    end
+  def fluid_layout
+    current_user && current_user.layout == "fluid"
   end
 
   def blank_container(enabled = false)
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index 94c53882623..237280872f1 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -1,5 +1,5 @@
 %header.navbar.navbar-fixed-top.navbar-gitlab{ class: nav_header_class }
-  %div{ class: fluid_layout ? "container-fluid" : "container-fluid" }
+  %div{ class: "container-fluid" }
     .header-content
       %button.side-nav-toggle{ type: 'button', "aria-label" => "Toggle global navigation" }
         %span.sr-only Toggle navigation
diff --git a/app/views/profiles/preferences/update.js.erb b/app/views/profiles/preferences/update.js.erb
index 4433cab7782..8966dd3fd86 100644
--- a/app/views/profiles/preferences/update.js.erb
+++ b/app/views/profiles/preferences/update.js.erb
@@ -4,9 +4,9 @@ $('body').addClass('<%= user_application_theme %>')
 
 // Toggle container-fluid class
 if ('<%= current_user.layout %>' === 'fluid') {
-  $('.content-wrapper').find('.container-fluid').removeClass('container-limited')
+  $('.content-wrapper .container-fluid').removeClass('container-limited')
 } else {
-  $('.content-wrapper').find('.container-fluid').addClass('container-limited')
+  $('.content-wrapper .container-fluid').addClass('container-limited')
 }
 
 // Re-enable the "Save" button
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 62aff36aadd..576e7ef021a 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -1,7 +1,5 @@
 - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
 - diff_files = diffs.diff_files
-- if diff_view == :parallel
-  - fluid_layout true
 
 .content-block.oneline-block.files-changed
   .inline-parallel-buttons
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index d03ff9ec7e8..9f34ca9ff4e 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -4,9 +4,6 @@
 - content_for :page_specific_javascripts do
   = page_specific_javascript_tag('diff_notes/diff_notes_bundle.js')
 
-- if diff_view == :parallel
-  - fluid_layout true
-
 .merge-request{'data-url' => merge_request_path(@merge_request)}
   = render "projects/merge_requests/show/mr_title"