Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)

adding Spark and sprint tags

Thanks LewisNyman and tarekdj for the patches. I think the approach represented in the patch in #20 has legs, but as mkadin points out in #24, the CSS responsible for positioning the overlay container doesn't respond well at narrow screen widths.

The approach in the latest patch is a great start. I'd like to suggest we continue down this route and further abstract the displacement code in overlay so that overlay isn't referring so directly to toolbar dimensions, like in this example below.

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -488,8 +488,13 @@ Drupal.overlay.eventhandlerAlterDisplacedElements = function (event) {
     // Prevent displaced elements overlapping window's scrollbar.
     var currentMaxWidth = parseInt($(this).css(maxWidthName), 10);
+    // If toolbar is visible add it's width to maxWidth
+    var toolbarWidth = 0;
+    if ($(".tray.vertical.active").width()) {
+      var toolbarWidth = parseInt($(".tray.vertical.active").width(), 10);
+    }
     if ((data.drupalOverlay && data.drupalOverlay.maxWidth) || isNaN(currentMaxWidth) || currentMaxWidth > maxWidth || currentMaxWidth <= 0) {
-      $(this).css(maxWidthName, maxWidth);
+      $(this).css(maxWidthName, maxWidth + toolbarWidth);
       (data.drupalOverlay = data.drupalOverlay || {}).maxWidth = true;

A module like toolbar should be able to register that it has visual components that require displacement on the top and left/right. A module like overlay could register that it needs to be displaced on the top, left and right. In contrast, a component like sticky table headers would only be interested in top displacement.

Currently the displacement code lives in the overlay javascript. I'd like to see us generalize these behaviors to the Drupal object, so that screen real estate isn't managed in any one particular module.

#27

This is a good idea but it is quite complicated. As you can see Toolbar has class on "body" that adds a left margin, if it would work like you describe then if there was a other module that would display something similar to toolbar on screen but for differeren purpose then it would have to register it's own displacement and so on..

Oh, I do agree. It is quite complicated! Nonetheless, I thought I'd give it a spin. We've got a few weeks to really take a hard look at integrating the features we just landed. I'd love to see us clean up some D7 cruft and make the new features play nice as well.

So I thought, what if we have a utility called Drupal.displace. It is responsible for finding the .data-offset-{edge} classes, getting their dimensions and triggering an event when those values change. A module like overlay just needs to listen for that event and update its dimensions based on the values it gets from the utility. The responsibility of measuring the elements is no longer located in Overlay.

By moving this code into a utility, we start to build an approach that could help us address this issue as well: #1827318: Fix tableheader placement which is broken in the current D8 ToolBar patch.

What I've got in this patch is still pretty rough. I just wanted to establish a draft framework, details to be cleaned up.

What this buys us is that overlay no longer refers to toolbar. So if in contrib later, someone builds a toolbar replacement, they don't have to override overlay as well to get the same or similar offset behavior.

#30

Sorry for my "it's complicated". I'm starting with helping with core and I though that we are looking for simple bug-fix. Obviously I prefer the way jessebeach suggested.

No need to be sorry! I agree, it is complicated. Sometimes we need complicated, sometimes we don't. I like to explore the complicated and see if we can pull something simple out of it. I'm glad you're starting with core. There are a lot of patterns and sub-systems buried in here and it just takes time to get familiar with them. I learned by working with others who have more experience than me. I like discussing options with folks to see if there's anything I haven't considered yet so if you're up for it, we can explore some options here as well.

What problems did you have with the patch in #29? Applying or creating an updated patch from the changes? Are you sure you applied the patch in #29 to the latest 8.x branch? Not the branch with your patch from #27 already applied. I just tried the patch in #29 applied to the latest 8.x commit (a3064d2020d442266b3bcdf70029775a72f67dae) and it applies cleanly.

I like using JavaScript classes, especially when we need to store state on a per-instance basis. From your patch in #31, I assume we would decorate elements like the toolbar container and the tray container with a .displace class. These we would create a Displace class instance for each of these elements.

In your model, how do you foresee us handling these situations?

1. Letting a module know what elements on the screen have displacement values.
2. A screen resize.
3. Displace elements resizing or toggle visibility.

For (1), I assume that a module calls Drupal.displace.getOffsets(true[, '{edge}']). I am, however, limited to the values at the precise moment I call this function, so we might run into race conditions if elements are still drawing or if the order of operations is not maintained.

For resizing and displacement element dimension changes, we need a way to update interested elements that they need to reposition themselves. The best way I could come up with to do this is to dispatch an event with the most current offset values and let other modules listen for these events and react as they want.

Attaching a handler to the document is a way to capture bubbled events: http://www.quirksmode.org/js/events_order.html. It's also a singular clearing house for custom events. We use this pattern a lot in core, especially to pass state information between modules without those modules referring directly to each other's methods.

Contextual module listening for an Edit module event
$(document).on('drupalEditModeChanged.contextual', toggleEditMode);

We always name these events with the pattern drupalPropertyEventtype.

You're right, it does require some documentation to know about these events. My personal philosophy is that contrib should be able to get by without knowing anything about these sub-systems. But when you do, you can build complex administration features that interact well with the subsystems or even completely replace them.

By going with a utility approach to managing element displacement, we'll need to address the following tasks

1. Update the sticky table headers to use this utility.
2. Clean up and optimize the displace utility code.
3. Further clean out overlay. Its eventhandlerAlterDisplacedElements is setting max widths and heights in a way that's no longer compatible with this new approach.

You could use XHTML5 to validate the data attributes: http://www.w3.org/TR/html5/introduction.html#html-vs-xhtml

#33

2. I'd suggest recalculating all offsets every time window resizes
3. Provide a method in displace object to recalculate offsets. This could be also resolved by custom event. Default resize event is only triggered on window.

Ok, cool, I think we did land on the same approach. Here's the resize handler in my patch from #29 (admittedly still very rough). It should only be triggering drupalViewportOffsetChange if the offset values change, not on every window resize event.

  /**
   * Registers a resize hanlder on the window.
   */
  Drupal.behaviors.drupalDisplace = {
    attach: function (context, settings) {
      $('body').once('drupalDisplace', function (index, element) {
        findDisplacingElements();
        $(window).on('resize.drupalDisplace', displace);
      });
    }
  };

/**
   * Informs listeners of the current offset dimensions.
   */
  function displace() {
    findDisplacingElements();
    calculateOffsets();
    $(document).trigger('drupalViewportOffsetChange', offsets);
  }

And to get the offsets outside of the event layer, we have:

/**
   * Returns the current offset dimensions.
   */
  displace.getOffsets = function (refresh) {
      if (refresh) {
        calculateOffsets();
      }
      return offsets;
  };
// Invoked like so.
var currentOffsetValues = Drupal.displace.getOffsets(true);

Also, when applying a patch, you can use the --reject flag to at least apply the parts that aren't in conflict, for example:

git apply ~/path/to/some.patch --reject

We're building to a doctype of <!DOCTYPE html>. I've not heard anyone mention the need to support validation of HTML4.X or XHTML 1.X.

What about percentage width?

We need to recalculate the displacement offsets on window resize, but only changes need to be broadcast. So if we compare the fresh calculations against the stored values from the previous resize, we can determine if they changed and only fire drupalViewportOffsetChange when they do. Sound good?

We don't want to overdose with calculations while resize, especially on mobile devices with slower cpu's.

That shouldn't be an issue since resizes happen rarely on small devices where the viewport is often simply maximized and unresizable.

what would happen if someone wants to change dimensions in his theme using CSS?

We should be fine there too, since we're getting the calculated dimensions of the elements to determine the displacements. As long as we get the width/height. padding and border sizes for any given box that will be considered part of the displacement value for an edge, we should be fine. The only way we could really know if an element changed size outside of a screen resize is to track DOM mutation events, but those aren't well supported at all yet. So any module that alters its view (like the toolbar and its tray) will need to call some method of displace to trigger a reassessment of the displacement values and possible trigger a drupalViewportOffsetChange event if they've changed. I don't like that part, but it seems hard to avoid if we can't simply monitor DOM elements for attribute changes.

Have you seen Ubuntu Touch? One of its features is that you can have browser on the left part of screen and something else on the right. The middle edge can be moved so window resize happens. I guess in future there will be more devices with such feature.

I haven't seen it, but I think I know what you're talking about.

Check out this issue for a list of the followups. You're welcome to add a feature request for this idea. I like it.
#1846970: [meta] Responsive Toolbar follow-ups

Some thoughts on your suggestions:

Provide a way of calling 'resize.drupalDisplace' only for specified element. It would be better if we could not only tell that element box has changed but also tell displace api what element has changed. Having this feature calculations would be done only for changed element. If we stored element previous offset then we would know what change it was and we could immediately adjust offsets.

I think I understand. What I'd like to avoid is having the Overlay ask Drupal.displace for the size of a specific element. This is an end-run around the API. Overlay shouldn't need to worry about referring to DOM elements directly or knowing anything about their classes or ids.

Do we really have to call "findDisplacingElements()" every time displacement happens? Why isn't this called only in behavior using context? Isn't it additional work every time "resize.drupalDisplace" is called?

You're quite right, we shouldn't need to do this. The only alternative I can come up with is any module, like Toolbar, that has DOM elements that will displace other elements, will need to proactively tell Drupal.displace what elements it should take into account. I'm not thrilled about that option, because it's more brittle. Take a shot at it and let's see what your ideas look like in code.

Another thing is calculating an offset. Now it is taken from .data-offset-{edge} and if this is not existing then only width is taken. First of all instead of "width()", "outerWidth()" should be called because it includes paddings.
Anyway I think that .data-offset-{edge} is way better than width() or outerWidth() because it gives modules a opportunity to set offset by itself. But what would happen if we had two toolbars on the left and one would be under another? In this case calculations would be wrong. I'm aware that this example is weird but this is going to be an api not only for toolbar module so we should think about such options.

To prevent mistakes in calculations we could use something like .offset().left + .outerWidth() for left edge in example. This guarantees us that we always get proper offset no matter how elements are positioned.

Yes, we should be using outerWidth and taking a displaced element's offset into account as well. Calculating right and bottom displacement will take a little more code on top of that. Don't forget about RTL languages like Hebrew, too.

I think you're in a good place to start coding it up!

I'm not following you, what feature request are you talking about? I got lost.

Oh, I meant if you'd like to add functionality to allow a user to resize a vertical try, you should open an issue for that.

trawekp, I'm working on an update of your patch in #46. I should have that posted today. There was some funkiness with its interaction with the Overlay that I'm smoothing out. Otherwise it looks great so far!

trawekp, this update to the patch addresses issues with the overlay and tableheader sticky tables. I also worked on the overlay styling a little so that it fits into a narrow space with more grace.

This is what I think we have left to do:

1. We need some kind of cacheing or at least debouncing so we're not going into the DOM on each window resize.
2. We need tests using testswarm.
3. The code should be gone over and cleaned up for core: comments, docblocks, etc.

There is an example of using Testswarm to set up front end tests here: http://drupal.org/files/mobile-preview-1741498-67.patch

What do you think trawekp? Want to get this up to the line then I'll do a review? I'll try to get some other folks in here as well now that we're pretty far along with it.

trawekp, box-sizing is one of those rare CSS properties that enjoys robust support in older versions of IE. It's very handy indeed. I'd rather keep it in so that we get consistent width calculation, especially if we set min or max widths on the overlay container.

http://caniuse.com/#search=box-sizing

Ok, I agree that getting rid of the resize handler in displace.js is a good thing. I went one step farther in this patch and removed the behavior altogether:

-  Drupal.behaviors.drupalDisplace = {
-    attach: function (context, settings) {
-      // once() returns a jQuery set. It will be empty if no unprocessed
-      // elements are found. window and window.parent are equivalent unless the
-      // Drupal page is itself wrapped in an iframe.
-      $(window.parent.document.body).once('drupalDisplace', function () {
-        // If this window is itself in an iframe it must be marked as processed.
-        // Its parent window will have been processed above.
-        // When attach() is called again for the preview iframe, it will check
-        // its parent window and find it has been processed. In most cases, the
-        // following code will have no effect.
-        $(window.document.body).once('drupalDisplace');
-        findDisplacingElements();
-        // Displace attaches a drupalOffsetChange handler to the parent body
-        // the handler is wrapped with debounce 
-        // in case of offsets changed on every resize
-        $(window.parent.document.body).on('drupalOffsetChange.drupalDisplace', 
-          Drupal.debounce(displace, 200));
-        });
-    });
-  })
- };

I'd rather we put the trigger of the offset change in the Drupal.displace() interface, so I changed this code back:

-    $toolbar.trigger('drupalOffsetChange');
+    // Trigger a recalculation of viewport displacing elements.
+    Drupal.displace();

One of the purposes of the using Drupal.displace is to eliminate the need for modules to know how to calculate the displacement values and what event to trigger.

Your changes inspired me to change the passive displacement element discovery process into an active registration process. This way we don't need to go through the DOM each time we want to calculate the offsets on the off chance that another displacing element is present. Now, the toolbar registers the elements that have the ability to displace the overlay.

/**
   * Registers elements that should be used to determine viewport offset sizes.
   */
  displace.registerElements = function (elementsByEdge) {
    // Go through each edge in elementsByEdge.
    for (var edge in elementsByEdge) {
      if (elementsByEdge.hasOwnProperty(edge)) {
        // Each element or set of elements must be listed under an edge
        // property.
        if ($.inArray(edge, ['top', 'right', 'bottom', 'left']) === -1) {
          continue;
        }
        var elements = elementsByEdge[edge];
        // Wrap the elements in an array if just one element is provided.
        elements = (!'length' in elements) ? [elements] : elements;
        // Loop through each element and add it to the list of displacing
        // elements for the given edge.
        for (var i = 0, n = elements.length; i < n; i++) {
          // The elements will be stored as DOM nodes for consistency.
          var el = ('get' in elements[i]) ? elements[i].get() : elements[i];
          displacingElements[edge].push(el);
        }
        // Sort the displacing elements for the edge and remove duplicates.
        displacingElements[edge] = $.unique(displacingElements[edge]);
      }
    }
  };

Let me know what you think of this approach. I think it solves our efficiency issues.

This patch improves the offset value calculation. Also, I removed an event trigger that passed the offset change event to the document in the iframe. It's unnecessary to propagate this event down into the iframe document.

-    // Workaround because of the way jQuery events works.
-    // jQuery from the parent frame needs to be used to catch this event.
-    parent.jQuery(document).on({
-      'drupalViewportOffsetChange': function (event, offsets) {
-        // Fires an event that the child iframe can listen to.
-        $(document).trigger('drupalViewportOffsetChange', offsets);
-      }
-    });

I'm not sure why calling a method is better than triggering an event.

In displace.js, we provide the offsets as a parameter when the drupalViewportOffsetChange event is triggered:

function displace() {
  calculateOffsets();
  $(document).trigger('drupalViewportOffsetChange', offsets);
}

If we just trigger this event from toolbar.js, then we would have to calculate the offsets in toolbar.js and send those along when the event is triggered. By calling Drupal.displace(), we put that logic in a common location and toolbar no longer needs to worry about it.

Do you have in mind that fluid elements would call the method on every window resize? Debounce would be a good thing in such situation.

Right now, nothing using Drupal.displace is registering elements that need to be checked on every resize event. Toolbar monitors its elements and calls Drupal.displace when they change. For the toolbar height change, it wraps its resize handler in Drupal.debounce. We might eventually need to put a resize handler in Drupal.displace later, but at the moment, we don't need it.

And the patch.

As I can see you've removed a behavior. I guess you hadn't have enough time and this is something I should take care of.

I don't think we have a use case for this behavior yet.

But it was a different event and Drupal.displace was called when that event was triggered. I used event because it was easier to attach a debounce. If we're not going to use debounce now then it's better to stay with method, as it is more elegant (i guess).

Currently, the toolbar module is wrapping the setHeight method in a debounce function. The tray width (left offset) is triggered on a matchMedia change event, not screen resize, so we don't need to debounce it. I think we're set here.

// Call setHeight on screen resize. Wrap it in debounce to prevent
// setHeight from being called too frequently.
var setHeight = Drupal.debounce(Drupal.toolbar.setHeight, 200);
// Attach behavior to the window.
$(window)
  .on('resize.toolbar', setHeight);

From your comments in #57, I see an incompatibility in these two approaches you describe. It seems you took the first one in your patch, but are thinking about following the second.

I've merged all elements into one array (elements are no more grouped by edge). I think that code seems simplier now.

Calculating offsets should be smarter. As long as there is one element per edge it's ok but there might be more (we are allowing for this) calculations might get totally wrong. I think that the only way to resolve this problem is to calculate offsets all by ourselves. Calculations would be still simple so it's worth trying.

If we eliminate the edge as a key in the object of displacing elements, then it becomes impossible to calculate those offsets in Drupal.displace. I agree with your second quote. Here's my logic:

1. Calculating viewport offsets is non-trivial for any element that isn't right up against the edge.
2. The toolbar calculates its own offsets because of the CSS of its contents, so we need a way for an offset element to override Drupal.displace calculations.
3. In the future, module developers might want to develop their own administration toolbars. They should have to reinvent placement and interaction with the overlay.
4. So, we need a common way to calculate offsets for both core and contrib modules.

So, where do we go? I followed your thinking about making the offset calculation smarter. It now takes the element's offset value into account. And only the largest displacing element for an edge is used. So if multiple displacing elements exist on an edge, the smaller ones are ignored.

function calculateOffsets () {
  // Go through each edge and add up the displacements.
  for (var edge in displacingElements) {
    if (displacingElements.hasOwnProperty(edge)) {
      var displacement = 0;
      // Zero out the offset for this edge.
      offsets[edge] = 0;
      // Go through the displacing elements on each edge and calculate the
      // offset.
      for (var i = 0, n = displacingElements[edge].length; i < n; i++) {
        var $el = $(displacingElements[edge][i]);
        // If the element is not visble, do not use its dimensions.
        if (!$el.is(':visible')) {
          continue;
        }
        // If the offset data attribute contains a displacing value, use it.
        displacement = parseInt($el.attr('data-offset-' + edge), 10);
        // If the element's offset data attribute does not contain a value,
        // try to get the displacing dimension from the element directly.
        if (typeof displacement === 'undefined' || typeof displacement !== 'number' || isNaN(displacement) || displacement < 0) {
          var size = 0;
          // Get the offset of the element itself.
          var placement = $el.offset()[(edge === 'left' || edge === 'right') ? 'left' : 'top'];
          // Subtract scroll distance from placement to get the distance
          // to the edge of the viewport.
          placement = placement - window['scroll' + ((edge === 'right' || edge === 'left') ? 'X' : 'Y')];
          // Find the displacement value according to the edge.
          switch (edge) {
            // Left and top elements displace as a sum of their own offset value
            // plus their size.
            case 'top':
              size = $el.outerHeight();
              // Total displacment is the sum of the elements placement and size.
              displacement = placement + size;
              break;
            case 'left':
              size = $el.outerWidth();
              // Total displacment is the sum of the elements placement and size.
              displacement = placement + size;
              break;
            // Right and bottom elements displace according to their left and
            // top offset. Their size isn't important.
            case 'bottom':
              var docSize = document.documentElement.clientHeight;
              displacement = docSize - placement;
              break;
            case 'right':
              var docSize = document.documentElement.clientWidth;
              displacement = docSize - placement;
              break;
            default:
              displacement = 0;
          }
        }
        // If the displacement value is larger than the current value for this
        // edge, use the value.
        if (displacement >= 0 && displacement > offsets[edge]) {
          // Store the displacement value in the closure's offsets variable.
          offsets[edge] = displacement;
        }
      }
    }
  }
}

For instance, the overlay is being displaced here by two elements (orange and red) that a offset and bigger than the toolbar elements.

We've still got the [data-offset-{{edge}}] override for toolbar, too.

I also wrote a test for calculateOffsets method. It's in a patch file included in this post. It applies to the 8.x-1.x branch of the testswarm project. This is not ideal. I talked to some of the Drupal JS folks and I think we'll be creating a new module that will contain all the Drupal 8 front end tests. Until then, we're stuck with this patch. Once you've got the testswarm module installed (You'll need to enable the testswarm_tests module inside of the project as well), go to this path on your installation.

?testswarm-test=displace&debug=on

The displace test should run. The file for the test is drupal.displace.test.js in the testswarm_tests submodule of the project.

Work left to do

We need to fix the Drupal.overlay.eventhandlerAlterDisplacedElements method in overlay-parent.js. Somewhere in there, it's restyling the top and bottom displacing elements so that this happens:

There are two elements that appear to be cut off on the right side because the overlay is setting a max-width on them that is much more narrow than the document. This code was originally there to prevent the iframe's scrollbar from going underneath the toolbar. It's not doing the right calculations any more.

The other issue is the Drupal.displace.removeElements method that needs work in a scenario where we're using the edge as a key to the displacing element object.

Not at all. "data-offset-{edge}" tells us when to calculate the edge. Also there is no problem to store the edge with an element

So what if we say that if an element has [data-offset-{edge}], use it to calculate the offset and since no value is provided, calculate it. If an element has [data-offset-{edge}="{value}"], use it to calculate the offset and use the value of the attribute to do it (skip calculations).

This would eliminate the need for registration and turn displace into a simple API for triggering a change event and calculating offsets if necessary.

I can totally back that.

Testswarm: http://drupal.org/project/testswarm
Front-end Tests: http://drupal.org/project/fat

I'm working on responses to your questions in #61.

Testswarm: http://drupal.org/project/testswarm
Front-end Tests: http://drupal.org/project/fat

From #61

Now everything looks nice but calculateOffsets. It is written very smart but still it can scare you. This needs some clean up. I think that moving direct calculation to another function would be good. Should we do that?

I moved this code into a function called getRawOffset.

I really wanted to explore what this code would look like without element registration. It ends up being a lot cleaner. I also turned the displace method into one that returns the calculated offsets. Passing false bypasses recalculation of the offsets and just returns the cached values. So under this approach, a module only ever calls Drupal.displace() and/or registers for the drupalViewportOffsetChange event.

Under this approach, Toolbar lets displace calculate the width of the trays, but it explicitly sets the height of the bar.

What do you think? Displace is only being called when triggered by events like the overlay loading or the orientation of the toolbar tray changing. I'm ok with the relative inefficiency of going through the DOM to find the offsetting elements if it only happens on a trigger like resizing the screen. That isn't going to happen on small devices.

Setting to needs review to get some other folks to chime in.

This isn't a "needs review" for RTBC. I know there are lots of comment issues and cleanup, so don't go nuts with dreditor. This is a "needs review" for the approach. We're debating two approaches.

Displacing elements are marked with a data-offset-{edge} attribute and auto-discovered when an offset calculation is triggered

Pro
It's easy for module developers to mark an element as a displacing element.

Con
If displace is triggered often (like on window.onresize), this could be very inefficient.

Displacing elements are registered with displace.registerElements()

Pro
The DOM is scanned for displacing elements on each trigger of displace; this is better for performance.

Con
It is slightly more complicated for module developers to mark elements as displacing.

I like the cleanliness of the code and simpler DX in option 1, but option 2 seems like it will have a better performance profile. Looking for input from others.

Not getting

// If the displacement value is larger than the current value for this
// edge, use the displacement value.
offset[edge] = Math.max(offset[edge], displacement);

Why use the biggest number? an offset can't ever shrink ? shouldn't there be a += somewhere too?
calculateOffsets Will need to be profiled and optimized once the rest is RTBC-able.

The thinking here is this. If we have two elements on the left edge that both offset the edge of the viewport, we really only want to consider the largest/most-offset one rather than add their widths.

Why if (typeof displacement !== 'number' || isNaN(displacement)) and not just if (isNaN(displacement)) ?

I'm being overly cautious I guess! I'll reduce it to just isNaN and add some tests for funky values of data-offset-{edge}.

Also if you're using .on, change all the other .bind next to it or use .bind, consistency ++

I'll just use bind for now in files that have it. I don't want to go refactoring stuff like that in this patch.

The way Drupal.displace(); is used ought to be an event really.

I struggle with this one. Currently, calling Drupal.displace() does 2 things.

1. Recalculates the edge offsets
2. Triggers the drupalViewportOffsetChange event.

If a module like Toolbar were to trigger drupalViewportOffsetChange instead of invoke Drupal.displace(), the recalculation step would be skipped. I'm having trouble understanding how we'd get the offset information updated if modules were responsible for triggering the event and not the utitlity.

I'm mixed about getRawOffset, it means the person will add a data-offset-xxx with rubish values in it? I ought to be the responsibility of the person adding this value to make sure it's the one that should be used. See my patch above, makes things simpler.

Yes, a module could provide a value in data-offset-{edge} and that value would be used. The purpose of the getRawOffset</code. method is to provide a common means of calculating an offset in situations where the module doesn't need to. For instance, the toolbar tray left offset is just a simple box so it adds the <code>data-offset-left attribute and lets Drupal.displace calculate the offset. The toolbar bar is a different story because of its CSS. It need to calulate the top offset and provide that value to Drupal.displace.

The calculation of an offset for a position fixed element when the screen is scrolled and the element is not flush against the viewport edge is not trivial. Because of this, I thought it would be useful to provide this calculation as a utility of Drupal.displace where we can unit test it, rather than have modules figure it out individually.

Finally, I've gotten through the problems with Drupal.overlay.eventhandlerAlterDisplacedElements. This code was complex and I believe I've cut it down significantly without losing any behavior.

So, this is the patch that we start considering for commit. These are the improvements it introduces

1. Toolbar and Overlay now interact well. This was the primary goal of this issue. When a vertical tray is active, the Overlay responds by resizing to fit in the space left to the right/left of the tray.
2. Previously, the Overlay module and tableheader.js had independently computed their viewport offset values. Now, they leverage the new Drupal.displace utility. See also #787940: Generic approach for position:fixed elements like Toolbar, tableHeader, which this issue should resolve.
3. The method Drupal.overlay.eventhandlerAlterDisplacedElements now has 75% less code and is much more readable. And we rid ourselves of the last vestige of $.browser in Drupal core module code!
4. Overlay placement and sizing in RTL documents across various platforms/browser versions was not stellar in D7. This patch at least keeps parity with D7 or improves it slightly. Now with cleaner code and better viewport offset displacement, we can make further improvements. I'll put some screenshots at the end of this issue.

Changes of note in this patch

I put a window resize handler in Drupal.displace() in a behavior (and removed the resize handler from Toolbar). The reason I did this is because Toolbar and Overlay both need to respond to altered viewport displacing elements on resizes. I'd rather not have two resize handlers on window when we could just have one in a common place.

/**
 * Registers a resize hanlder on the window.
 */
Drupal.behaviors.drupalDisplace = {
  attach: function (context, settings) {
    // Do not process the window of the overlay.
    if (parent.Drupal.overlay && parent.Drupal.overlay.iframeWindow === window) {
      return;
    }
    // Mark this behavior as processed on the first pass.
    if (this.displaceProcessed) {
      return;
    }
    this.displaceProcessed = true;

    $(window).on('resize.drupalDisplace', Drupal.debounce(function (event) {
      displace(true);
    }, 200));
  }
};

Drupal.displace() calculates the offsets, broadcasts the updated values through the drupalViewportOffsetChange event and returns the offset values object to the calling function. The broadcast can be skipped by passing false to the method.

/**
 * Informs listeners of the current offset dimensions.
 *
 * @param Boolean broadcast
 *   - When true or undefined, causes the recalculated offsets values to be
 *   broadcast to listeners.
 */
function displace (broadcast) {
  offsets = calculateOffsets();
  if (typeof broadcast === 'undefined' || broadcast) {
    $(document).trigger('drupalViewportOffsetChange', offsets);
  }
  return offsets;
}

Also worth noting is that these changes feed directly into #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors. Once we get this committed, I'll work these changes into the updated Toolbar JavaScript for even more improvements and less code.

What we need now

More permutation testing

I've tried to test as many OS/browser combos as possible (in both LTR and RTL), comparing D7 to D8, to make sure that there are no regressions. In many cases, the broken D7 behavior continues without improvement in D8 because the machinations of overlay-parent.js and the styling of the overlay are just not going to result in a harmonious arrangement of elements in all permutations. We can't reliably know the width and placement of a scrollbar in all browsers on all platforms in all language directions. I spent more than 8 hours breaking my brain trying to prove this statement wrong and I couldn't solve it (not to say it's unsolvable). The better approach is to continue making design improvements to the Overlay presentation, specifically those being discussed in #1889150: Simplify overlay look, *only use for contextual operations* and http://groups.drupal.org/node/283223.

Code review

I'd love to get this issue committed this week so we can focus on further improvements that will build on this code.

Screenshots

What's up with the borked styling on the close tab you ask? Well, this is one of those great IE issues that you dream about. Watch this video to see how IE mixes up right and left for border radius in RTL languages: http://screencast.com/t/xXrZGYPNrr. There is no way to fix this except through IE-targeted CSS which we aren't doing in D8. I hope this resolves through styling changes that a proposed for Overlay that eliminate this hanging chad element. If not, we'll have to reconsider this again before release, but my prediction is that it'll resolve itself in due time.

This patch won't apply in Drupal 7.

I think the scrollbar is caused by the hanging close tab. The redesign of the overlay should fix this.

#1953374: Implement Seven style guide for core overlay

The changes in #75 look great. Thanks for putting that together nod_! I think we're ready to get this in.

One thing you changed in the patch from #70 is the comment format for functions. Here's an example.

-   * @param boolean broadcast
+   * @param {boolean} broadcast

According to http://drupal.org/node/172169, we should be using doxygen format for type descriptors of function params and returns. The doxygen format does not include {} around the type descriptor. I've seen both in our codebase. Personally I'd rather go without the brackets, but I don't have a good sense of what the docs parsing requires. Do you know if the brackets are necessary? If so, we'll want to update the JS documentation docs.

I'll put together the change notice and post that today.

Change notice was posted [#1956804].

Setting to fixed.

Hi pandaeskimo, do you see any other JavaScript errors on the page? Could you post the top style value of the overlay from the Inspector Tools?