Stop ordering attribute values

Fix #161

This is porting https://github.com/haml/haml/pull/882 to Hamlit.
Note that it also required to port some changes which I made in Haml parser.

ext/hamlit/hamlit.c

@@ -152,7 +152,6 @@ hamlit_build_multi_class(VALUE escape_attrs, VALUE values)
     }
   }
 
-  rb_ary_sort_bang(buf);
   rb_funcall(buf, id_uniq_bang, 0);
 
   return escape_attribute(escape_attrs, rb_ary_join(buf, str_space()));

lib/hamlit/attribute_builder.rb

@@ -47,7 +47,7 @@ module Hamlit::AttributeBuilder
           when value.is_a?(String)
             # noop
           when value.is_a?(Array)
-            value = value.flatten.select { |v| v }.map(&:to_s).sort.uniq.join(' ')
+            value = value.flatten.select { |v| v }.map(&:to_s).uniq.join(' ')
           when value
             value = value.to_s
           else
@@ -67,7 +67,7 @@ module Hamlit::AttributeBuilder
             classes << value.to_s
           end
         end
-        escape_html(escape_attrs, classes.map(&:to_s).sort.uniq.join(' '))
+        escape_html(escape_attrs, classes.map(&:to_s).uniq.join(' '))
       end
 
       def build_data(escape_attrs, quote, *hashes)

lib/hamlit/attribute_compiler.rb

@@ -17,7 +17,7 @@ module Hamlit
       if node.value[:object_ref] != :nil || !Ripper.respond_to?(:lex) # No Ripper.lex in truffleruby
         return runtime_compile(node)
       end
-      node.value[:attributes_hashes].each do |attribute_str|
+      [node.value[:dynamic_attributes].new, node.value[:dynamic_attributes].old].compact.each do |attribute_str|
         hash = AttributeParser.parse(attribute_str)
         return runtime_compile(node) unless hash
         hashes << hash
@@ -28,11 +28,11 @@ module Hamlit
     private
 
     def runtime_compile(node)
-      attrs = node.value[:attributes_hashes]
+      attrs = []
       attrs.unshift(node.value[:attributes].inspect) if node.value[:attributes] != {}
 
       args = [@escape_attrs.inspect, "#{@quote.inspect}.freeze", @format.inspect].push(node.value[:object_ref]) + attrs
-      [:html, :attrs, [:dynamic, "::Hamlit::AttributeBuilder.build(#{args.join(', ')})"]]
+      [:html, :attrs, [:dynamic, "::Hamlit::AttributeBuilder.build(#{args.join(', ')}, #{node.value[:dynamic_attributes].to_literal})"]]
     end
 
     def static_compile(static_hash, dynamic_hashes)

lib/hamlit/compiler/children_compiler.rb

@@ -39,7 +39,7 @@ module Hamlit
         when :script, :silent_script
           @lineno += 1
         when :tag
-          node.value[:attributes_hashes].each do |attribute_hash|
+          [node.value[:dynamic_attributes].new, node.value[:dynamic_attributes].old].compact.each do |attribute_hash|
             @lineno += attribute_hash.count("\n")
           end
           @lineno += 1 if node.children.empty? && node.value[:parse]

lib/hamlit/parser/haml_attribute_builder.rb

@@ -0,0 +1,164 @@
+# frozen_string_literal: true
+
+module Hamlit
+  module HamlAttributeBuilder
+    # https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
+    INVALID_ATTRIBUTE_NAME_REGEX = /[ \0"'>\/=]/
+
+    class << self
+      def build_attributes(is_html, attr_wrapper, escape_attrs, hyphenate_data_attrs, attributes = {})
+        # @TODO this is an absolutely ridiculous amount of arguments. At least
+        # some of this needs to be moved into an instance method.
+        join_char = hyphenate_data_attrs ? '-' : '_'
+
+        attributes.each do |key, value|
+          if value.is_a?(Hash)
+            data_attributes = attributes.delete(key)
+            data_attributes = flatten_data_attributes(data_attributes, '', join_char)
+            data_attributes = build_data_keys(data_attributes, hyphenate_data_attrs, key)
+            verify_attribute_names!(data_attributes.keys)
+            attributes = data_attributes.merge(attributes)
+          end
+        end
+
+        result = attributes.collect do |attr, value|
+          next if value.nil?
+
+          value = filter_and_join(value, ' ') if attr == 'class'
+          value = filter_and_join(value, '_') if attr == 'id'
+
+          if value == true
+            next " #{attr}" if is_html
+            next " #{attr}=#{attr_wrapper}#{attr}#{attr_wrapper}"
+          elsif value == false
+            next
+          end
+
+          value =
+            if escape_attrs == :once
+              Hamlit::HamlHelpers.escape_once_without_haml_xss(value.to_s)
+            elsif escape_attrs
+              Hamlit::HamlHelpers.html_escape_without_haml_xss(value.to_s)
+            else
+              value.to_s
+            end
+          " #{attr}=#{attr_wrapper}#{value}#{attr_wrapper}"
+        end
+        result.compact!
+        result.sort!
+        result.join
+      end
+
+      # @return [String, nil]
+      def filter_and_join(value, separator)
+        return '' if (value.respond_to?(:empty?) && value.empty?)
+
+        if value.is_a?(Array)
+          value = value.flatten
+          value.map! {|item| item ? item.to_s : nil}
+          value.compact!
+          value = value.join(separator)
+        else
+          value = value ? value.to_s : nil
+        end
+        !value.nil? && !value.empty? && value
+      end
+
+      # Merges two attribute hashes.
+      # This is the same as `to.merge!(from)`,
+      # except that it merges id, class, and data attributes.
+      #
+      # ids are concatenated with `"_"`,
+      # and classes are concatenated with `" "`.
+      # data hashes are simply merged.
+      #
+      # Destructively modifies `to`.
+      #
+      # @param to [{String => String,Hash}] The attribute hash to merge into
+      # @param from [{String => Object}] The attribute hash to merge from
+      # @return [{String => String,Hash}] `to`, after being merged
+      def merge_attributes!(to, from)
+        from.keys.each do |key|
+          to[key] = merge_value(key, to[key], from[key])
+        end
+        to
+      end
+
+      # Merge multiple values to one attribute value. No destructive operation.
+      #
+      # @param key [String]
+      # @param values [Array<Object>]
+      # @return [String,Hash]
+      def merge_values(key, *values)
+        values.inject(nil) do |to, from|
+          merge_value(key, to, from)
+        end
+      end
+
+      def verify_attribute_names!(attribute_names)
+        attribute_names.each do |attribute_name|
+          if attribute_name =~ INVALID_ATTRIBUTE_NAME_REGEX
+            raise InvalidAttributeNameError.new("Invalid attribute name '#{attribute_name}' was rendered")
+          end
+        end
+      end
+
+      private
+
+      # Merge a couple of values to one attribute value. No destructive operation.
+      #
+      # @param to [String,Hash,nil]
+      # @param from [Object]
+      # @return [String,Hash]
+      def merge_value(key, to, from)
+        if from.kind_of?(Hash) || to.kind_of?(Hash)
+          from = { nil => from } if !from.is_a?(Hash)
+          to   = { nil => to }   if !to.is_a?(Hash)
+          to.merge(from)
+        elsif key == 'id'
+          merged_id = filter_and_join(from, '_')
+          if to && merged_id
+            merged_id = "#{to}_#{merged_id}"
+          elsif to || merged_id
+            merged_id ||= to
+          end
+          merged_id
+        elsif key == 'class'
+          merged_class = filter_and_join(from, ' ')
+          if to && merged_class
+            merged_class = (to.split(' ') | merged_class.split(' ')).join(' ')
+          elsif to || merged_class
+            merged_class ||= to
+          end
+          merged_class
+        else
+          from
+        end
+      end
+
+      def build_data_keys(data_hash, hyphenate, attr_name="data")
+        Hash[data_hash.map do |name, value|
+          if name == nil
+            [attr_name, value]
+          elsif hyphenate
+            ["#{attr_name}-#{name.to_s.tr('_', '-')}", value]
+          else
+            ["#{attr_name}-#{name}", value]
+          end
+        end]
+      end
+
+      def flatten_data_attributes(data, key, join_char, seen = [])
+        return {key => data} unless data.is_a?(Hash)
+
+        return {key => nil} if seen.include? data.object_id
+        seen << data.object_id
+
+        data.sort {|x, y| x[0].to_s <=> y[0].to_s}.inject({}) do |hash, (k, v)|
+          joined = key == '' ? k : [key, k].join(join_char)
+          hash.merge! flatten_data_attributes(v, joined, join_char, seen)
+        end
+      end
+    end
+  end
+end

lib/hamlit/parser/haml_helpers.rb

@@ -617,6 +617,9 @@ MESSAGE
       text.gsub(HTML_ESCAPE_REGEX, HTML_ESCAPE)
     end
 
+    # Always escape text regardless of html_safe?
+    alias_method :html_escape_without_haml_xss, :html_escape
+
     HTML_ESCAPE_ONCE_REGEX = /[\"><]|&(?!(?:[a-zA-Z]+|#(?:\d+|[xX][0-9a-fA-F]+));)/
 
     # Escapes HTML entities in `text`, but without escaping an ampersand
@@ -629,6 +632,9 @@ MESSAGE
       text.gsub(HTML_ESCAPE_ONCE_REGEX, HTML_ESCAPE)
     end
 
+    # Always escape text once regardless of html_safe?
+    alias_method :escape_once_without_haml_xss, :escape_once
+
     # Returns whether or not the current template is a Haml template.
     #
     # This function, unlike other {Haml::Helpers} functions,

lib/hamlit/parser/haml_parser.rb

@@ -1,6 +1,7 @@
 require 'strscan'
 require 'hamlit/parser/haml_util'
 require 'hamlit/parser/haml_error'
+require 'hamlit/parser/haml_attribute_builder'
 
 module Hamlit
   class HamlParser
@@ -206,6 +207,31 @@ module Hamlit
       end
     end
 
+    # @param [String] new - Hash literal including dynamic values.
+    # @param [String] old - Hash literal including dynamic values or Ruby literal of multiple Hashes which MUST be interpreted as method's last arguments.
+    DynamicAttributes = Struct.new(:new, :old) do
+      undef :old=
+      def old=(value)
+        unless value =~ /\A{.*}\z/m
+          raise ArgumentError.new('Old attributes must start with "{" and end with "}"')
+        end
+        self[:old] = value
+      end
+
+      # This will be a literal for Haml::Buffer#attributes's last argument, `attributes_hashes`.
+      def to_literal
+        [new, stripped_old].compact.join(', ')
+      end
+
+      private
+
+      # For `%foo{ { foo: 1 }, bar: 2 }`, :old is "{ { foo: 1 }, bar: 2 }" and this method returns " { foo: 1 }, bar: 2 " for last argument.
+      def stripped_old
+        return nil if old.nil?
+        old.sub!(/\A{/, '').sub!(/}\z/m, '')
+      end
+    end
+
     # Processes and deals with lowering indentation.
     def process_indent(line)
       return unless line.tabs <= @template_tabs && @template_tabs > 0
@@ -403,22 +429,20 @@ module Hamlit
       end
 
       attributes = ::Hamlit::HamlParser.parse_class_and_id(attributes)
-      attributes_list = []
+      dynamic_attributes = DynamicAttributes.new
 
       if attributes_hashes[:new]
         static_attributes, attributes_hash = attributes_hashes[:new]
-        ::Hamlit::HamlBuffer.merge_attrs(attributes, static_attributes) if static_attributes
-        attributes_list << attributes_hash
+        HamlAttributeBuilder.merge_attributes!(attributes, static_attributes) if static_attributes
+        dynamic_attributes.new = attributes_hash
       end
 
       if attributes_hashes[:old]
         static_attributes = parse_static_hash(attributes_hashes[:old])
-        ::Hamlit::HamlBuffer.merge_attrs(attributes, static_attributes) if static_attributes
-        attributes_list << attributes_hashes[:old] unless static_attributes || @options.suppress_eval
+        HamlAttributeBuilder.merge_attributes!(attributes, static_attributes) if static_attributes
+        dynamic_attributes.old = attributes_hashes[:old] unless static_attributes || @options.suppress_eval
       end
 
-      attributes_list.compact!
-
       raise ::Hamlit::HamlSyntaxError.new(::Hamlit::HamlError.message(:illegal_nesting_self_closing), @next_line.index) if block_opened? && self_closing
       raise ::Hamlit::HamlSyntaxError.new(::Hamlit::HamlError.message(:no_ruby_code, action), last_line - 1) if parse && value.empty?
       raise ::Hamlit::HamlSyntaxError.new(::Hamlit::HamlError.message(:self_closing_content), last_line - 1) if self_closing && !value.empty?
@@ -433,7 +457,7 @@ module Hamlit
       line = handle_ruby_multiline(line) if parse
 
       ParseNode.new(:tag, line.index + 1, :name => tag_name, :attributes => attributes,
-        :attributes_hashes => attributes_list, :self_closing => self_closing,
+        :dynamic_attributes => dynamic_attributes, :self_closing => self_closing,
         :nuke_inner_whitespace => nuke_inner_whitespace,
         :nuke_outer_whitespace => nuke_outer_whitespace, :object_ref => object_ref,
         :escape_html => escape_html, :preserve_tag => preserve_tag,
@@ -641,7 +665,6 @@ module Hamlit
         raise e
       end
 
-      attributes_hash = attributes_hash[1...-1] if attributes_hash
       return attributes_hash, rest, last_line
     end
 

lib/hamlit/parser/haml_xss_mods.rb

@@ -6,12 +6,15 @@ module Hamlit
     # to work with Rails' XSS protection methods.
     module XssMods
       def self.included(base)
-        %w[html_escape find_and_preserve preserve list_of surround
-           precede succeed capture_haml haml_concat haml_internal_concat haml_indent
-           escape_once].each do |name|
+        %w[find_and_preserve preserve list_of surround
+           precede succeed capture_haml haml_concat haml_internal_concat haml_indent].each do |name|
           base.send(:alias_method, "#{name}_without_haml_xss", name)
           base.send(:alias_method, name, "#{name}_with_haml_xss")
         end
+        # Those two always have _without_haml_xss
+        %w[html_escape escape_once].each do |name|
+          base.send(:alias_method, name, "#{name}_with_haml_xss")
+        end
       end
 
       # Don't escape text that's already safe,

test/haml/engine_test.rb

@@ -151,8 +151,8 @@ class EngineTest < Haml::TestCase
 
   def test_class_attr_with_array
     assert_equal("<p class='a b'>foo</p>\n", render("%p{:class => %w[a b]} foo")) # basic
-    assert_equal("<p class='a b css'>foo</p>\n", render("%p.css{:class => %w[a b]} foo")) # merge with css
-    assert_equal("<p class='b css'>foo</p>\n", render("%p.css{:class => %w[css b]} foo")) # merge uniquely
+    assert_equal("<p class='css a b'>foo</p>\n", render("%p.css{:class => %w[a b]} foo")) # merge with css
+    assert_equal("<p class='css b'>foo</p>\n", render("%p.css{:class => %w[css b]} foo")) # merge uniquely
     assert_equal("<p class='a b c d'>foo</p>\n", render("%p{:class => [%w[a b], %w[c d]]} foo")) # flatten
     assert_equal("<p class='a b'>foo</p>\n", render("%p{:class => [:a, :b] } foo")) # stringify
     # [INCOMPATIBILITY] Hamlit limits boolean attributes
@@ -162,7 +162,7 @@ class EngineTest < Haml::TestCase
     # [INCOMPATIBILITY] Hamlit limits boolean attributes
     # assert_equal("<p>foo</p>\n", render("%p{:class => false} foo")) # single falsey
     assert_equal("<p class=''>foo</p>\n", render("%p{:class => false} foo")) # single falsey
-    assert_equal("<p class='a b html'>foo</p>\n", render("%p(class='html'){:class => %w[a b]} foo")) # html attrs
+    assert_equal("<p class='html a b'>foo</p>\n", render("%p(class='html'){:class => %w[a b]} foo")) # html attrs
   end
 
   def test_id_attr_with_array
@@ -198,7 +198,7 @@ HAML
   def test_attributes_with_to_s
     assert_equal(<<HTML, render(<<HAML))
 <p id='foo_2'></p>
-<p class='2 foo'></p>
+<p class='foo 2'></p>
 <p blaz='2'></p>
 <p 2='2'></p>
 HTML
@@ -1185,8 +1185,8 @@ HAML
   def test_nil_class_with_syntactic_class
     assert_equal("<p class='foo'>nil</p>\n", render("%p.foo{:class => nil} nil"))
     assert_equal("<p class='bar foo'>nil</p>\n", render("%p.bar.foo{:class => nil} nil"))
-    assert_equal("<p class='bar foo'>nil</p>\n", render("%p.foo{{:class => 'bar'}, :class => nil} nil"))
-    assert_equal("<p class='bar foo'>nil</p>\n", render("%p.foo{{:class => nil}, :class => 'bar'} nil"))
+    assert_equal("<p class='foo bar'>nil</p>\n", render("%p.foo{{:class => 'bar'}, :class => nil} nil"))
+    assert_equal("<p class='foo bar'>nil</p>\n", render("%p.foo{{:class => nil}, :class => 'bar'} nil"))
   end
 
   def test_locals

test/haml/haml-spec/ugly_test.rb

@@ -531,7 +531,7 @@ class UglyTest < MiniTest::Test
 
     def test_HTML_style_tag_with_a_CSS_class_and_class_as_an_attribute
       haml    = %q{%p.class2(class='class1')}
-      _html   = %q{<p class='class1 class2'></p>}
+      _html   = %q{<p class='class2 class1'></p>}
       locals  = {}
       options = {}
       haml_result = UglyTest.haml_result(haml, options, locals)
@@ -665,7 +665,7 @@ class UglyTest < MiniTest::Test
 
     def test_Ruby_style_tag_with_a_CSS_class_and_class_as_an_attribute
       haml    = %q{%p.class2{:class => 'class1'}}
-      _html   = %q{<p class='class1 class2'></p>}
+      _html   = %q{<p class='class2 class1'></p>}
       locals  = {}
       options = {}
       haml_result = UglyTest.haml_result(haml, options, locals)

test/haml/results/helpful.xhtml

@@ -3,11 +3,11 @@
 <div>World</div>
 </div>
 <div class='article' id='id_article_1'>id</div>
-<div class='article class' id='article_1'>class</div>
-<div class='article class' id='id_article_1'>id class</div>
+<div class='class article' id='article_1'>class</div>
+<div class='class article' id='id_article_1'>id class</div>
 <div class='article full' id='article_1'>boo</div>
 <div class='article full' id='article_1'>moo</div>
-<div class='article articleFull' id='article_1'>foo</div>
+<div class='articleFull article' id='article_1'>foo</div>
 <span>
 Boo
 </span>

test/hamlit/engine/attributes_test.rb

@@ -264,7 +264,7 @@ describe Hamlit::Engine do
     it { assert_render(%Q|<div class='static' id='static'></div>\n|, %q|.static#static[nil]|) }
     it do
       assert_render(
-        %Q|<a class='dynamic pre_test_object static' id='static_dynamic_pre_test_object_10'></a>\n|,
+        %Q|<a class='static dynamic pre_test_object' id='static_dynamic_pre_test_object_10'></a>\n|,
         %q|%a.static#static[foo, 'pre']{ id: dynamic, class: dynamic }|,
         locals: { foo: TestObject.new(10), dynamic: 'dynamic' },
       )

test/hamlit/engine/new_attribute_test.rb

@@ -63,7 +63,7 @@ describe Hamlit::Engine do
     describe 'element class with attribute class' do
       it 'does not generate double classes' do
         assert_render(<<-HTML.unindent, <<-HAML.unindent)
-          <div class='first item'></div>
+          <div class='item first'></div>
         HTML
           .item(class='first')
         HAML

test/hamlit/engine/old_attribute_test.rb

@@ -140,10 +140,10 @@ describe Hamlit::Engine do
 
       it 'joins attribute class and element class' do
         assert_render(<<-HTML.unindent, <<-HAML.unindent)
-          <div class='bar foo'></div>
-          <div class='bar foo'></div>
-          <div class='bar foo'></div>
-          <div class='bar baz foo'></div>
+          <div class='foo bar'></div>
+          <div class='foo bar'></div>
+          <div class='foo bar'></div>
+          <div class='foo bar baz'></div>
         HTML
           .foo{ class: ['bar'] }
           .foo{ class: ['bar', 'foo'] }
@@ -390,7 +390,7 @@ describe Hamlit::Engine do
     describe 'element class with attribute class' do
       it 'does not generate double classes' do
         assert_render(<<-HTML.unindent, <<-HAML.unindent)
-          <div class='first item'></div>
+          <div class='item first'></div>
         HTML
           .item{ class: 'first' }
         HAML