On the module's settings page, the help text for the "Text Resize Scope" setting explains that you may enter a CSS class, id, or HTML tag. The corresponding javascript in text_resize.js attempts to check the existance of an id, class, or html tag that matches the user's input.

This has two disadvantages:
1. If there is a CSS ID on the page that is the same as a class, the script won't match the scope correctly.

2. If the user enters a CSS ID in the way it is actually used in CSS (i.e. "#myidentifier") the script will throw JS errors and break everything.

I propose to amend the instructions to explain the proper way to write the selector and allow the script to use the selector as written.

I've created a patch to this effect.

For ease, here are the help instructions I've written:

Which portion of the body would you like to be resized by the Text Resize block? You may enter either the CSS class attribute, the CSS id attribute, or an HTML tag.
For example, if you want all text within <div id="my-container"> to be resized, enter the ID #my-container.
If you want all text within <div class="my-container"> to be resized, enter .my-container.
If, on the other hand, you would like all text within the BODY tag to be resized, enter body.

CommentFileSizeAuthor
text-resize-scope-css-id.patch2.57 KBjustindodge

Comments

attheshow’s picture

Category: bug » feature
justindodge’s picture

I filed under bug report because the current code will actually not work under certain conditions - the user cannot specify a class if there is an id with the same name, so the instructions in that instance are incorrect and the module will not function as intended.

I understand there is some ambiguity in the matter but I just want to make it clear that this is not purely a feature request but that there is an existing component that is broken as well. Have said that, I will leave you to classify it as you prefer.

attheshow’s picture

Status: Needs review » Closed (won't fix)

In my opinion, it's bad coding practice to use the same name in a theme for both an ID and a class, and it seems like kind of an edge case that someone would code their CSS in this way. If they do that, there's always the opportunity to use a different ID or class that's available within the document to remove the ambiguity that they're causing themselves.

justindodge’s picture

Users in Drupal don't often get a lot of direct opportunity to alter class names and ids that are used in markup.

Besides that, IDs are intended to be unique, class names are not, and there is no best practices to suggest that sharing a name among these different attributes is bad. Also, you are ignoring the fact that an HTML tag with the same name as a class or ID suffers from the same ambiguity issue with your code.

Best practice intentions aside, please remember that it's common for Drupal or any CMS to dynamically generate identifiers and so it's easy to end up with class names that match, for example, the letters of the alphabet. Instantly you've got several conflicts with various HTML tags. HTML is intended to be used this way, where classes can be very generic and ids very specific.

These conflicts may not be exceedingly common, but they are far from edge case and I could come up with a dozen more scenarios if you like.

Finally, the patch I submitted just makes the module more robust and free from complications and ambiguities in general. I'd be more apt to understand a "won't fix" without a patch, but I have to say, I just can't see any good reasons NOT to apply it.

Thanks for listening!

justindodge’s picture

Issue summary: View changes

Forgot to capitalize a letter in the instructions.