bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

X-Debbugs-CC: Stefan Monnier <monnier@iro.umontreal.ca>
Tags: patch

In an nxml-mode buffer, put the following text:

<x a="foo" b='bar'/>

Only "foo" is fontified in as a string, while bar is not.  This is a
regression since Emacs 25.3.  The patch below seems solves problem,
though I'm not entirely sure about it: there are some confusing comments
in sgml-mode.el about drawbacks and tradeoffs of recognizing single
quotes that I'm not really following (are they applicable to SGML only,
and not XML?).

There's also the difference that attribute values are now fontified with
font-lock-string-face rather than nxml-attribute-value, though I'm not
sure that's worth fixing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Possible patch --]
[-- Type: text/x-diff, Size: 3822 bytes --]

From a3d2dbcd0c4e272802a45a7c751877b6982fb257 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Apr 2019 22:44:50 -0400
Subject: [PATCH] Make nxml-mode recognize single quote attribute again

* lisp/nxml/nxml-mode.el (nxml-syntax-propertize): New function.
(nxml-tag-syntax-table): New constant.
(nxml-mode): Set them as syntax-propertize-function and
syntax-ppss-table, respectively.
* test/lisp/nxml/nxml-mode-tests.el (nxml-mode-font-lock-quotes): New
test.
---
 lisp/nxml/nxml-mode.el            | 22 ++++++++++++++++++++--
 test/lisp/nxml/nxml-mode-tests.el | 20 ++++++++++++++++++++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/lisp/nxml/nxml-mode.el b/lisp/nxml/nxml-mode.el
index ab035b927e..94fbe4b781 100644
--- a/lisp/nxml/nxml-mode.el
+++ b/lisp/nxml/nxml-mode.el
@@ -426,6 +426,24 @@ (defun nxml-parent-document-set (parent-document)
 (defvar tildify-space-string)
 (defvar tildify-foreach-region-function)
 
+(defun nxml-syntax-propertize (start end)
+  "Syntactic keywords for `nxml-mode'."
+  (goto-char start)
+  (sgml-syntax-propertize-inside end)
+  (funcall
+   (syntax-propertize-rules
+    sgml-syntax-propertize-rules
+    ;; Like the " rule in `sgml-syntax-propertize-rules', but for '.
+    ("'" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
+                  (goto-char (match-end 0)))
+                (string-to-syntax ".")))))
+   start end))
+
+(defconst nxml-tag-syntax-table
+  (let ((table (make-syntax-table sgml-tag-syntax-table)))
+    (modify-syntax-entry ?\' "\"'" table))
+  "Syntax table used to parse XML tags.")
+
 ;;;###autoload
 (define-derived-mode nxml-mode text-mode "nXML"
   ;; We use C-c C-i instead of \\[nxml-balanced-close-start-tag-inline]
@@ -517,8 +535,8 @@ (define-derived-mode nxml-mode text-mode "nXML"
       (with-silent-modifications
 	(nxml-with-invisible-motion
 	  (nxml-scan-prolog)))))
-  (setq-local syntax-ppss-table sgml-tag-syntax-table)
-  (setq-local syntax-propertize-function #'sgml-syntax-propertize)
+  (setq-local syntax-ppss-table nxml-tag-syntax-table)
+  (setq-local syntax-propertize-function #'nxml-syntax-propertize)
   (add-hook 'change-major-mode-hook #'nxml-cleanup nil t)
 
   ;; Emacs 23 handles the encoding attribute on the xml declaration
diff --git a/test/lisp/nxml/nxml-mode-tests.el b/test/lisp/nxml/nxml-mode-tests.el
index 57a731ad18..92744be619 100644
--- a/test/lisp/nxml/nxml-mode-tests.el
+++ b/test/lisp/nxml/nxml-mode-tests.el
@@ -58,5 +58,25 @@ (ert-deftest nxml-balanced-close-start-tag-inline ()
     (nxml-balanced-close-start-tag-inline)
     (should (equal (buffer-string) "<a><b c=\"\"></b></a>"))))
 
+(ert-deftest nxml-mode-font-lock-quotes ()
+  (with-temp-buffer
+    (nxml-mode)
+    (insert "<x a=\"dquote attr\" b='squote attr'>\"dquote text\"'squote text'</x>")
+    (font-lock-ensure)
+    (let ((squote-txt-pos (search-backward "squote text"))
+          (dquote-txt-pos (search-backward "dquote text"))
+          (squote-att-pos (search-backward "squote attr"))
+          (dquote-att-pos (search-backward "dquote attr")))
+      ;; Just make sure that each quote uses the same face for quoted
+      ;; attribute values, and a different face for quoted text
+      ;; outside tags.  Don't test `font-lock-string-face' vs
+      ;; `nxml-attribute-value' here.
+      (should (equal (get-text-property squote-att-pos 'face)
+                     (get-text-property dquote-att-pos 'face)))
+      (should (equal (get-text-property squote-txt-pos 'face)
+                     (get-text-property dquote-txt-pos 'face)))
+      (should-not (equal (get-text-property squote-txt-pos 'face)
+                         (get-text-property dquote-att-pos 'face))))))
+
 (provide 'nxml-mode-tests)
 ;;; nxml-mode-tests.el ends here
-- 
2.11.0

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Stefan Monnier]

> In an nxml-mode buffer, put the following text:
>
> <x a="foo" b='bar'/>
>
> Only "foo" is fontified in as a string, while bar is not.

IIRC this is technically correct because IIRC in XML (contrary to SGML)
only "..."  is allowed and not '...'.  Don't take my word for it, tho,
it's just a vague recollection.

This said, it's probably a good idea to understand '...' in nxml-mode,
since there are various circumstances where you may want to use
nxml-mode to edit files in a format that's more permissive than strict XML.

> This is a regression since Emacs 25.3.

Even more reason to allow '...'.
Have you investigated the source of the regression?

> The patch below seems solves problem, though I'm not entirely sure
> about it: there are some confusing comments in sgml-mode.el about
> drawbacks and tradeoffs of recognizing single quotes that I'm not
> really following (are they applicable to SGML only, and not XML?).

IIRC the comments aren't related to the issue of accepting '...' itself,
but to some of the side-effects of doing it naively in cases such as

    <em>That's right!</em>

but we're no so naive any more, so I believe that we can support this
without the downsides.

> There's also the difference that attribute values are now fontified with
> font-lock-string-face rather than nxml-attribute-value, though I'm not
> sure that's worth fixing.

I don't see where else font-lock-string-face would be used in nxml-mode,
so I see no need to use a special face like nxml-attribute-value for
attributed values.

> +(defun nxml-syntax-propertize (start end)
> +  "Syntactic keywords for `nxml-mode'."
> +  (goto-char start)
> +  (sgml-syntax-propertize-inside end)
> +  (funcall
> +   (syntax-propertize-rules
> +    sgml-syntax-propertize-rules
> +    ;; Like the " rule in `sgml-syntax-propertize-rules', but for '.
> +    ("'" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
> +                  (goto-char (match-end 0)))
> +                (string-to-syntax ".")))))
> +   start end))
> +
> +(defconst nxml-tag-syntax-table
> +  (let ((table (make-syntax-table sgml-tag-syntax-table)))
> +    (modify-syntax-entry ?\' "\"'" table))
> +  "Syntax table used to parse XML tags.")
> +
>  ;;;###autoload
>  (define-derived-mode nxml-mode text-mode "nXML"
>    ;; We use C-c C-i instead of \\[nxml-balanced-close-start-tag-inline]
> @@ -517,8 +535,8 @@ (define-derived-mode nxml-mode text-mode "nXML"
>        (with-silent-modifications
>  	(nxml-with-invisible-motion
>  	  (nxml-scan-prolog)))))
> -  (setq-local syntax-ppss-table sgml-tag-syntax-table)
> -  (setq-local syntax-propertize-function #'sgml-syntax-propertize)
> +  (setq-local syntax-ppss-table nxml-tag-syntax-table)
> +  (setq-local syntax-propertize-function #'nxml-syntax-propertize)

Hmm... I think it would be better to change `sgml-syntax-propertize` so
it does what we need.  After all, it's more important to support '...'
for SGML  than for XML.


        Stefan

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In an nxml-mode buffer, put the following text:
>>
>> <x a="foo" b='bar'/>
>>
>> Only "foo" is fontified in as a string, while bar is not.
>
> IIRC this is technically correct because IIRC in XML (contrary to SGML)
> only "..."  is allowed and not '...'.  Don't take my word for it, tho,
> it's just a vague recollection.

Well, I was pretty sure that XML allows both quotes, but just to make
things definitive, https://www.w3.org/TR/xml/#NT-AttValue:

    [10]    AttValue       ::= '"' ([^<&"] | Reference)* '"'
                            |  "'" ([^<&'] | Reference)* "'"

I found this for SGML http://xml.coverpages.org/sgmlsyn/sgmlsyn.htm#P34:

    [34] attribute value literal =

        ( lit , "
        replaceable character data [46] *,
        lit ) | "
        ( lita , '
        replaceable character data [46] *,
        lita ) '

> This said, it's probably a good idea to understand '...' in nxml-mode,
> since there are various circumstances where you may want to use
> nxml-mode to edit files in a format that's more permissive than strict XML.
>
>> This is a regression since Emacs 25.3.
>
> Even more reason to allow '...'.
> Have you investigated the source of the regression?

I didn't actually checkout and compile before+after, but I'm pretty sure
it's [56e1097584], same as Bug#32003.

[56e1097584]: 2016-01-16 15:03:42 -0500
  lisp/nxml: Use syntax-tables for comments
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=56e1097584c13f2b6db85592769db1c6c36e9419

>> The patch below seems solves problem, though I'm not entirely sure
>> about it: there are some confusing comments in sgml-mode.el about
>> drawbacks and tradeoffs of recognizing single quotes that I'm not
>> really following (are they applicable to SGML only, and not XML?).
>
> IIRC the comments aren't related to the issue of accepting '...' itself,
> but to some of the side-effects of doing it naively in cases such as
>
>     <em>That's right!</em>
>
> but we're no so naive any more, so I believe that we can support this
> without the downsides.

>> +  (setq-local syntax-ppss-table nxml-tag-syntax-table)
>> +  (setq-local syntax-propertize-function #'nxml-syntax-propertize)
>
> Hmm... I think it would be better to change `sgml-syntax-propertize` so
> it does what we need.  After all, it's more important to support '...'
> for SGML  than for XML.

s/more/equally/, but otherwise yes.  Patching sgml-mode is shorter and
even fixes Bug#8203 as well.  Still good for emacs-26 I hope, since this
is for an (nxml-mode) regression in 26.1.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 4405 bytes --]

From 4288bf689b9dcdf32c7074d54e76c34ff115dea9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Apr 2019 22:44:50 -0400
Subject: [PATCH] Recognize single quote attribute values in nxml and sgml
 (Bug#35381)

* lisp/textmodes/sgml-mode.el (sgml-specials): Add single quote.
(sgml-syntax-propertize-rules): Handle single quote.
* test/lisp/nxml/nxml-mode-tests.el (nxml-mode-font-lock-quotes): New
test.
* test/lisp/textmodes/sgml-mode-tests.el
(sgml-delete-tag-bug-8203-should-not-delete-apostrophe): Now passes.
---
 lisp/textmodes/sgml-mode.el            | 10 +++++-----
 test/lisp/nxml/nxml-mode-tests.el      | 20 ++++++++++++++++++++
 test/lisp/textmodes/sgml-mode-tests.el |  1 -
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 50b2077ef4..0eaad1a1ed 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -103,7 +103,7 @@ (defcustom sgml-mode-hook nil
 ;; As long as Emacs's syntax can't be complemented with predicates to context
 ;; sensitively confirm the syntax of characters, we have to live with this
 ;; kludgy kind of tradeoff.
-(defvar sgml-specials '(?\")
+(defvar sgml-specials '(?\" ?\')
   "List of characters that have a special meaning for SGML mode.
 This list is used when first loading the `sgml-mode' library.
 The supported characters and potential disadvantages are:
@@ -351,12 +351,12 @@ (eval-and-compile
      ("--[ \t\n]*\\(>\\)" (1 "> b"))
      ("\\(<\\)[?!]" (1 (prog1 "|>"
                          (sgml-syntax-propertize-inside end))))
-     ;; Double quotes outside of tags should not introduce strings.
+     ;; Quotes outside of tags should not introduce strings.
      ;; Be careful to call `syntax-ppss' on a position before the one we're
      ;; going to change, so as not to need to flush the data we just computed.
-     ("\"" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
-                    (goto-char (match-end 0)))
-                  (string-to-syntax ".")))))))
+     ("[\"']" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
+                       (goto-char (match-end 0)))
+                     (string-to-syntax ".")))))))
 
 (defun sgml-syntax-propertize (start end)
   "Syntactic keywords for `sgml-mode'."
diff --git a/test/lisp/nxml/nxml-mode-tests.el b/test/lisp/nxml/nxml-mode-tests.el
index 57a731ad18..92744be619 100644
--- a/test/lisp/nxml/nxml-mode-tests.el
+++ b/test/lisp/nxml/nxml-mode-tests.el
@@ -58,5 +58,25 @@ (ert-deftest nxml-balanced-close-start-tag-inline ()
     (nxml-balanced-close-start-tag-inline)
     (should (equal (buffer-string) "<a><b c=\"\"></b></a>"))))
 
+(ert-deftest nxml-mode-font-lock-quotes ()
+  (with-temp-buffer
+    (nxml-mode)
+    (insert "<x a=\"dquote attr\" b='squote attr'>\"dquote text\"'squote text'</x>")
+    (font-lock-ensure)
+    (let ((squote-txt-pos (search-backward "squote text"))
+          (dquote-txt-pos (search-backward "dquote text"))
+          (squote-att-pos (search-backward "squote attr"))
+          (dquote-att-pos (search-backward "dquote attr")))
+      ;; Just make sure that each quote uses the same face for quoted
+      ;; attribute values, and a different face for quoted text
+      ;; outside tags.  Don't test `font-lock-string-face' vs
+      ;; `nxml-attribute-value' here.
+      (should (equal (get-text-property squote-att-pos 'face)
+                     (get-text-property dquote-att-pos 'face)))
+      (should (equal (get-text-property squote-txt-pos 'face)
+                     (get-text-property dquote-txt-pos 'face)))
+      (should-not (equal (get-text-property squote-txt-pos 'face)
+                         (get-text-property dquote-att-pos 'face))))))
+
 (provide 'nxml-mode-tests)
 ;;; nxml-mode-tests.el ends here
diff --git a/test/lisp/textmodes/sgml-mode-tests.el b/test/lisp/textmodes/sgml-mode-tests.el
index 20b5e27ff5..7318a667b3 100644
--- a/test/lisp/textmodes/sgml-mode-tests.el
+++ b/test/lisp/textmodes/sgml-mode-tests.el
@@ -125,7 +125,6 @@ (ert-deftest sgml-delete-tag-should-signal-error-when-deleting-too-much ()
      (should (string= content (buffer-string))))))
 
 (ert-deftest sgml-delete-tag-bug-8203-should-not-delete-apostrophe ()
-  :expected-result :failed
   (sgml-with-content
    "<title>Winter is comin'</title>"
    (sgml-delete-tag 1)
-- 
2.11.0

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Stefan Monnier]

> Well, I was pretty sure that XML allows both quotes, but just to make
> things definitive, https://www.w3.org/TR/xml/#NT-AttValue:
>
>     [10]    AttValue       ::= '"' ([^<&"] | Reference)* '"'
>                             |  "'" ([^<&'] | Reference)* "'"

Thanks for checking.

> I didn't actually checkout and compile before+after, but I'm pretty sure
> it's [56e1097584], same as Bug#32003.
>
> [56e1097584]: 2016-01-16 15:03:42 -0500
>   lisp/nxml: Use syntax-tables for comments
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=56e1097584c13f2b6db85592769db1c6c36e9419

Ah, yes, that makes sense.

> s/more/equally/, but otherwise yes.  Patching sgml-mode is shorter and
> even fixes Bug#8203 as well.  Still good for emacs-26 I hope, since this
> is for an (nxml-mode) regression in 26.1.

Great, thanks.

> --- a/lisp/textmodes/sgml-mode.el
> +++ b/lisp/textmodes/sgml-mode.el
> @@ -103,7 +103,7 @@ (defcustom sgml-mode-hook nil
>  ;; As long as Emacs's syntax can't be complemented with predicates to context
>  ;; sensitively confirm the syntax of characters, we have to live with this
>  ;; kludgy kind of tradeoff.
> -(defvar sgml-specials '(?\")
> +(defvar sgml-specials '(?\" ?\')
>    "List of characters that have a special meaning for SGML mode.
>  This list is used when first loading the `sgml-mode' library.
>  The supported characters and potential disadvantages are:

I think this "disadvantages" part of the docstring is out of date.
Can you update it while you're at it?

Also, we probably want to mark this var as obsolete.


        Stefan

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> +(defvar sgml-specials '(?\" ?\')
>>    "List of characters that have a special meaning for SGML mode.
>>  This list is used when first loading the `sgml-mode' library.
>>  The supported characters and potential disadvantages are:
>
> I think this "disadvantages" part of the docstring is out of date.
> Can you update it while you're at it?
>
> Also, we probably want to mark this var as obsolete.

I'm not sure about this, is the disadvantage of the ?- also out of date?
There's some FIXME on the "--" rule of sgml-syntax-propertize-rules,
 
-;; As long as Emacs's syntax can't be complemented with predicates to context
-;; sensitively confirm the syntax of characters, we have to live with this
-;; kludgy kind of tradeoff.
 (defvar sgml-specials '(?\" ?\')
   "List of characters that have a special meaning for SGML mode.
 This list is used when first loading the `sgml-mode' library.
-The supported characters and potential disadvantages are:
-
-  ?\\\"	Makes \" in text start a string.
-  ?\\='	Makes \\=' in text start a string.
-  ?-	Makes -- in text start a comment.
-
-When only one of ?\\\" or ?\\=' are included, \"\\='\" or \\='\"\\=', as can be found in
-DTDs, start a string.  To partially avoid this problem this also makes these
-self insert as named entities depending on `sgml-quick-keys'.
+The supported characters are ?\\\", ?\\=', and ?-.
 
 Including ?- has the problem of affecting dashes that have nothing to do
 with comments, so we normally turn it off.")

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Stefan Monnier]

> I'm not sure about this, is the disadvantage of the ?- also out
> of date?

If not, we should just fix it with appropriate syntax-propertize tricks.

>  Including ?- has the problem of affecting dashes that have nothing to do
>  with comments, so we normally turn it off.")

Actually, I think the issue with -- comments is not just that they
should not be treated as such when appearing outside tags, but that even
within tags it's often not to be considered as comments.

E.g. according to my flaky memory --...-- is not considered as a comment
in HTML nor in XML (both inside and outside of tags).


        Stefan

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> I'm not sure about this, is the disadvantage of the ?- also out
>> of date?
>
> If not, we should just fix it with appropriate syntax-propertize tricks.
>
>>  Including ?- has the problem of affecting dashes that have nothing to do
>>  with comments, so we normally turn it off.")
>
> Actually, I think the issue with -- comments is not just that they
> should not be treated as such when appearing outside tags, but that even
> within tags it's often not to be considered as comments.
>
> E.g. according to my flaky memory --...-- is not considered as a comment
> in HTML nor in XML (both inside and outside of tags).

XML says comments should always be <!--...--> where ... contains no
"--".

https://www.w3.org/TR/xml/#sec-comments

For SGML, it seems that --...-- is a comment within DTD definitions.
Otherwise only <!-- starts a comment, but "--" can toggle interpretation
of angle brackets and other special characters.  But browsers may or may
not handle the toggling thing correctly, so the recommendation for HTML
is to stick <!--...--> just like XML.  It's all rather confusing.

http://www.flightlab.com/~joe/sgml/comments.html
https://www.w3.org/TR/WD-html40-970917/intro/sgmltut.html

-;; As long as Emacs's syntax can't be complemented with predicates to context
-;; sensitively confirm the syntax of characters, we have to live with this
-;; kludgy kind of tradeoff.
+;; TODO: Add syntax-propertize rules to handle "--" properly, and
+;; eventually get rid of this variable altogether.
 (defvar sgml-specials '(?\" ?\')
   "List of characters that have a special meaning for SGML mode.
 This list is used when first loading the `sgml-mode' library.
-The supported characters and potential disadvantages are:
+The supported characters are ?\\\", ?\\=', and ?-.
 
-  ?\\\"	Makes \" in text start a string.
-  ?\\='	Makes \\=' in text start a string.
-  ?-	Makes -- in text start a comment.
-
-When only one of ?\\\" or ?\\=' are included, \"\\='\" or \\='\"\\=', as can be found in
-DTDs, start a string.  To partially avoid this problem this also makes these
-self insert as named entities depending on `sgml-quick-keys'.
-
-Including ?- has the problem of affecting dashes that have nothing to do
-with comments, so we normally turn it off.")
+Including ?- makes double dashes into comment delimiters, but
+they are really only supposed to delimit comments within DTD
+definitions.  So we normally turn it off.")

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Stefan Monnier]

> XML says comments should always be <!--...--> where ... contains no
> "--".

That's also what I remember.

> For SGML, it seems that --...-- is a comment within DTD definitions.

But only within the DTD definition, so in practice I think it means it's
not a comment in SGML files.

> Otherwise only <!-- starts a comment, but "--" can toggle interpretation
> of angle brackets and other special characters.  But browsers may or may
> not handle the toggling thing correctly, so the recommendation for HTML
> is to stick <!--...--> just like XML.  It's all rather confusing.

Confusing is a pretty good description.  From where I stand, I think we
simply shouldn't bother to try and handle "--" specially.  At least not
before someone comes with a concrete use-case.


        Stefan

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> From where I stand, I think we simply shouldn't bother to try and
> handle "--" specially.  At least not before someone comes with a
> concrete use-case.

Agreed (I updated the commentary a bit to say that).  


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5665 bytes --]

From 6985354ef612cf0e2db1987e51e5a9f06b396ad9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 21 Apr 2019 22:44:50 -0400
Subject: [PATCH] Recognize single quote attribute values in nxml and sgml
 (Bug#35381)

* lisp/textmodes/sgml-mode.el (sgml-specials): Add single quote.
(sgml-syntax-propertize-rules): Handle single quote.
* test/lisp/nxml/nxml-mode-tests.el (nxml-mode-font-lock-quotes): New
test.
* test/lisp/textmodes/sgml-mode-tests.el
(sgml-delete-tag-bug-8203-should-not-delete-apostrophe): Now passes.
---
 lisp/textmodes/sgml-mode.el            | 34 +++++++++++++++-------------------
 test/lisp/nxml/nxml-mode-tests.el      | 20 ++++++++++++++++++++
 test/lisp/textmodes/sgml-mode-tests.el |  1 -
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 50b2077ef4..128e58810e 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -100,24 +100,20 @@ (defcustom sgml-mode-hook nil
   :group 'sgml
   :type 'hook)
 
-;; As long as Emacs's syntax can't be complemented with predicates to context
-;; sensitively confirm the syntax of characters, we have to live with this
-;; kludgy kind of tradeoff.
-(defvar sgml-specials '(?\")
+;; The official handling of "--" is complicated in SGML, and
+;; historically not well supported by browser HTML parsers.
+;; Recommendations for writing HTML comments is to use <!--...-->
+;; (where ... doesn't contain "--") to avoid the complications
+;; altogether (XML goes even further by requiring this in the spec).
+;; So there is probably no need to handle it "correctly".
+(defvar sgml-specials '(?\" ?\')
   "List of characters that have a special meaning for SGML mode.
 This list is used when first loading the `sgml-mode' library.
-The supported characters and potential disadvantages are:
+The supported characters are ?\\\", ?\\=', and ?-.
 
-  ?\\\"	Makes \" in text start a string.
-  ?\\='	Makes \\=' in text start a string.
-  ?-	Makes -- in text start a comment.
-
-When only one of ?\\\" or ?\\=' are included, \"\\='\" or \\='\"\\=', as can be found in
-DTDs, start a string.  To partially avoid this problem this also makes these
-self insert as named entities depending on `sgml-quick-keys'.
-
-Including ?- has the problem of affecting dashes that have nothing to do
-with comments, so we normally turn it off.")
+Including ?- makes double dashes into comment delimiters, but
+they are really only supposed to delimit comments within DTD
+definitions.  So we normally turn it off.")
 
 (defvar sgml-quick-keys nil
   "Use <, >, &, /, SPC and `sgml-specials' keys \"electrically\" when non-nil.
@@ -351,12 +347,12 @@ (eval-and-compile
      ("--[ \t\n]*\\(>\\)" (1 "> b"))
      ("\\(<\\)[?!]" (1 (prog1 "|>"
                          (sgml-syntax-propertize-inside end))))
-     ;; Double quotes outside of tags should not introduce strings.
+     ;; Quotes outside of tags should not introduce strings.
      ;; Be careful to call `syntax-ppss' on a position before the one we're
      ;; going to change, so as not to need to flush the data we just computed.
-     ("\"" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
-                    (goto-char (match-end 0)))
-                  (string-to-syntax ".")))))))
+     ("[\"']" (0 (if (prog1 (zerop (car (syntax-ppss (match-beginning 0))))
+                       (goto-char (match-end 0)))
+                     (string-to-syntax ".")))))))
 
 (defun sgml-syntax-propertize (start end)
   "Syntactic keywords for `sgml-mode'."
diff --git a/test/lisp/nxml/nxml-mode-tests.el b/test/lisp/nxml/nxml-mode-tests.el
index 57a731ad18..92744be619 100644
--- a/test/lisp/nxml/nxml-mode-tests.el
+++ b/test/lisp/nxml/nxml-mode-tests.el
@@ -58,5 +58,25 @@ (ert-deftest nxml-balanced-close-start-tag-inline ()
     (nxml-balanced-close-start-tag-inline)
     (should (equal (buffer-string) "<a><b c=\"\"></b></a>"))))
 
+(ert-deftest nxml-mode-font-lock-quotes ()
+  (with-temp-buffer
+    (nxml-mode)
+    (insert "<x a=\"dquote attr\" b='squote attr'>\"dquote text\"'squote text'</x>")
+    (font-lock-ensure)
+    (let ((squote-txt-pos (search-backward "squote text"))
+          (dquote-txt-pos (search-backward "dquote text"))
+          (squote-att-pos (search-backward "squote attr"))
+          (dquote-att-pos (search-backward "dquote attr")))
+      ;; Just make sure that each quote uses the same face for quoted
+      ;; attribute values, and a different face for quoted text
+      ;; outside tags.  Don't test `font-lock-string-face' vs
+      ;; `nxml-attribute-value' here.
+      (should (equal (get-text-property squote-att-pos 'face)
+                     (get-text-property dquote-att-pos 'face)))
+      (should (equal (get-text-property squote-txt-pos 'face)
+                     (get-text-property dquote-txt-pos 'face)))
+      (should-not (equal (get-text-property squote-txt-pos 'face)
+                         (get-text-property dquote-att-pos 'face))))))
+
 (provide 'nxml-mode-tests)
 ;;; nxml-mode-tests.el ends here
diff --git a/test/lisp/textmodes/sgml-mode-tests.el b/test/lisp/textmodes/sgml-mode-tests.el
index 20b5e27ff5..7318a667b3 100644
--- a/test/lisp/textmodes/sgml-mode-tests.el
+++ b/test/lisp/textmodes/sgml-mode-tests.el
@@ -125,7 +125,6 @@ (ert-deftest sgml-delete-tag-should-signal-error-when-deleting-too-much ()
      (should (string= content (buffer-string))))))
 
 (ert-deftest sgml-delete-tag-bug-8203-should-not-delete-apostrophe ()
-  :expected-result :failed
   (sgml-with-content
    "<title>Winter is comin'</title>"
    (sgml-delete-tag 1)
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 405 bytes --]


I think that plus the patches for #32003 and #32897 should cover the
nxml fixes I'm planning for emacs-26.  I'll wait for another few days
and then push.

https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Fix-nxml-get-inside-Bug-32003.patch;bug=32003;msg=8;att=1
https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Disable-extra-display-of-10-in-nxml-mode-Bug-32897.patch;att=1;msg=8;bug=32897

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Stefan Monnier]

> I think that plus the patches for #32003 and #32897 should cover the
> nxml fixes I'm planning for emacs-26.  I'll wait for another few days
> and then push.

LGTM.

> https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Fix-nxml-get-inside-Bug-32003.patch;bug=32003;msg=8;att=1

You might like to mention in nxml-get-inside what "normal strings" are
used for (AFAIK that's used exclusively for attributes).

> https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Disable-extra-display-of-10-in-nxml-mode-Bug-32897.patch;att=1;msg=8;bug=32897

-  (when nxml-char-ref-extra-display
+  (when (and ;; Displaying literal newline is unhelpful.
+             (not eql n ?\n)
+             nxml-char-ref-extra-display)

There's a bunch of parens missing.


        Stefan

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Disable-extra-display-of-10-in-nxml-mode-Bug-32897.patch;att=1;msg=8;bug=32897
>
> -  (when nxml-char-ref-extra-display
> +  (when (and ;; Displaying literal newline is unhelpful.
> +             (not eql n ?\n)
> +             nxml-char-ref-extra-display)
>
> There's a bunch of parens missing.

How do you mean?

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[martin rudalics]

 > How do you mean?

I suppose Stefan means to write

(not (eql n ?\n))

instead of

(not eql n ?\n)

martin

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

martin rudalics <rudalics@gmx.at> writes:

>> How do you mean?
>
> I suppose Stefan means to write
>
> (not (eql n ?\n))
>
> instead of
>
> (not eql n ?\n)

Oh right, I had actually fixed that locally already, but forgot to
update the bug thread.

bug#35381: 26.2; nxml-mode doesn't fontify attributes in single quotes

[Noam Postavsky]

tags 35381 fixed
close 35381 26.3
quit

> I think that plus the patches for #32003 and #32897 should cover the
> nxml fixes I'm planning for emacs-26.  I'll wait for another few days
> and then push.

Done.

7dab3ee7ab 2019-05-09T06:42:40-04:00 "Recognize single quote attribute values in nxml and sgml (Bug#35381)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7dab3ee7ab54b3c2e7bc24170376054786c01d6f