FEATURE: Serve RTL versions of admin and plugins CSS bundles for RTL locales (#21876)

Prior to this commit, we didn't have RTL versions of our admin and plugins CSS bundles and we always served LTR versions of those bundles even when users used an RTL locale, causing admin and plugins UI elements to never look as good as when an LTR locale was used. Example of UI issues prior to this commit were: missing margins, borders on the wrong side and buttons too close to each other etc.

This commit creates an RTL version for the admin CSS bundle as well as RTL bundles for all the installed plugins and serves those RTL bundles to users/sites who use RTL locales.

app/assets/stylesheets/admin_rtl.scss

@@ -0,0 +1 @@
+@import "admin";

app/assets/stylesheets/common/base/rtl.scss

@@ -24,15 +24,6 @@
   }
 }
 
-.rtl .admin-customize .current-style .toggle-mobile {
-  position: static !important;
-  float: left !important;
-}
-.rtl .admin-customize .current-style .toggle-maximize {
-  position: static !important;
-  float: left !important;
-}
-
 // For the support_mixed_text_direction setting
 html:not(.rtl) .cooked ul[dir="rtl"],
 html:not(.rtl) .d-editor-preview ul[dir="rtl"],
@@ -43,36 +34,6 @@ html:not(.rtl) .d-editor-preview ul[dir="rtl"],
   margin-right: 1.25em;
 }
 
-.rtl {
-  $mobile-breakpoint: 700px;
-
-  .admin-detail.mobile-open {
-    @media (max-width: $mobile-breakpoint) {
-      transition: transform 0.3s ease;
-      @include transform(translateX(-250px));
-    }
-    @media (max-width: 500px) {
-      transition: transform 0.3s ease;
-      @include transform(translateX(-50%));
-    }
-  }
-
-  .admin-detail.mobile-closed {
-    @media (max-width: $mobile-breakpoint) {
-      transition: transform 0.3s ease;
-      @include transform(translateX(0));
-      margin-left: -10px !important;
-      padding-left: 10px !important;
-    }
-  }
-
-  .admin-nav {
-    .nav-stacked {
-      margin: 0 -10px 0 10px !important;
-    }
-  }
-}
-
 .rtl .ace_placeholder {
   direction: rtl !important;
   text-align: right !important;

app/assets/stylesheets/wizard_rtl.scss

@@ -0,0 +1 @@
+@import "wizard";

app/controllers/bootstrap_controller.rb

@@ -26,8 +26,8 @@ class BootstrapController < ApplicationController
     else
       add_style(mobile_view? ? :mobile : :desktop)
     end
-    add_style(:admin) if staff?
-    add_style(:wizard) if admin?
+    add_style(rtl? ? :admin_rtl : :admin) if staff?
+    add_style(rtl? ? :wizard_rtl : :wizard) if admin?
 
     assets_fake_request = ActionDispatch::Request.new(request.env.dup)
     assets_for_url = params[:for_url]
@@ -44,6 +44,7 @@ class BootstrapController < ApplicationController
         mobile_view: mobile_view?,
         desktop_view: !mobile_view?,
         request: assets_fake_request,
+        rtl: rtl?,
       )
       .each { |file| add_style(file, plugin: true) }
     add_style(mobile_view? ? :mobile_theme : :desktop_theme) if theme_id.present?

app/views/common/_discourse_preload_stylesheet.html.erb

@@ -7,14 +7,22 @@
 <%- end %>
 
 <%- if staff? %>
-  <%= discourse_stylesheet_preload_tag(:admin) %>
+  <%- if rtl? %>
+    <%= discourse_stylesheet_preload_tag(:admin_rtl) %>
+  <%- else %>
+    <%= discourse_stylesheet_preload_tag(:admin) %>
+  <%- end %>
 <%- end %>
 
 <%- if admin? %>
-  <%= discourse_stylesheet_preload_tag(:wizard) %>
+  <%- if rtl? %>
+    <%= discourse_stylesheet_preload_tag(:wizard_rtl) %>
+  <%- else %>
+    <%= discourse_stylesheet_preload_tag(:wizard) %>
+  <%- end %>
 <%- end %>
 
-<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request).each do |file| %>
+<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request, rtl: rtl?).each do |file| %>
   <%= discourse_stylesheet_preload_tag(file) %>
 <%- end %>
 

app/views/common/_discourse_publish_stylesheet.html.erb

@@ -1,6 +1,6 @@
 <%= discourse_stylesheet_link_tag 'publish', theme_id: nil %>
 
-<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request).each do |file| %>
+<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request, rtl: rtl?).each do |file| %>
   <%= discourse_stylesheet_link_tag(file) %>
 <%- end %>
 

app/views/common/_discourse_stylesheet.html.erb

@@ -7,14 +7,22 @@
 <%- end %>
 
 <%- if staff? %>
-  <%= discourse_stylesheet_link_tag(:admin) %>
+  <%- if rtl? %>
+    <%= discourse_stylesheet_link_tag(:admin_rtl) %>
+  <%- else %>
+    <%= discourse_stylesheet_link_tag(:admin) %>
+  <%- end %>
 <%- end %>
 
 <%- if admin? %>
-  <%= discourse_stylesheet_link_tag(:wizard) %>
+  <%- if rtl? %>
+    <%= discourse_stylesheet_link_tag(:wizard_rtl) %>
+  <%- else %>
+    <%= discourse_stylesheet_link_tag(:wizard) %>
+  <%- end %>
 <%- end %>
 
-<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request).each do |file| %>
+<%- Discourse.find_plugin_css_assets(include_official: allow_plugins?, include_unofficial: allow_third_party_plugins?, mobile_view: mobile_view?, desktop_view: !mobile_view?, request: request, rtl: rtl?).each do |file| %>
   <%= discourse_stylesheet_link_tag(file) %>
 <%- end %>
 

lib/discourse.rb

@@ -431,6 +431,7 @@ module Discourse
           end
     end
 
+    assets.map! { |asset| "#{asset}_rtl" } if args[:rtl]
     assets
   end
 

lib/stylesheet/manager.rb

@@ -43,13 +43,21 @@ class Stylesheet::Manager
   end
 
   def self.precompile_css
-    targets = %i[desktop mobile desktop_rtl mobile_rtl admin wizard]
+    targets = %i[desktop mobile admin wizard desktop_rtl mobile_rtl admin_rtl wizard_rtl]
+
     targets +=
       Discourse.find_plugin_css_assets(
         include_disabled: true,
         mobile_view: true,
         desktop_view: true,
       )
+    targets +=
+      Discourse.find_plugin_css_assets(
+        include_disabled: true,
+        mobile_view: true,
+        desktop_view: true,
+        rtl: true,
+      )
 
     targets.each do |target|
       $stderr.puts "precompile target: #{target}"

lib/stylesheet/manager/builder.rb

@@ -35,11 +35,11 @@ class Stylesheet::Manager::Builder
       end
     end
 
-    rtl = @target.to_s =~ /_rtl\z/
+    rtl = @target.to_s.end_with?("_rtl")
     css, source_map =
       with_load_paths do |load_paths|
         Stylesheet::Compiler.compile_asset(
-          @target,
+          @target.to_s.gsub(/_rtl\z/, "").to_sym,
           rtl: rtl,
           theme_id: theme&.id,
           theme_variables: theme&.scss_variables.to_s,

spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss

@@ -8,6 +8,8 @@ body {
   line-height: $lineheight;
 }
 
+.pull-left { float: left; }
+
 footer {
   border-color: $footer1;
 }

spec/lib/stylesheet/compiler_spec.rb

@@ -89,6 +89,20 @@ RSpec.describe Stylesheet::Compiler do
         expect(css).to include("background:")
       end
 
+      context "with the `rtl` option" do
+        it "generates an RTL version of the plugin CSS if the option is true" do
+          css, _ = Stylesheet::Compiler.compile_asset("scss_plugin", theme_id: theme.id, rtl: true)
+          expect(css).to include(".pull-left{float:right}")
+          expect(css).not_to include(".pull-left{float:left}")
+        end
+
+        it "returns an unchanged version of the plugin CSS" do
+          css, _ = Stylesheet::Compiler.compile_asset("scss_plugin", theme_id: theme.id, rtl: false)
+          expect(css).to include(".pull-left{float:left}")
+          expect(css).not_to include(".pull-left{float:right}")
+        end
+      end
+
       it "supports SCSS imports" do
         css, _map = Stylesheet::Compiler.compile_asset("scss_plugin", theme_id: theme.id)
 

spec/lib/stylesheet/manager_spec.rb

@@ -877,7 +877,11 @@ RSpec.describe Stylesheet::Manager do
     end
   end
 
-  describe ".precompile css" do
+  describe ".precompile_css" do
+    let(:core_targets) do
+      %w[desktop mobile admin wizard desktop_rtl mobile_rtl admin_rtl wizard_rtl]
+    end
+
     before { STDERR.stubs(:write) }
 
     after do
@@ -888,7 +892,6 @@ RSpec.describe Stylesheet::Manager do
     it "correctly generates precompiled CSS" do
       scheme1 = ColorScheme.create!(name: "scheme1")
       scheme2 = ColorScheme.create!(name: "scheme2")
-      core_targets = %i[desktop mobile desktop_rtl mobile_rtl admin wizard]
       theme_targets = %i[desktop_theme mobile_theme]
 
       Theme.update_all(user_selectable: false)
@@ -922,7 +925,7 @@ RSpec.describe Stylesheet::Manager do
       output = capture_output(:stderr) { Stylesheet::Manager.precompile_css }
 
       results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(core_targets.size)
+      expect(results).to contain_exactly(*core_targets)
 
       StylesheetCache.destroy_all
 
@@ -937,7 +940,7 @@ RSpec.describe Stylesheet::Manager do
       # themes + core
       Stylesheet::Manager.precompile_css
       results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(28) # 9 core targets + 9 theme + 10 color schemes
+      expect(results.size).to eq(30) # 11 core targets + 9 theme + 10 color schemes
 
       theme_targets.each do |tar|
         expect(
@@ -952,7 +955,7 @@ RSpec.describe Stylesheet::Manager do
       Stylesheet::Manager.precompile_css
       Stylesheet::Manager.precompile_theme_css
       results = StylesheetCache.pluck(:target)
-      expect(results.size).to eq(28) # 9 core targets + 9 theme + 10 color schemes
+      expect(results.size).to eq(30) # 11 core targets + 9 theme + 10 color schemes
 
       expect(results).to include("color_definitions_#{scheme1.name}_#{scheme1.id}_#{user_theme.id}")
       expect(results).to include(
@@ -969,7 +972,6 @@ RSpec.describe Stylesheet::Manager do
       upload = UploadCreator.new(image, "logo.png").create_for(-1)
 
       scheme = ColorScheme.create!(name: "scheme")
-      core_targets = %i[desktop mobile desktop_rtl mobile_rtl admin wizard]
       theme_targets = %i[desktop_theme mobile_theme]
 
       default_theme =
@@ -1009,6 +1011,71 @@ RSpec.describe Stylesheet::Manager do
       css = File.read(theme_builder.stylesheet_fullpath)
       expect(css).to include("border:3px solid green}")
     end
+
+    context "when there are enabled plugins" do
+      let(:plugin1) do
+        plugin1 = Plugin::Instance.new
+        plugin1.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb"
+        plugin1.register_css "body { padding: 1px 2px 3px 4px; }"
+        plugin1
+      end
+
+      let(:plugin2) do
+        plugin2 = Plugin::Instance.new
+        plugin2.path = "#{Rails.root}/spec/fixtures/plugins/scss_plugin/plugin.rb"
+        plugin2
+      end
+
+      before do
+        Discourse.plugins << plugin1
+        Discourse.plugins << plugin2
+        plugin1.activate!
+        plugin2.activate!
+        Stylesheet::Importer.register_imports!
+        StylesheetCache.destroy_all
+      end
+
+      after do
+        Discourse.plugins.delete(plugin1)
+        Discourse.plugins.delete(plugin2)
+        Stylesheet::Importer.register_imports!
+        DiscoursePluginRegistry.reset!
+      end
+
+      it "generates LTR and RTL CSS for plugins" do
+        output = capture_output(:stderr) { Stylesheet::Manager.precompile_css }
+
+        results = StylesheetCache.pluck(:target)
+        expect(results).to contain_exactly(
+          *core_targets,
+          "my_plugin",
+          "my_plugin_rtl",
+          "scss_plugin",
+          "scss_plugin_rtl",
+        )
+
+        expect(output.scan(/my_plugin$/).length).to eq(1)
+        expect(output.scan(/my_plugin_rtl$/).length).to eq(1)
+        expect(output.scan(/scss_plugin$/).length).to eq(1)
+        expect(output.scan(/scss_plugin_rtl$/).length).to eq(1)
+
+        plugin1_ltr_css = StylesheetCache.where(target: "my_plugin").pluck(:content).first
+        plugin1_rtl_css = StylesheetCache.where(target: "my_plugin_rtl").pluck(:content).first
+
+        expect(plugin1_ltr_css).to include("body{padding:1px 2px 3px 4px}")
+        expect(plugin1_ltr_css).not_to include("body{padding:1px 4px 3px 2px}")
+        expect(plugin1_rtl_css).to include("body{padding:1px 4px 3px 2px}")
+        expect(plugin1_rtl_css).not_to include("body{padding:1px 2px 3px 4px}")
+
+        plugin2_ltr_css = StylesheetCache.where(target: "scss_plugin").pluck(:content).first
+        plugin2_rtl_css = StylesheetCache.where(target: "scss_plugin_rtl").pluck(:content).first
+
+        expect(plugin2_ltr_css).to include(".pull-left{float:left}")
+        expect(plugin2_ltr_css).not_to include(".pull-left{float:right}")
+        expect(plugin2_rtl_css).to include(".pull-left{float:right}")
+        expect(plugin2_rtl_css).not_to include(".pull-left{float:left}")
+      end
+    end
   end
 
   describe ".fs_asset_cachebuster" do

spec/requests/embed_controller_spec.rb

@@ -211,7 +211,7 @@ RSpec.describe EmbedController do
         )
 
         topic_embed = Fabricate(:topic_embed, embed_url: embed_url)
-        post = Fabricate(:post, topic: topic_embed.topic)
+        Fabricate(:post, topic: topic_embed.topic)
 
         get "/embed/comments", params: { embed_url: embed_url }, headers: { "REFERER" => embed_url }
 

spec/requests/stylesheets_controller_spec.rb

@@ -60,6 +60,63 @@ RSpec.describe StylesheetsController do
     expect(response.status).to eq(200)
   end
 
+  context "when there are enabled plugins" do
+    fab!(:user) { Fabricate(:user) }
+
+    let(:plugin) do
+      plugin = Plugin::Instance.new
+      plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb"
+      plugin.register_css "body { padding: 1px 2px 3px 4px; }"
+      plugin
+    end
+
+    before do
+      Discourse.plugins << plugin
+      plugin.activate!
+      Stylesheet::Importer.register_imports!
+      StylesheetCache.destroy_all
+      SiteSetting.has_login_hint = false
+      SiteSetting.allow_user_locale = true
+      sign_in(user)
+    end
+
+    after do
+      Discourse.plugins.delete(plugin)
+      Stylesheet::Importer.register_imports!
+      DiscoursePluginRegistry.reset!
+    end
+
+    it "can lookup plugin specific css" do
+      get "/"
+
+      html = Nokogiri::HTML5.fragment(response.body)
+      expect(html.at("link[data-target=my_plugin_rtl]")).to eq(nil)
+
+      href = html.at("link[data-target=my_plugin]").attribute("href").value
+      get href
+
+      expect(response.status).to eq(200)
+      expect(response.headers["Content-Type"]).to eq("text/css")
+      expect(response.body).to include("body{padding:1px 2px 3px 4px}")
+      expect(response.body).not_to include("body{padding:1px 4px 3px 2px}")
+
+      user.locale = "ar" # RTL locale
+      user.save!
+      get "/"
+
+      html = Nokogiri::HTML5.fragment(response.body)
+      expect(html.at("link[data-target=my_plugin]")).to eq(nil)
+
+      href = html.at("link[data-target=my_plugin_rtl]").attribute("href").value
+      get href
+
+      expect(response.status).to eq(200)
+      expect(response.headers["Content-Type"]).to eq("text/css")
+      expect(response.body).to include("body{padding:1px 4px 3px 2px}")
+      expect(response.body).not_to include("body{padding:1px 2px 3px 4px}")
+    end
+  end
+
   it "ignores Accept header and does not include Vary header" do
     StylesheetCache.destroy_all
     manager = Stylesheet::Manager.new(theme_id: nil)