Regression: Wrapping of last word in span may break text display, make it optional

Patch seems to be full of out of scope changes and should be in an MR too

Also will need screenshots

I encountered this same issue in my project after I updated extlink module from 1.x to 2.x.
I'll try to add a screenshot where the issue can be seen. Basically a span with a class "extlink-nobreak" wraps the last word of the link and white-space disappears merging the words in the link together.

It seems some side effects were already anticipated in the issue https://www.drupal.org/project/extlink/issues/3172378#comment-15757800

While the provided patch here "solves" the issue by removing the wrapping span, I guess a more thorough solution might still be better? Like adding another checkbox to the module where it could be selected if that span should be added there or not (default: false)?

Thanks all. I'm also seeing this, here's an example where you can see the impact of an update.

Previously the link was in line and vertically centered.

I've taken a stab at that. I opted to default it to off (I added a warning that it might affect styling, and it felt weird to then default it to enabled) but I appreciate folk might prefer the other way around (:

Thanks!

I tested the new setting in my project and it seems to work - so thanks a lot!

When it's off by default at least it shouldn't break anyone's theme when they update from older version (1.x) to new version (2.x).
However, I'm not sure if there would be any better wording for the setting title, but as a non-native English speaker I wasn't able to come up with better suggestions :)
The way it is now is probably good enough, I think!
-> "Try to prevent text wrapping separating the last word from the icon."

Edit: I forgot to mention that the notice given by extlink module concerning this change after running database updates was good.

Thanks for testing it @hartsak.

I'm not sure if there would be any better wording for the setting title, but as a non-native English speaker I wasn't able to come up with better suggestions :)

I agree it's clunky, I'm a native speaker but words were failing me (:

Open to suggestions!

So I believe the default should actually be TRUE. This change will break people's existing sites who don't have a problem with the change.

This change will break people's existing sites who don't have a problem with the change.

I think that's perhaps a strong use of break: it will switch off a feature to prevent orphans IIUC, which is arguably less of a break than adding extra markup without warning and the associated visual regressions. Certainly now that it's already been introduced in a released version, there's no way I can see to avoid potentially upsetting some group: either it's going to be switched off for you and some will want it on, or it's going to be switched on and some will want it off. (In both cases we can add a message to the update to try and help.)

We can also default to having it enabled on a fresh install and disabled on update.

If we go the route of defaulting to enabled (either for a fresh install or on update), do we also want to adjust the warning on the option? It seems odd to me have a warning with an option and to default to having it enabled?

Thanks

Most updates don't actually have a message so I think we can drop that.

I've checked the changes myself and they're looking good, though I agree with andyf on:

I think that's perhaps a strong use of break: it will switch off a feature to prevent orphans IIUC, which is arguably less of a break than adding extra markup without warning and the associated visual regressions.

I've already had this become an issue on 3 sites and it would be ideal to have it default as FALSE but I would be happy anyway if we had the setting to switch on/off and it defaulted to TRUE

I'm in the same boat with dabodia and andyf about the default setting value, but of course I can also understand other opinions about it.

Anyway, if my options are to use a patch or go and change some setting value, I'd much rather change that setting.
I have now 2 projects with this issue and I'm using a patch, so I'd be happy to get rid of that patch :)

Thanks all!

Most updates don't actually have a message so I think we can drop that.

I appreciate most updates don't have a message, but as explained in this case you're going to be upsetting some group of users necessarily (or have I missed something there?). If someone's going to be upset, and we can give them a message to explain, I'm not sure why we wouldn't? Does removing it bring any advantages?

There's still the outstanding question about if we want to have a config option with a warning and default it to enabled? And would we want it enabled by default for both new and upgrading users?

So the current tagged release has this feature on. This ticket wants the option to turn off, which I'm completely fine with. But since the current behavior is on then this change should not change that. So essentially if I download this fix and run the update nothing should change for me but for those who want to turn off they can now do that.

Will hold next tag release for this fix btw

Thanks @smustgrave. I don't want it to sound like I'm arguing for one side over the other, it's just so far it all your responses have ignored a group of users it seems to me.

the current behavior is on

I don't think that's an accurate statement. For users on 1.x it's not on. For users on 2.x it's on. The ideal solution would've been imho to default it to off on upgrade when it was originally introduced, but enable it by default (if that's what you want) for new installs. That way nobody gets a change in behavior. That cat's out of the bag now, so somebody's going to be disappointed it seems to me. There's no way we ensure that we don't change current behavior for anybody.

Again, I'm not arguing for one side over the other, just making it clear there's another side that isn't represented in your responses so far from my PoV. I can personally see arguments both ways:

• It's better not to change people's markup without them opting in, so turn it off. It's safer to let those who are happy with the markup being altered to explicitly opt in.
• It's ok to make changes like this in a major version upgrade - don't inconvenience users of 2.x that already have it switched on

If we go the second route, I think there should be a big warning on the release notes (and perhaps the project page?) about the automatic change in behavior. Currently it's a very inocuous item in the notes (:

There's also still the outstanding question about if we want to have a config option with a warning and default it to enabled? (ie if we default to it enabled should we remove the warning? As I say, seems odd to me to have a warning with an option that's enabled by default.)

Thanks again

Only plan on merging this into 2.0.x

I get that, but for users upgrading from 1.x to 2.x, this is a change of behavior. Sorry I feel like I'm making heavy work of explaining this.

There's been 4 releases in 2.0.x is someone chose to wait that long think that's on them

is someone chose to wait that long think that's on them

I mean it's a little over 2 months it's been released, so it feels to me a little harsh to characterize people who haven't had the budget/opportunity to update in such a way, but you're the maintainer.

There's still the other question about the warning, and personally I'd argue it'd be sensible to update the release notes for 2.0.0 to mention the breaking change.

Sure updated 2.0.0 release notes

@smustgrave: FWIW, despite marking 1.x as unsupported, there are still 33k installations on 1.x vs 23k on 2.x. In my experience, it takes a long time until even just a majority of users switch over to new versions and there will be a long tail of those that don't.

As a maintainer, I prefer to keep the supported flag on older release branches on for quite some time, because as soon as disable it, all sites that are still on 1.x get a big red update alert about using unsupported versions. The only cost for you is the risk that you might need to backport a security issue in the project also to 1.x.

See I have had the opposite experience. Saying you support older branches means more maintenance work. Issue could be fixed in 1 and broken in another and people expect/sometimes demand fixing in the older.

We actually experienced this issue also when upgrading the project from 1.x to 2.x. We were on a tight schedule and were not immediately able to fix the issue and actually did not notice this BC issue so we ended up downgrading the module back to 1.x. Usually we do contrib module updates when doing a large core update and this might mean a lot of contrib module updates at the same time so the time frame for testing each and every contrib module is pretty small because budget and time reasons.

These kinds of things are hard because although the breaking change is quite minimal (some styling broken) it's still a bit hard when it's visible in almost all the pages of the site.

I think it would make sense to keep the update path from 1.x to 2.x so that the old behavior does not change. I would vote for keeping the default value for the new option as false because as mentioned by @andyf this seems to add extra markup to existing sites doing the update.

I don't mind committing this but I feel the default value should be true. With a message if updating from 1.x that you may want to turn this off.

So if we change to true are we fine to commit?

At least I am fine with this change since it's now well documented and clearly visible that this setting can be changed if there is an issue with the styling after upgrading. 1.x to 2.x is a major update so there can be breaking changes in my opinion as long as they are well documented which seems to be now done well.

Tested the patch and both enabled and disabled options.

+1 for RTBC with true as default but I'll let the people with the initial discussion make the final status change.

See also #3487095: New option "Wrap only text-like links." as a related/follow-up issue, which solves issues that we have where external links are on cards and similar block elements and not just in text.

So if we change to true are we fine to commit?

I had an open question about the warning:

There's also still the outstanding question about if we want to have a config option with a warning and default it to enabled? (ie if we default to it enabled should we remove the warning? As I say, seems odd to me to have a warning with an option that's enabled by default.)

If we remove the warning I’m fine with that. But pretty stuck in the default being true

My 2 cents: I think a regression was introduced by that wrapping span, causing trouble for existing pages and propably for future pages to solve a definitely existing problem, which HTML /CSS has no perfect solution for. The regression is worse than the break we had before and breaks existing sites.

So from my perspective the extlink_prevent_orphan should be FALSE and we should show the warning on update, so the admin is informed and can change the value back to enabled if needed.

Let's get this done. :)

Won't die on this hill.

Will work on committing this evening or tomorrow most likely