toolbar layout error in IE

Ok good to go.

I'm not sure I understand what is going on in this patch. Why did we make the test more strict? Could someone briefly explain this extra test in 1 or 2 sentences?

In the code comments, shouldn't 'remove' be 'subtract'? I guess either works.

2 maintainers are basically telling you the comment needs work, so please attempt to make the comment read more clearly. The problem is that it's very vague. If we don't need this for IE9, writing "IE8 and below" would be better and it's not clear how much height is being added, and whether or not this is a known bug or something.

BTW, you are not supposed to mark your own patch RTBC, especially after Dries and webchick say it needs work and you haven't changed anything.

Pre Patch:
I installed the latest D8 dev today and observed that IE9 32bit and 64bit both scrunched a bit under the toolbar when scrolling the module list. Firefox and Chrome did not scrunch under the toolbar.

Post Patch:
I installed toolbar_0.patch and it worked. The scrunching disappeared and IE9 behaved the same when scrolling the module list as Firefox and Chrome.

Nice improvement.

Okay, so as for a non-IE test of the patch goes, it didn't mess with the toolbar. :) Checked in Firefox, Safari, and Chrome.

Before

After

Thanks @ryanissamson and @Dig1. So we've covered IE 9 and non-IE; we should stil test IE 6-8. a_thakur started to test IE 8 but wasn't able to reproduce the bug.

That's normal :) It's an IE9 only fix, with IE8 the original (non buggy) behavior is present.

As long as it works on IE9 and doesn't break any other browser, it's fixed. Thanks for testing :)

Well, the patch still needs a little cleanup. ;) The comment should be shortened to be a little more clear, I think, and should wrap at 80 characters. Minor thing though.

Zgear and I are double-checking IE6, 7, and 8 now.

So, wow, toolbar has a few interesting bugs in IE6. That said, this patch doesn't make them any worse. IE7 is also fine.

Same JS, with comments that are hopefully a little more clear.

Further testing:

- Comments added in #17 toolbar-1199774-17.patch are a better description of the fix. I can now understand the cause of the original problem and the proposed fix.

- When toolbar-1199774-17.patch is applied to Drupal core 8.x-dev (2012-Feb-23) then cache cleared and cron run...

1) It works when overlay is uninstalled.
2) It does NOT work when overlay is installed.

So the overlay, which is what most people will see and use, does not seem to recognise the patch at the moment.

Any ideas?

@Dig1, just checked, yes that's a overlay bug. Now it's in my backyard :D

One might ague that it'll need to be refactored in a function and I'd agree but overloading drupal.js with that might not be the best thing ever.

Anyway, can you try this out please?

tested in IE 7 and the toolbar was pretty bugged either way, also tested in IE 8 and toolbar was fine. Tested in Safari and chrome to make sure and both showed no difference.

@xjm you sure it was IE 6 that was bugged? I used IE 7 and it was pretty bugged, although I don't have IE 6 so I wouldn't know. On another note I thought drupal 8 wasn't going to support IE 6 anymore, could I be wrong about that as well?

Yep, IE6 and IE7 aren't supported in D8, but they still are in D7 unfortunately.

So I didn't actually need to test for IE 7? cool :)

@Zgear -- Well, we do need to test both IE6 and IE7 for the backport, so we might as well do so now to make sure the same patch will work for backport. (Also, that allows the D7 maintainer to commit the patch to D7 and D8 if appropriate.) :)

To clarify, the issues in IE6/7 had to do with bugs like form elements showing through the toolbar, etc., and are unrelated to the patch.

Edit: @nod, is the new patch supposed to resolve the bug Dig1 describes?

yes

Cool, let's quickly retest it then. Test that #19:

• Resolves the issue in IE 9 without the overlay.
• Resolves the issue in IE 9 with the overlay.
• Does not cause any regressions with or without overlay in IE6-8, webkit, FF.

Would be cool if Zgear, ryanissamson, or Dig1 want to test it since they already know what to look for. (Otherwise, we'll get it in COH in the coming week.) Thanks!

Also, I'm a JS idiot so forgive if this is a dumb question--but what's with all the added vars?

Okay, tested #19 and found issues in neither Webkit nor FF. :) And sorry about the different sized screenshots - my true newbness shining through.

Webkit

Before

After

FF

Before

After

@xjm: Drupal.toolbar.height is actually very performance sensitive because it's called by tableheader on each page scroll (and possibly several times on each scroll too).

CSS is also an expensive thing to get because it's DOM related and anything DOM, styles especially are expensive to get/set.
Caching $toolbar is just common sense on top of being more efficient.
The boxShadow boolean is to keep the if readable.

and now that I read it again, .match is not a good idea to use here.

Okay here are the results after separately reinstalling the latest D8 dev and testing both:
core-js-toolbar-overlay-boxshadow-1199774-19.patch
core-js-toolbar-overlay-boxshadow-1199774-28.patch

IE9 toolbar height before Patch
- Problem when overlay installed.
- Problem when overlay not installed.

Apply Patch
I got two lines responding in the terminal:
patching file core/modules/overlay/overlay-parent.js
patching file core/modules/toolbar/toolbar.js

IE9 toolbar height after Patch
- Problem when overlay installed.
- NO problem when overlay not installed.

So it would seem that neither patch has managed to resolve the IE9 toolbar height problem when the overlay is installed...

Here's hoping :)

Have you cleared you browser caches? Since overlay is in an iframe it's not enough to just clear the landing page cache.

Thanks for testing this :)

oops...I cleared the server caches but I did not clear the browser caches...let me do it again

After clearing browser cache and reinstalling the latest D8 dev the good news is:

IE9 toolbar height after applying #30 Patch core-js-toolbar-overlay-boxshadow-1199774-28.patch
- NO Problem when overlay installed.
- NO problem when overlay not installed.

Great job.

Awesome, thanks for testing that one more time :)

back to RTBC, #4 rtbc it, code and comments got moved around a bit without changing the way it works. Dig1 did the testing on IE9 in #34, no regressions on other browsers.

OK. Committed/pushed to 8.x, moving to 7.x for backport.

and that should take care of #1477236: Loading Drupal pages in a hidden iframe throws the JavaScript error in Firefox as well.

Backport looks fine.

Marked #1477236: Loading Drupal pages in a hidden iframe throws the JavaScript error in Firefox as a duplicate of this issue.

Committed and pushed to 7.x! Thanks, folks! :) And thanks, Ryan, for the manual testing!

It's awesome that this got fixed for D7. However, based on discussions in #1465948: [meta] Drop some IE8 coddling from Drupal core and #1507960: [meta] Isolate and/or remove IE-specific hacks in core markup, CSS and JavaScript, I think it's a mistake to continue this kind of stuff in D8. Therefore, #1651270: Remove IE8-specific workaround for lack of CSS3 box-shadow support .

Updated issue summary.