Remove eval() from tableHeader JavaScript

new Function is exactly the same as eval(). So no, it won't do.

Try going up from where Drupal.settings.tableHeaderOffset is defined, right now it's a string it should be a function that can be called directly without having to use eval.

Turns out, it's complicated who would have guessed :þ. There is a need for API change on ajax.js and tableheader.js.

Tableheader

Let's travel to 2007 when this eval() was introduced.

The issue is related to elements that are displayed fixed on top of the screen. If nothing is done, tableheader will set the header to go on top of the screen, under toolbar and shortcut. That's why the tableHeaderOffset setting was introduced, to have the offset from the top given by a js function that can tell the fixed element's height in order to display the header correctly.

Now there is 2 issues with this code (beside using eval()):

1. a reference to a js function has nothing to do in PHP, it's dynamic JS it has to be defined/detemined on the JS side, not PHP,
2. it's not sufficient, overlay_child.js had to monkey-patch it's way around :
   

   /**
    * Use displacement from parent window.
    */
   Drupal.overlayChild.behaviors.alterTableHeaderOffset = function (context, settings) {
     if (Drupal.settings.tableHeaderOffset) {
       Drupal.overlayChild.prevTableHeaderOffset = Drupal.settings.tableHeaderOffset;
     }
     Drupal.settings.tableHeaderOffset = 'Drupal.overlayChild.tableHeaderOffset';
   };
   
   /**
    * Callback for Drupal.settings.tableHeaderOffset.
    */
   Drupal.overlayChild.tableHeaderOffset = function () {
     var topOffset = Drupal.overlayChild.prevTableHeaderOffset ? eval(Drupal.overlayChild.prevTableHeaderOffset + '()') : 0;
   
     return topOffset + parseInt($(document.body).css('marginTop'));
   };

There should be a js-only solution to this problem. A script should only have to register it's function to tableheader that will go through the array of registered function summing it's results to end up with the right offset.

Ajax

This actually was introduced in ahah.js (yeah it's old too, both were a few days apart).

I'm a bit lost, I couldn't find code using this, to me that's dead code that should be removed, unless someone tell me which module uses it. The problem and solutions are the same as above, lucky.

No that won't work, the API needs to be changed it's flawed.

Here is a patch, a side effect is that It should be much smoother to scroll down a module list with a lot of packages now, eval was used at the worst possible place.

the API change is that Drupal.settings.tableHeaderOffset is now an array added by the theme_table function, where modules add their offset function, I got rid of overlay monkey-patch. Too bad jQuery doesn't have a .reduce() function that would have helped :þ.

I'm still looking for code that uses this.progress.update_callback in ajax.js. In the meantime please review this.

(edit) removed a bit of exaggeration :)

that's not an offset that's the base the number will be parsed with/in. it's because parseInt('08') gives an… interesting result. just fool-proofing this bit of code.

1) the var was defined by core/modules/toolbar/toolbar.module which has nothing to do with tableheader, I took it out and forced the empty array in the theme function to avoid painful checking on the JS side.

2) yeah wasn't inspired for comments.

3) It's more complicated, it's basically an inline if, expanded it's the same as :

var imax;
if (headerOffset) {
  imax = headerOffset.length;
}
else {
  imax = false;
}

// this works too
var value = condition && myDynamicValue();

It's to be extra sure Drupal.settings.tableHeaderOffset is defined and that the script won't choke on it.

About ajax.js since it's the same problem with the same solution I didn't think i needed a separate issue.

That makes sense, I like when js gets out from PHP.

met you halfway for the imax thing.

Comments might still need some work, i'm just not very good at it.

I broke stuff when the table gets reloaded by ajax.

Working patch. Did some profiling while scrolling on the module page (with just the list of core modules). What I'm looking at is the time spent in the function eventhandlerRecalculateStickyHeader which is called on page scroll, even if it's not scrolling over a table.

I'm scrolling gently on the module page like you'd do to look at modules titles (takes around 10 seconds). Below is the average time spent on this function, used Firebug profiler, google chrome profiler gives similar results.

Patched

• without overlay: 0.757ms
• with overlay: 0.622ms

Eval version

• without overlay: 1.358ms
• with overlay: 1.263ms

It's twice as fast now. Just need to check I haven't broken other things, ajax and overlay is working obviously :)

Oh and it looks like two binds are useless: drupalDisplaceAnchor.drupal-tableheader, drupalDisplaceFocus.drupal-tableheader. I didn't get how there were called.

Just tested on a website with around 70 tables on the module page (don't ask) half of that with sticky headers. It makes a very significant difference, so real world usage +1.

You're right, here it is fixed.

topoffset was always 0, because Drupal.overlayChild.prevTableHeaderOffset (which is Drupal.settings.tableHeaderOffset) is always undefined. what is defined here is top.Drupal.settings.tableHeaderOffset. there is an iframe involve and two different Drupal objects.

So no, i'm not changing the calculation. Moreover, Drupal.settings.tableHeaderOffset is an object now, you can add as many functions you want and they'll be summed up.

(edit) Oh and $(document.body).css('marginTop') is actually calculated earlier in the iframe creation and guess what it's calculated from… yes, the toolbar size.

It's just a big mess, this patch solve the eval issue, not the crazy way everything is calculated, that'll be a followup patch.

I'd like to keep talking in this issue: #1440628: [Meta] javascript toolbar/tableheader with url fragment mess there are links to other issues related there.

reroll

yeah yeah, failling patch is for D7.

All right I think I haven't explained enough. I'm changing the nature of the tableHeaderOffset setting.

Before it was a string that we called with eval() later on.
Drupal.settings.tableHeaderOffset = 'Drupal.toolbar.height';

This is not flexible. If another module, say overlay wants to add an offset it has to do monkey patching.

What it is now is an object of function:

Drupal.settings.tableHeaderOffset = {
  "toolbar": Drupal.toolbar.height, // this is a function!
  "overlay": Drupal.overlayChild.tableHeaderOffset // another function
};

And in the tableheader script, we just sum all the functions of this object. I choose an object instead of an array to be able to modify all that and avoid duplicates. extend is just an easy way to merge things and create what is needed if it doesn't exists. Since you can have several values, the || trick wouldn't work here.

Overlay is looking for the wrong value at the wrong place. Turns out this margin comes from toolbar to begin with, it should look for parent.Drupal.tableHeaderOffset instead of that. I'll change the patch :) didn't want to move things around too much for this patch.

So i'm actually making that extensible and contrib friendly. Which it is not right now.

The follow-up to make this generalized is to use a data-offset-top attribute on the right HTML elements and expose an event that scripts can be bound to and react in case the value changes. the values would be fetched from a "[data-offset-top]" CSS selector so that you won't have to care about which element it is.

There are other solution but this one would be the simplest from a code and extensibility perspective. I think there is an issue already i'll look it up.

reroll

See: #1574738: Rewrite tableheader.js

add reference to meta issue