Problem/Motivation

The house style for our organization is to not include a trailing period in abbreviations (e.g. "Doing D.I.Y is fun."). However this seems not to be properly captured by the "Wrap Caps" filter, and the last initial is not wrapped.

The same problem behavior was apparently reported and fixed in version 7 here: Wrap Caps is inconsistent (2304643) - the code intended to fix the issue seems to have been carried forward to version 8, without diverging, but doesn't behave as it should (period separated abbreviations without a trailing period are not captured/wrapped), at least not in version 8. (The commentary in #2304643 includes abbreviations without trailing periods ("B.A") but those do not actually appear in the test-case, and weren't the focus of the issue, but rather interractions with punctuation, which was fixed)

Looking at the the regex on line 90 of src/Typogrify.php and testing it locally - I can see the solution is most of the way there but still looks for a period followed by possible white-space (\.\s?) - This period would not exist for the last initial of period-separated abbreviations without trailing periods hence it is not captured.

Here's the line in question:

      (?:[[\p{Lu}]+\.\s?)+)  # Followed by the same thing at least once more

We can make period optional on proceeding initials by appending the ? quantifier after \.

Steps to reproduce

1. Enter the following text into a new article using a text-format that has typogrify enabled with the "Wrap Caps" filter switched on.

"D.I.Y is a great way to save money on home renovation.

2. Save the article

3. Inspect element in your browser to view the rendered markup as follows

<span class="caps">D.I.</span>Y is a great way to save money on home renovation.

4. Note that the "Y" is not captured in the span.

Proposed resolution

Change line 90 of src/Typogrify.php from...

      (?:[[\p{Lu}]+\.\s?)+)  # Followed by the same thing at least once more

..to...

      (?:[[\p{Lu}]+\.?\s?)+)  # Followed by the same thing at least once more

Remaining tasks

Suggest code changes
Test locally
Create MR
Review by maintainers
Revisions/Changes
Merge

User interface changes

Rendered text before (with wrapped caps styled red)
Y is not red, and is enlarged compared to D and I

Rendered text after (with wrapped caps still styled red)
Y is red, and is the same size as D and I

API changes

N/A

Data model changes

N/A

Issue fork typogrify-3540951

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

jacobupal created an issue. See original summary.

jacobupal’s picture

Issue summary: View changes

jacobupal’s picture

Issue summary: View changes
jacobupal’s picture

(The failing tests on the issue branch don't seem to be related to the file I changed)

jacobupal’s picture

Status: Active » Needs review
benjifisher’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

@jacobupal:

Thanks for working on this issue. I will have a look at the failing tests in a separate issue.

I would like to have test coverage for this change, mostly as a form of documentation. If you look at the bottom of tests/src/Kernel/TypogrifySmartyPantsTest.php, it is pretty easy to add a test case to providerTypogrifyExamples(). If the "before" text includes several abbreviations, then a single test case can cover many variations. Something like this:

There are many common three-letter abbreviations (TLAs) such as Q.E.D. or Q.E.D and TLA, or TL.A, itself.

The test should fail with the current version of the code an pass with the updated version.

I am adding the issue tag for test coverage.

jacobupal’s picture

Added a test using your example sentence, however it fails for the "TL.A", part:

or TL.A, itself becomes or <span class="caps">TL</class>.A, itself and the ".A" is not included...

I can't think of any acronyms which would actually have that format though... Usually they're either period separated or not. If there is some application for "TL.A" style acronyms, in some technical context or in another language I can look into updating the regex to accommodate it.

However, if it was just a typo, my appologies, I can adjust the test so we aren't testing for that case.

jacobupal’s picture

Status: Needs work » Needs review
benjifisher’s picture

Status: Needs review » Needs work

@jacobupal:

I updated the MR by merging with the latest 8.x-1.x. That updates the tests so that they work with the current version of PHPUnit.

The test you added is failing. Either the test needs updating or the code that it tests needs to be fixed. Probably the test should be fixed. As I said in Comment #7, "I would like to have test coverage for this change, mostly as a form of documentation." That is, I should be able to look at the test and see an example of "before" and "after" text, exactly as the module would transform it.

I also asked for a new test case in TypogrifySmartyPantsTest.php. All you have to do is add another array (or row) to the data provider: the array elements are then passed as arguments to testTypogrify(). Instead, you added a new test to tests/src/Unit/TypogrifyClassTest.php.

In the issue summary, you wrote,

The same problem was apparently reported and fixed in version 7 here: Wrap Caps is inconsistent (2304643) - the code intended to fix the issue seems to have been carried forward to version 8, but doesn't behave as it should, at least not in version 8.

I do not think that is true. Issue #2304643 was fixed in Commit b9950a5, and the code was further modified for #2404819: small caps skips unicode uppercase characters in Commit 813123d. All of that was done before the 8.x-1.x branch was created. Based on what you wrote, I assumed that the 8.x and 7.x versions had diverged, but in fact they have the same code.

jacobupal’s picture

Issue summary: View changes
jacobupal’s picture

When I said "The same problem was apparently reported and fixed in version 7" I meant that the issue history and status indicated that the desired behavior had been achieved, not that the code itself was any different, and as I said, the code itself had been carried forward. I can't account for why, even though the code was unchanged while the unwanted behavior seems to have persisted or reverted or when, if so. However, I can see why my wording could have been confusing, so I've amended the issue description for clarity.

I've also moved the test from TypogrifyClassTest.php to right place in TypogrifySmartyPantsTest.php and amended the test case to reflect actual behavior.

jacobupal’s picture

Status: Needs work » Needs review
benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

I added a test case based on the issue summary from #2304643: Wrap Caps is inconsistent and made a couple of other small changes to the tests. When I run the test-only changes in GitLab CI,

  1. The test for this issue fails as expected. +1
  2. The test for #2304643 passes, confirming that there has not been a regression on that issue.

I am removing #2304643 as a related issue. I will Fix this one and soon I will release a new version of this module.

benjifisher’s picture

Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

jacobupal’s picture

Thanks for this!

(Just as a reflection: I finally figured out the confusion around how this related to the previous issue: I can see now that the OP used abreviations without trailing periods when they described the behavior ("B.A" and "B.F.A") - perhaps as a typo, perhaps as a stylistic habit and I relied on that description too much - whereas their actual example/test-case had periods at the ends of the abrevations all along ("B.A." and "B.F.A.")... so it was only their interractions with punctuation which was reported, fixed and tested).

jacobupal’s picture

Issue summary: View changes
jacobupal’s picture

Issue summary: View changes

  • benjifisher committed cf264f14 on 8.x-1.x
    feat: #3540951 "Wrap caps" should capture period-separated abbreviations...
benjifisher’s picture

The auto-generated comment is based on the commit message. The commit that fixes the issue is 3695e11.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.