From c2fcd55a802a7cad85bbbd84ddbe8c1b4ef4140e Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 1 Jun 2023 05:27:11 +0300 Subject: [PATCH] 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 | 1 + app/assets/stylesheets/common/base/rtl.scss | 39 --------- app/assets/stylesheets/wizard_rtl.scss | 1 + app/controllers/bootstrap_controller.rb | 5 +- .../_discourse_preload_stylesheet.html.erb | 14 +++- .../_discourse_publish_stylesheet.html.erb | 2 +- .../common/_discourse_stylesheet.html.erb | 14 +++- lib/discourse.rb | 1 + lib/stylesheet/manager.rb | 10 ++- lib/stylesheet/manager/builder.rb | 4 +- .../assets/stylesheets/common/common.scss | 2 + spec/lib/stylesheet/compiler_spec.rb | 14 ++++ spec/lib/stylesheet/manager_spec.rb | 79 +++++++++++++++++-- spec/requests/embed_controller_spec.rb | 2 +- spec/requests/stylesheets_controller_spec.rb | 57 +++++++++++++ 15 files changed, 187 insertions(+), 58 deletions(-) create mode 100644 app/assets/stylesheets/admin_rtl.scss create mode 100644 app/assets/stylesheets/wizard_rtl.scss diff --git a/app/assets/stylesheets/admin_rtl.scss b/app/assets/stylesheets/admin_rtl.scss new file mode 100644 index 00000000000..1c1bb0974c0 --- /dev/null +++ b/app/assets/stylesheets/admin_rtl.scss @@ -0,0 +1 @@ +@import "admin"; diff --git a/app/assets/stylesheets/common/base/rtl.scss b/app/assets/stylesheets/common/base/rtl.scss index fcfa7f904af..6959a71547e 100644 --- a/app/assets/stylesheets/common/base/rtl.scss +++ b/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; diff --git a/app/assets/stylesheets/wizard_rtl.scss b/app/assets/stylesheets/wizard_rtl.scss new file mode 100644 index 00000000000..f4274872a20 --- /dev/null +++ b/app/assets/stylesheets/wizard_rtl.scss @@ -0,0 +1 @@ +@import "wizard"; diff --git a/app/controllers/bootstrap_controller.rb b/app/controllers/bootstrap_controller.rb index 48a31a6e16c..575dd25f78d 100644 --- a/app/controllers/bootstrap_controller.rb +++ b/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? diff --git a/app/views/common/_discourse_preload_stylesheet.html.erb b/app/views/common/_discourse_preload_stylesheet.html.erb index 06402715769..efe1ac15d44 100644 --- a/app/views/common/_discourse_preload_stylesheet.html.erb +++ b/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 %> diff --git a/app/views/common/_discourse_publish_stylesheet.html.erb b/app/views/common/_discourse_publish_stylesheet.html.erb index 9429b0cd8dd..5a7b6a9bb72 100644 --- a/app/views/common/_discourse_publish_stylesheet.html.erb +++ b/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 %> diff --git a/app/views/common/_discourse_stylesheet.html.erb b/app/views/common/_discourse_stylesheet.html.erb index ca92af30768..68dd71ab7bb 100644 --- a/app/views/common/_discourse_stylesheet.html.erb +++ b/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 %> diff --git a/lib/discourse.rb b/lib/discourse.rb index 97c1954e1b8..bbb568246fa 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -431,6 +431,7 @@ module Discourse end end + assets.map! { |asset| "#{asset}_rtl" } if args[:rtl] assets end diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index a9d636b6c3b..7f71831ba87 100644 --- a/lib/stylesheet/manager.rb +++ b/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}" diff --git a/lib/stylesheet/manager/builder.rb b/lib/stylesheet/manager/builder.rb index 47a846e1403..5f4fbabbaa0 100644 --- a/lib/stylesheet/manager/builder.rb +++ b/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, diff --git a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss b/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss index 0ff1ecfff14..bcf35618fbb 100644 --- a/spec/fixtures/plugins/scss_plugin/assets/stylesheets/common/common.scss +++ b/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; } diff --git a/spec/lib/stylesheet/compiler_spec.rb b/spec/lib/stylesheet/compiler_spec.rb index 3334879d3ee..753c54a3104 100644 --- a/spec/lib/stylesheet/compiler_spec.rb +++ b/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) diff --git a/spec/lib/stylesheet/manager_spec.rb b/spec/lib/stylesheet/manager_spec.rb index 5c00f4e6742..433f99decbe 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/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 diff --git a/spec/requests/embed_controller_spec.rb b/spec/requests/embed_controller_spec.rb index 0b00ab6e633..d430bdd657d 100644 --- a/spec/requests/embed_controller_spec.rb +++ b/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 } diff --git a/spec/requests/stylesheets_controller_spec.rb b/spec/requests/stylesheets_controller_spec.rb index 4622d5e163b..38a03e4706d 100644 --- a/spec/requests/stylesheets_controller_spec.rb +++ b/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)