Fusion RTL issues

Hi Amnon! Thanks for trying it out on your site. We really need folks like you with real RTL sites to help test, as our little testing sites only go so far :)

However, Acquia Prosper isn't yet RTL-themed. There's an issue open to do this and we'll be tackling it soon though: http://drupal.org/node/649184

If you could try out Fusion Starter as your default theme and report back with any issues, that would be extremely helpful.

One thing to note is that the text in the Skinr styles for left/right in blocks will actually be reversed in RTL. ie. the setting that says "Float block to the right" will actually mean left when you're viewing in RTL.

Thanks again!

I was playing today with Fusion Starter on an RTL (hebrew) site I work on.
I'm afraid to say this may be the best RTL/BiDi theme I have worked with, and I am a big fan of Tendu - by far the best starter BiDi theme until Fusion (see comparison table attached here, once published here , look for RTL support).
I work on linux firefox, everything went fine obviously, so I started up my xp VM and tryed IE6, 7 and 8 - at first I was disapointed but then I remembered I didn't have css optimization on, ten seconds later everything was perfect.
The only thing I have noticed unRTLized is the float of -
#content-tabs ul.primary, #content-tabs ul.secondary
and
#content-tabs ul.primary li, #content-tabs ul.secondary li
but I know you will fix this soon :)
If I remember tommorow I'll make a patch.
Great job, keep it going.

Issue in comment #2 fixed in Fusion 6.x-1.0-rc1

fusion core does not load the gridxx-960.css it just loads gridxx-960-rtl.css

so I added missing bits into rtl ones.

PS: this patch is for Fusion core, should work on any rtl subtheme

sorry about the patches, they were SVN patches. Uploaded again

I have applied this file grid16-960-rtl.css_.patch since it is the only one that I need at my site. Works fine. Thanks very much fo the effort.

Yes. The patches here work and they need to be applied in future releases.

I also checked this patches and it works. please apply in future releases.

The patches provided in #6 just duplicate codes from gridX-Y.css in gridX-Y-rtl.css and don't solve the main problem in a proper way. The problem comes from the following line in the fusion_core_preprocess_html() of the template.php of fusion core theme:

drupal_add_css($path . '/css/' . $file, array('basename' => $name . '-' . $file, 'group' => CSS_THEME, 'preprocess' => TRUE));

If you remove basename option from this line like below, everything will work correctly:

drupal_add_css($path . '/css/' . $file, array('group' => CSS_THEME, 'preprocess' => TRUE));

It seems that the basename option some how removes gridX-Y.css from the css queue for RTL languages. I wonder if this is a fusion theme bug or drupal 7 core's.

The suggestion inside reply #13 by Massud works fine.

I knew about this by the way, just though a css patch would be easier for others. I will update the patch to reflect your suggestions. the css duplication is not a good solution indeed.

However, some subthemes are made without base css files. they work now but if you apply this patch you probably brake them. It's a bug people got used to in subtheme building. If you apply this patch instead of CSS patches, you may break your theme everytime updating fusion. remember to apply it everytime. Looks like we're not getting a fix by Fusion developer here.

revised

Patch by Hadi Farnoud (#19) works fine inside the new version (version 2)

I'm not quite sure I understand the implications of this patch. Fusion is assigning a basename equal to the name of the theme (eg: "fusion_core") + the name of the grid file (eg: "grid12-960.css"). That should make each file called by drupal_add_css() to have a unique basename across themes, even if the same filename (eg: "style.css") is used.

The doc on the basename option isn't very clear to me, but I take it this is some form of namespacing?

'basename': Force a basename for the file being added. Modules are expected to use stylesheets with unique filenames, but integration of external libraries may make this impossible. The basename of 'modules/node/node.css' is 'node.css'. If the external library "node.js" ships with a 'node.css', then a different, unique basename would be 'node.js.css'.

What I don't understand is why namespacing is required if the two node.css files in the above example are in distinct locations. I also don't understand the term "basename" in this context, or how it's used. In PHP, the basename() function returns the trailing name component. I don't know now that relates to adding CSS files.

You folks seem to have a better understanding, so any tips would be helpful.

If the basename is causing incompatibilities with RTF, that does sound like a potential bug in core and the issue should be filed there to rule that out.

As #13 said, basenamejust makes the naming convention wrong for some reason. if you try it with an RTL language like Farsi, you'll see the css is completely messed up.

This might not matter to LTR languages. But for RTL languages (which are many) is a serious bug! Please get this bug the attention it deserves.

Hi Hadi, the proposed patch could have implications on thousands of sites running Fusion, so I need a clearer understanding of why the patch in #19 fixes the issue. "makes the naming convention wrong for some reason" doesn't provide much information. Why is basename causing problems? What affect will removing it create for other sites?

Thank's for that.
Patch in #19 works grate for me.
Should be committed, no?

I changed the line manually in fusion_core/template.php and it solved the issue, thanx Hadi.

However, I tested it as a patch and it did not work in my case.
I never tried to patch drupal before so it is most likely my bad, what I did is put the patch file in sites/all/themes/fusion , and run the following:
patch -p0 --dry-run patch_file_name.patch
It did not output anything, also never returned me to the prompt.
tried it also from drupal base dir and got the same, didn't try without the dry-run.
Only reason I bother this list by reporting this, is because I see recent activity here so - just in case I save someone a few minutes.

As I said, the suggested fix did work.

Can anyone answer the question that aquariumtap posed in #24? In the very least, any assistance we could have in recreating and testing this issue for ourselves would be very helpful. If anyone has a multi-lingual demo site that presents this RTL issue and can share the code and database with us, it would help us understand the issue, test the patches and get this fix into a release.

Thanks!

patch #19 worked partially
And the answer for why it worked that because using "drupal_add_css" with "basename" is to force basename in case of multiple modules with similar names and since "drupal_add_css" called with full path name so there no place for other css files to get loaded.

also in the following code from drupal show why "basename" is not needed if we provide "filename" ($item['data'])

function drupal_get_css($css = NULL, $skip_alter = FALSE) {
 ...

  $previous_item = array();
  foreach ($css as $key => $item) {
    if ($item['type'] == 'file') {
      // If defined, force a unique basename for this file.
      $basename = isset($item['basename']) ? $item['basename'] : basename($item['data']); 

#19 fixes the issue #28 explained. I tested this patch on both English and Farsi version. Didn't break mine. I don't think it does any harm to existing themes. Can anyone else test and report back?

this patch for beta2 I test it in English and Arabic.

#30 patch works for English and Arabic Languages.

You can test it here
http://tech-news.dreamswebsite.com

#30: CSS_RTL_fix_beta-2.patch queued for re-testing.

path #19 works for master branch.
Thank you Hadi Farnoud

Hi Hadi,

I'd love to get this into Fusion for you but can you help clarify? I see a patch against beta2 in two places (#19 and #30) - Could you possibly regenerate this patch for the 7.x-2.x-DEV version? If you can do that, I'll test it and we'll get this into Fusion ASAP.

Thanks for your work!

Also moving this into DEV version.

@webkenny

I tried the dev version and fixed every error I saw. I might have missed something.

oops. correct patch

is someone going to review this patch?

This patch file seems a bit odd. Can you review and re-post?

This patch file seems a bit odd

that's not a helpful comment. please do tell what's wrong with it.

it's just a css patch anyway. what could be possibly confusing?

Sorry. The contents of the patch seem to be corrupted. It contains a massive string of characters and no discernible code.

the patch is fine. I'm not sure why it's in binary format. but if you apply it, you'd see it's just css changes.

I'll try to find out why SourceTree produced it in this way. will re-upload another one.

this must be fine. please review it.

Thanks again. I'm assuming this is in addition to the patch in #30 which updates the drupal_add_css call? Is that correct?

yes @Poieo. I assumed that patch got applied long ago.

Unfortunately no. Development pretty much stopped 2 years ago. But, we've taken over and we're trying to bring this great theme back to life.

basename is used to call grid, local, and responsive per the code below so I'm going to test out removing it from all of them.

    // Fusion Accelerator does not exist, or responsive has been disabled.
    // Search themes for grid files, and use those.
    $vars['mobile_friendly'] = FALSE;
    $vars['classes_array'][] = theme_get_setting('sidebar_layout');

    foreach ($themes as $name => $path) {
      if (empty($cache->data['grid_css'][$name][$file])) {
        if (file_exists($path . '/css/' . $file)) {
          drupal_add_css($path . '/css/' . $file, array('basename' => $name . '-' . $file, 'group' => CSS_THEME, 'preprocess' => TRUE));
          $new_cache['grid_css'][$name][$file] = TRUE;
        } else {
          $new_cache['grid_css'][$name][$file] = FUSION_FILE_NOT_FOUND;
        }
      } else {
        if ($cache->data['grid_css'][$name][$file] === TRUE) {
          drupal_add_css($path . '/css/' . $file, array('basename' => $name . '-' . $file, 'group' => CSS_THEME, 'preprocess' => TRUE));
        }
      }
    }

  }

  // Include any instances of local.css.
  foreach ($themes as $name => $path) {
    // Include any instances of local.css.
    if (empty($cache->data['local_css'][$path])) {
      if (file_exists($path . '/css/local.css')) {
        drupal_add_css($path . '/css/local.css', array('basename' => $name . '-local.css', 'group' => CSS_THEME, 'preprocess' => TRUE));
        $new_cache['local_css'][$path] = TRUE;
      } else {
        $new_cache['local_css'][$path] = FUSION_FILE_NOT_FOUND;
      }
    } else {
      if ($cache->data['local_css'][$path] === TRUE) {
        drupal_add_css($path . '/css/local.css', array('basename' => $name . '-local.css', 'group' => CSS_THEME, 'preprocess' => TRUE));
      }
    }

    // If responsive is enabled, include any instances of responsive.css.
    if (module_exists('fusion_accelerator') && theme_get_setting('responsive_enabled')) {
      if (empty($cache->data['responsive_css'][$path])) {
        if (file_exists($path . '/css/responsive.css')) {
          drupal_add_css($path . '/css/responsive.css', array('basename' => $name . '-responsive.css', 'group' => CSS_THEME, 'preprocess' => TRUE));
          $new_cache['responsive_css'][$path] = TRUE;
        } else {
          $new_cache['responsive_css'][$path] = FUSION_FILE_NOT_FOUND;
        }
      } else {
        if ($cache->data['responsive_css'][$path] === TRUE) {
          drupal_add_css($path . '/css/responsive.css', array('basename' => $name . '-responsive.css', 'group' => CSS_THEME, 'preprocess' => TRUE));
        }
      }
    }
  }

Attached patch removes basename and adds some additional rtl css.