When we use this on our tables, it strips out any other CSS classes that we want to use (e.g. <table class="msul-table-stripes"> becomes 

<table class="tablesaw tablesaw-stack" id="table-696" data-tablesaw-mode="stack">

. The existing classes and attributes should be retained. The issue lies with the preg_replace in line 24 which assumes that there are no other classes.

CommentFileSizeAuthor
#6 responsive_tables_filter-append_classes_2575033-6-D7.patch1.23 KBmark_fullmer
#3 responsive_tables_filter-regex_strips_attributes-2575033-3-D7.patch4.54 KBkwfinken
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kwfinken created an issue. See original summary.

kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented

Doing some research, using regex is really the wrong tool for this...html is not regular. This can be more appropriately handled through DOM or string handling techniques. I will try to have a patch available for testing tomorrow.

kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented
FileSize
responsive_tables_filter-regex_strips_attributes-2575033-3-D7.patch4.54 KB

The attached patch leaves other attributes and class values intact while adding the necessary tablesaw attributes and classes.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
Status: Active » Reviewed & tested by the community

Thanks for this patch, Kevin. I'll plan to commit it in the next day or two.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Hi Kevin,

After reviewing the code again, the patch contains a typo (parenthesis in the wrong position). As it is now, the below condition will evaluate as FALSE if the position of ' class' is less than the end of the current table tag:

    if (($class_start = stripos($text, ' class', $table_start) > $table_end) ||
        ( $class_start === FALSE)) {

The correct placement is here:

    if (($class_start = stripos($text, ' class', $table_start)) > $table_end ||
        ( $class_start === FALSE)) {

Having said that, I also think that this is not the ideal (or most efficient) way to check for class elements in the table tag and insert the desired tag among them, as it is in essence a find-replace approach.

Instead, using PHP's DOMDocument method should allow us to get the existing class elements as an object and then add to them the tablesaw class as necessary. I'll post something using this approach shortly.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
FileSize
responsive_tables_filter-append_classes_2575033-6-D7.patch1.23 KB

This patch should accomplish the following:

  • Loop through all tables in the user-submitted field.
  • Append tablesaw class to tables with existing class attributes, else create a class attribute.
  • Insert the tablesaw data mode attribute regardless of its presence.
mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
Status: Needs work » Needs review

  • mark_fullmer committed 35cb6f7 on 7.x-1.x 
    Issue #2575033 by kwfinken, mark_fullmer: other CSS Classes stripped out...
kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented

I'll try out the patch tomorrow at work. No doubt that the DOM approach is superior, but I was under a time crunch and didn't have time to learn the DOM functions. Being an old school programmer (I started using C in 1985 and Pascal in 1980), string techniques are an easy fallback.

Either way, this at least got the direction changed to saving existing classes and attributes for all tables on a page, so it advances the usability of the module. We all win!

kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented
Status: Needs review » Needs work

Not working. Get errors:
Warning: DOMDocument::loadHTML(): Tag svg invalid in Entity, line: 1 in _tablesaw_filter() (line 25 of /var/www/mainweb/sites/all/modules/responsive_tables_filter/responsive_tables_filter.module).
Warning: DOMDocument::loadHTML(): Tag path invalid in Entity, line: 1 in _tablesaw_filter() (line 25 of /var/www/mainweb/sites/all/modules/responsive_tables_filter/responsive_tables_filter.module).
Warning: DOMDocument::loadHTML(): Tag text invalid in Entity, line: 1 in _tablesaw_filter() (line 25 of /var/www/mainweb/sites/all/modules/responsive_tables_filter/responsive_tables_filter.module).
Warning: DOMDocument::loadHTML(): Tag tspan invalid in Entity, line: 1 in _tablesaw_filter() (line 25 of /var/www/mainweb/sites/all/modules/responsive_tables_filter/responsive_tables_filter.module).

It looks like the problems are with SVG graphics on our page, but looking into web reports for DOMDocument, it looks like it may not support most HTML5. See http://stackoverflow.com/questions/9149180/domdocumentloadhtml-error, http://stackoverflow.com/questions/6090667/php-domdocument-errors-warnin..., http://php.net/manual/en/libxml.constants.php (especially LIBXML_HTML_NODEFDTD).

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
Status: Needs work » Needs review

Thanks for testing this. Yes, it does indeed look like the DOMDocument class cannot parse HTML5 elements. In most instances, I would agree with the user on http://stackoverflow.com/questions/6090667/php-domdocument-errors-warnin... who says "Error suppression is not the way to deal with this."

However, in this particular instance, since we are using DOMDocument only to parse the table tag (non-HTML5), we actually can ignore HTML5 elements for this class. Therefore, I'm in favor of using the following:

libxml_use_internal_errors(TRUE);

Tested this locally with HTML5 tags in the markup. You can verify it works for you on the develop branch: https://www.drupal.org/node/2549781

kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented
Status: Needs review » Reviewed & tested by the community

Looks good on my end. No errors and keeps the HTLM5.

Did reveal an issue on my end. The designer had used the character … instead of &hellip; which, after processing through DOM functions got changed to …. Only mentioning it in case someone gets a similar issue and tries to blame the module.

kwfinken’s picture

kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commented

Since there are no additional inputs, I suggest taking the fix live as 7.x-1.1. Since you are the owner, you will have to make the change.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented

I just pushed the 7.x-1.1 tag and will close out this issue accordingly.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
CreditAttribution: mark_fullmer as a volunteer commented
Assigned: Unassigned » mark_fullmer
Status: Reviewed & tested by the community » Closed (fixed)