WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 20 months ago

#22044 closed defect (bug) (fixed)

Twenty Twelve: better support for IE7 and IE8 (don't use mobile menu)

Reported by: bpetty Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

The issue of IE7/8 using the mobile version of the menu (see #21440) is caused by the stylesheet defining mobile styles as the default, and overriding those in media queries for desktop screen sizes, but since IE7/8 don't support media queries, they only use the mobile styles.

Having discussed this in IRC with lancewillet and mattwiebe for a bit, there's some that believe that "mobile first" design (used for Twenty Twelve) forces the stylesheet to define mobile screen styles as the default non-media-query-supporting styles before defining desktop screen styles within media queries.

Even though I still don't believe that is true, it's really too late to change that now anyway with Twenty Twelve now up on Extend. So instead, I'm proposing that this can still be fixed without changing the stylesheet at all by using this hack which conditionally includes a JS library that adds fake support for media queries for browsers that don't support it:

I think this was a huge oversight to assume that it would just be fine to show the mobile version of the menu on IE7/8, and we already had bug reports coming in before Beta 1 was even released. I know that with over 30% of all desktop users still using IE7/8, this will be an ongoing problem with duplicate reports constantly coming in if it is not fixed.

I have tested this on IE8, and it does indeed fix the menu, and seems to work well. As an added bonus, IE7 and IE8 are also now actually responsive too.

Attachments (25)

22044.patch (17.3 KB) - added by bpetty 22 months ago.
22044.ie7.png (15.4 KB) - added by SergeyBiryukov 22 months ago.
22044.ie8.png (15.3 KB) - added by SergeyBiryukov 22 months ago.
22044.2.patch (4.9 KB) - added by SergeyBiryukov 22 months ago.
22044.ie8-unpatched.png (792.4 KB) - added by SergeyBiryukov 22 months ago.
22044.ie8-unpatched-real.png (687.6 KB) - added by bpetty 22 months ago.
22044.ie8-unpatched-svn-trunk.png (65.5 KB) - added by bpetty 22 months ago.
2012-wordpress.com-IE8-without-compatibility-mode.png (172.5 KB) - added by Ov3rfly 22 months ago.
2012-wordpress.com-IE8-with-compatibility-mode.png (182.2 KB) - added by Ov3rfly 22 months ago.
22044.3.diff (17.9 KB) - added by DrewAPicture 22 months ago.
IE7/8 nav fix + CSS3 MediaQueries
22044.3-IE7.jpg (40.7 KB) - added by DrewAPicture 22 months ago.
patched nav IE7
22044.3.-IE8.jpg (42.9 KB) - added by DrewAPicture 22 months ago.
patched nav IE8
22044.4.patch (18.8 KB) - added by SergeyBiryukov 22 months ago.
22044.ie7-overlapping.png (4.2 KB) - added by SergeyBiryukov 22 months ago.
trunk.werdswords.com-ie7-submenu-items-behind-parent-menu-items.png (6.6 KB) - added by Ov3rfly 22 months ago.
22044.css-only.diff (7.3 KB) - added by obenland 22 months ago.
22044.css-only-ie7-love.diff (6.9 KB) - added by mindctrl 22 months ago.
Add IE7 support to @obenland's patch. Needs IE8/9 testing.
22044.5.patch (5.7 KB) - added by SergeyBiryukov 22 months ago.
22044.rtl.firefox.png (33.0 KB) - added by SergeyBiryukov 22 months ago.
22044.rtl.ie7.png (32.3 KB) - added by SergeyBiryukov 22 months ago.
22044.rtl.ie8.png (32.9 KB) - added by SergeyBiryukov 22 months ago.
22044.6.patch (10.9 KB) - added by bpetty 21 months ago.
22044.7-rtl.diff (542 bytes) - added by DrewAPicture 20 months ago.
IE nav RTL first pass
22044.8-rtl.diff (1.1 KB) - added by DrewAPicture 20 months ago.
22044.9-rtl.diff (1.5 KB) - added by lancewillett 20 months ago.
A few more RTL IE7,8 fixes; move styles to bottom and a typo fix

Change History (87)

bpetty22 months ago

comment:1 nacin22 months ago

I didn't know that Twenty Twelve did not support IE8 so oddly. Google may be dropping support for IE8, but we have not even dropped IE7. This kind of degradation is fine in IE7, but 8? It's curious.

SergeyBiryukov22 months ago

SergeyBiryukov22 months ago

comment:2 SergeyBiryukov22 months ago

Tested the patch, works as expected in IE 8: 22044.ie8.png.

In IE 7, however, the menu is displayed vertically: 22044.ie7.png.

comment:3 SergeyBiryukov22 months ago

In IE 7, however, the menu is displayed vertically

Seems to be a CSS problem. Respond.js (mentioned in ticket:21440:4 and in the IRC chat as another potential solution) gives the same results.

SergeyBiryukov22 months ago

comment:4 SergeyBiryukov22 months ago

22044.2.patch loads Respond.js for IE 8 only, as suggested by DrewAPicture.

It needs to be loaded after the CSS, hence the placement after wp_head().

comment:5 bpetty22 months ago

Thanks @SergeyBiryukov, that looks better.

comment:6 follow-ups: obenland22 months ago

  • Cc lancewillett drewstrojny added
  • Severity changed from major to normal

This was previously discussed extensively on Trac (#21440, #21489, #21955) and IRC (today), so this is really not a huge oversight.

And as @lancewillett pointed out in Why Default Themes Change Each Year:

"[...] the default theme isn’t trying to be an end-all-be-all theme. It won’t please everyone."

Duplicate of #21498, #21955.

comment:7 in reply to: ↑ 6 bpetty22 months ago

Replying to obenland:

This was previously discussed extensively on Trac (#21440, #21489, #21955) and IRC (today), so this is really not a huge oversight.

#21440 does not address the fact that this is a problem with over 30% of desktop browsers, and it's a serious degradation that makes site navigation incredibly difficult for most sites that will use this theme. This ticket only addressed making sure the theme actually worked in IE7 and IE8, not that it worked well.

#21489 contains zero discussion, it was just marked as duplicate, but it's definitely relevant since it's another duplicate bug report from another frustrated user before the first beta was even released.

#21955 just says "works as intended" with no explanation beyond that before closing (not even to answer the poor guy's question of "How do you show normal navigation such as other browsers?" - which #21440 does have a solution, but it's not one any non-developer will have any clue how to apply).

Also, just for reference, I was involved in the IRC discussion linked, so yes, I'm aware of that as well.

And as @lancewillett pointed out in Why Default Themes Change Each Year:

"[...] the default theme isn’t trying to be an end-all-be-all theme. It won’t please everyone."

"Supporting the same browsers that even the version of WordPress this theme is going to ship with supports" does not fall into this category of features the default theme shouldn't be forced to bother with. It's a bug. This has nothing to do with anything Lance is explaining in that post.

This is something we will see a massive number of complaints on, and it will be a dark spot in history for the Twenty Twelve theme (and WordPress itself) unless it's fixed. This is confirmed by the fact that on the first day of release on Extend, there's already five different users complaining about it on the support forums: http://wordpress.org/support/topic/theme-twenty-twelve-nav-bar-fails-in-ie8

Yes, users could fix it themselves (one user on that forum thread did so with a child theme), but most users won't have a clue how to.

I haven't heard a single person say that the mobile menu is "better" for IE8, the only thing I've heard is "we shouldn't care". I'm saying we have a good reason to care.

"Best practices in my opinion, shouldn't leave such a great percent of users behind."

comment:8 in reply to: ↑ 6 ; follow-up: Ov3rfly22 months ago

Replying to obenland:

This was previously discussed extensively on Trac ... and IRC ..., so this is really not a huge oversight.

And as @lancewillett pointed out in Why Default Themes Change Each Year:

"[...] the default theme isn’t trying to be an end-all-be-all theme. It won’t please everyone."

My two cents:

1.) Obviously nobody really tested Twenty Twelve on "real life pc-systems" at all. Only one week before 1.0 release I downloaded 0.9 and spotted clearly visible layout problems after a 5 minutes test in Firefox and Chrome, didn't do any tests in IE, otherwise there would have been more bugs in tracker...

5 Minutes test results: #21964, #21965

The IE disaster now confirms this assumption.

2.) A responsive design should be written completely the other way round. Default look should be available without media queries, with maybe some conditionals for older browsers (which also can be added by third party). "Narrow" look should be done with media queries.

These two major problems make the really nice looking Twenty Twelve (props to the designer) in the current version 1.0 unfortunately a total failure which will also give WordPress itself a bad name.

Many big companies are "lightyears" away from migration to Windows 7 and run mostly XP on their desktops, maybe even with IE7. My various stats have over 50% IE7/IE8. This is not a problem with Twenty Ten and Twenty Eleven (and the child themes which are based on them). But all these users are basically kicked out with current Twenty Twelve if it ships like this as default with next WordPress.

Note: I have also posted this rant here for further discussion.

comment:9 in reply to: ↑ 8 nacin22 months ago

Replying to Ov3rfly:

a total failure which will also give WordPress itself a bad name

No, it's not, and no, it won't. Hyperbole won't get you anywhere.

comment:10 follow-up: mindctrl22 months ago

The menus work in IE 8 using respond.js in the attached patch, but respond.js doesn't work with CSS referenced via @import. This would present problems in child themes that use the @import rule, as shown in the examples in the Codex.

comment:11 in reply to: ↑ 10 ; follow-up: bpetty22 months ago

Replying to mindctrl:

The menus work in IE 8 using respond.js in the attached patch, but respond.js doesn't work with CSS referenced via @import. This would present problems in child themes that use the @import rule, as shown in the examples in the Codex.

That's true, but it really is too late to go back and re-organize the styles to work the opposite way (of using desktop styles first, and media queries for mobile) even if the original designers wanted to (which they still don't, despite this problem). So this is pretty much the only solution that can still be applied to fix this.

However, child themes don't ever actually need to use @import in their CSS (and are actually encouraged not to regardless), so that's not a big concern.

comment:12 in reply to: ↑ 11 Ov3rfly22 months ago

Replying to bpetty:

However, child themes don't ever actually need to use @import in their CSS (and are actually encouraged not to regardless), so that's not a big concern.

Isn't the very first line in every child theme CSS this (see also Codex):

@import url("../parent-theme/style.css");
Last edited 22 months ago by Ov3rfly (previous) (diff)

comment:13 follow-up: bpetty22 months ago

Sorry, I guess that advice didn't make it into the codex afterall. Most general web development advice recommends against it (non-WordPress related), and I've actually seen a few WordCamp presentations (usually on optimization) that advise you to take the file, and compress/minify with your custom CSS to toss in your child theme if you have to use custom CSS rules.

I guess that can be a little difficult to explain in simple terms on the Codex.

Anyway, the point is that you can still do anything and everything @import does without actually using @import inside child themes.

You could also just manually add the parent theme CSS file to a custom header you define in your child theme without using @import.

Last edited 22 months ago by bpetty (previous) (diff)

comment:14 in reply to: ↑ 13 Ov3rfly22 months ago

Replying to bpetty:

Anyway, the point is that you can still do anything and everything @import does without actually using @import inside child themes.

All child themes I ever saw use @import for parent style.css as explained in Codex.

That's why the use of respond.js is no solution to the IE7/8 menu problem.

It's just a bad hack which will cause even more bad hacks, as writing real-world compatible child themes for Twenty Twelve will not be possible and everybody will have to directly edit the main Twenty Twelve to include their styles and code. This will cause a lot of confusion and probably messed up and buggy themes with bad user experience.

There is a reason for this bold sentence in Codex:

..child themes are the recommended way of making modifications to a theme.

I still stand to my "Hyperbole", the current release of Twenty Twelve should be withdrawn and the menu be rewritten to work also in IE or it will hurt WordPress in the long run...

comment:15 follow-ups: nacin22 months ago

There appears to be a way to force Internet Explorer that can't understand media queries to apply the rules inside of them, with @media screen, all and (min-width: 300px) {.

This is from http://dev.opera.com/articles/view/safe-media-queries/, via http://coding.smashingmagazine.com/2011/08/10/techniques-for-gracefully-degrading-media-queries/. However, it seems that may not work in IE8, but only IE6 and IE7. Also in my search, I found https://github.com/h5bp/html5-boilerplate/issues/816 (via http://jsfiddle.net/boblet/59e99/), which is a pretty strong indictment of respond.js, and is a good read that covers a number of facets of the mobile-first versus desktop-first debate.

I don't know what the best solution is here. But, saying Twenty Twelve "isn’t trying to be an end-all-be-all theme," it is a straw man. The suggestion here isn't that Twenty Twelve should be such. The suggestion is that it should support IE8 better than it does.

We had a long and determined conversation some time ago when the WordPress project agreed to drop IE6. That didn't happen here, and given IE8's continued browser share, the suggestion holds a lot of merit.

Can someone upload a screenshot of an unpatched Twenty Twelve running IE8? Is anything other than the menu significantly affected in any adverse way?

(A side note, when looking through the stylesheet, is there a reason the background default color is only applied when the device width is 960px? The custom background code won't do the same for any other color; this declaration should be pulled out of @media.)

comment:16 in reply to: ↑ 15 ; follow-up: SergeyBiryukov22 months ago

Replying to nacin:

Can someone upload a screenshot of an unpatched Twenty Twelve running IE8?

22044.ie8-unpatched.png.

Is anything other than the menu significantly affected in any adverse way?

Didn't see any other outstanding issues so far.

comment:17 bpetty22 months ago

Replying to Ov3rfly:

Replying to bpetty:

Anyway, the point is that you can still do anything and everything @import does without actually using @import inside child themes.

All child themes I ever saw use @import for parent style.css as explained in Codex.

That's why the use of respond.js is no solution to the IE7/8 menu problem.

It's a new theme, not an upgrade to Twenty Eleven, so there are almost zero child themes for this change to even break. There might be a few child themes created in the last couple days based on a download from the theme repository, and even then, adding this patch won't actually break any of them that aren't already broken, it just might not actually fix this bug if they are using @import. It won't break @import though.

I agree that it's a hack, and I don't like it anymore than you do. As I pointed out when I opened the ticket, this is a last resort since the appropriate fix (that you agree is the appropriate fix) of re-organizing the media query styles is impossible at this point. It would require a new round of testing past the time it would take to actually get it re-organized that would have to push it back to being released with WordPress 3.6. That's not going to happen.

There is nothing else we can do at this point except make use of the Respond.js hack to get this fixed for IE8. It's either that, or nothing.

It's just a bad hack which will cause even more bad hacks, as writing real-world compatible child themes for Twenty Twelve will not be possible and everybody will have to directly edit the main Twenty Twelve to include their styles and code. This will cause a lot of confusion and probably messed up and buggy themes with bad user experience.

No, no-one will have to touch Twenty Twelve at all. The addition of Respond.js won't change that at all, and yes, even with this, the recommended way of making modifications to a theme is still to use a child theme.

comment:18 in reply to: ↑ 16 Ov3rfly22 months ago

Replying to SergeyBiryukov:

Replying to nacin:

Can someone upload a screenshot of an unpatched Twenty Twelve running IE8?

22044.ie8-unpatched.png.

Beware, the screenshot of http://twentytwelvedemo.wordpress.com/ site is not a default fresh install. The html output is different to real Twenty Twelve, there are many changes for wordpress.com

Last edited 22 months ago by Ov3rfly (previous) (diff)

comment:19 follow-up: bpetty22 months ago

Replying to SergeyBiryukov:

Replying to nacin:

Can someone upload a screenshot of an unpatched Twenty Twelve running IE8?

22044.ie8-unpatched.png.

Actually, I get something with minor differences using real, native IE8 on Windows XP:

22044.ie8-unpatched-real.png

Some of that sidebar stuff could just be actual content messing with layout... not sure.

Anyway, I've done this with a fresh SVN trunk *clean* install, and see exactly the same thing.

comment:20 bpetty22 months ago

Well, maybe a little different now:

22044.ie8-unpatched-svn-trunk.png

comment:21 in reply to: ↑ 15 Ov3rfly22 months ago

Replying to nacin:

(A side note, when looking through the stylesheet, is there a reason the background default color is only applied when the device width is 960px? The custom background code won't do the same for any other color; this declaration should be pulled out of @media.)

(Also a side note, see #22030 for further problems with custom background color. It seems to me that this feature is only "half way" implemented and was never really tested.)

comment:22 in reply to: ↑ 19 SergeyBiryukov22 months ago

Replying to bpetty:

Actually, I get something with minor differences using real, native IE8 on Windows XP

To be clear, 22044.ie8-unpatched.png was also taken using native IE8 on Windows XP. The settings turned out to be:

  • Browser Mode: IE8 Compat View
  • Document Mode: IE7 Standards

With the following settings, I can reproduce 22044.ie8-unpatched-real.png:

  • Browser Mode: IE8
  • Document Mode: IE8 Standards

On a clean install of 3.5-beta1 I see the same as in 22044.ie8-unpatched-svn-trunk.png with the latter settings.

comment:23 Ov3rfly22 months ago

Looking at the above attached wordpress.com version with and without compatibility mode in IE8 speaks for itself.

Will try to find time to add some test-content to a fresh clean Twenty Twelve 1.0 official download within next days and provide screenshots then.

comment:24 bpetty22 months ago

Ah, thanks @SergeyBiryukov, I didn't remember to check that.

DrewAPicture22 months ago

IE7/8 nav fix + CSS3 MediaQueries

DrewAPicture22 months ago

patched nav IE7

DrewAPicture22 months ago

patched nav IE8

comment:25 follow-up: DrewAPicture22 months ago

  • Cc xoodrew@… added

Attached 22044.3.diff which puts the menus in line for me in IE7/8 using a combination of CSS hacks for IE7 and CSS3 Media Queries from 22044.patch. Looks and acts pretty close to Firefox.

Also attached screenshots of the patched nav menus on hover from IE7/8. Please test.

Last edited 22 months ago by DrewAPicture (previous) (diff)

SergeyBiryukov22 months ago

comment:26 SergeyBiryukov22 months ago

22044.4.patch is an attempt to introduce and use .ie7 class instead of CSS hacks.

Found one more issue (not addressed in the patch). In IE 7, submenus can be inaccessible due to overlapping with the second line of menu items: 22044.ie7-overlapping.png.

comment:27 in reply to: ↑ 25 Ov3rfly22 months ago

Replying to DrewAPicture:

Thanks a lot for your for all your efforts!

Also attached screenshots of the patched nav menus on hover from IE7/8. Please test.

Confirming the overlapping. Seems the submenu is opened behind the parent items.

comment:28 mindctrl22 months ago

I think the theme should include a line (shown below) in header.php telling IE to use its latest rendering engine. I've already seen three separate IE9 installs showing the default Twenty Twelve in Compatibility View (which breaks the menus), and on http://wordpress.org/support/topic/theme-twenty-twelve-nav-bar-fails-in-ie8" there have been two reports of it so far. In all of my tests, adding this single line fixed the IE9 problem.

<!-- [if IE]> <meta http-equiv="X-UA-Compatible" content="IE=Edge"/> <! [endif]-->

Last edited 22 months ago by mindctrl (previous) (diff)

comment:29 lancewillett22 months ago

There are many problems with this Trac ticket:

  1. It's a duplicate. See notes from Obenland above.
  2. The IE8 navigation menu is not broken. It's just different than what modern browsers display. http://dowebsitesneedtolookexactlythesameineverybrowser.com/
  3. The IE8 navigation menu behavior is not a mistake or oversight. We, the team tasked with creating this theme, decided to give IE7 and IE8 the basic view of the theme, equivalent of the mobile view. We agonized, tested, discussed in IRC and Trac, and went with the best pragmatic approach to keep the theme lean and clean.
  4. It's not accurate to state that IE8 is in use on 30% of websites — that kind of blanket statement is hard to back up with hard stats. It depends on your site, your audience. If you check the global numbers (http://en.wikipedia.org/wiki/Browser_usage) you'll see it varies greatly — but IE itself could be around 30% with and IE8 and 7 are a smaller percentage of that.
  5. I doubt that we will see massive complaints. This theme has been live on WordPress.com for over a month, and is now active on hundreds of thousands of sites. Smooth sailing there. Again, it depends on your exact audience and site.
  6. This theme is usable and ready for IE users. We tested it extensively in IE versions as part of the theme breaking — especially at WCSF 2012 and after it hit WP.com and Extend. That's not the same as saying it's pixel perfect in older IEs, see number 2 above.

That said, if we can focus on facts and a pragmatic approach I'm open to re-looking at the best and most logical solution to making the navigation work more like modern browsers.

Whether that's loading a JS script to enable media query support in IE7 and IE8 or a CSS hack on the .ie selectors we have in place.

Last edited 22 months ago by lancewillett (previous) (diff)

obenland22 months ago

comment:30 follow-up: obenland22 months ago

  • Keywords needs-testing added

22044.css-only.diff moves all IE-specific styles, and copies styles wrapped in media-queries, to a separate IE-file.

@bpetty, @DrewAPicture, @SergeyBiryukov, @Ov3rfly, @mindctrl: Any testing help would be greatly appreciated!

Last edited 22 months ago by obenland (previous) (diff)

comment:31 obenland22 months ago

Let me add: #18753 would come in real handy here ;)

comment:32 in reply to: ↑ 30 Ov3rfly22 months ago

Replying to obenland:

22044.css-only.diff moves all IE-specific styles, and copies styles wrapped in media-queries, to a separate IE-file.

Works perfect in IE8! Thank you very much for your valuable contribution.

If manually forced into compatibility view, the menu breaks and looks like with IE7 (left-aligned vertical list of top items).

Any chance that you also look into IE7 css? As an extra a compatible IE7 css could be a fallback for IE8/IE9 in compatibility view.

comment:33 mor1022 months ago

On the reasoning why IE8 support matters, keep in mind that while most North Americans and Western Europeans have moved on from IE8, much of the rest of the world is stuck with the older browsers for economical reasons. The upgrade from XP to Windows 7 (and soon Windows 8) is prohibitively expensive for many individuals and companies and for this reason they are stuck with IE8. Because WordPress is a world wide application with a world wide reach, the world wide end-user should be provided with as good of an experience as possible.

The main issue here is the decision to make the mobile menu default and placing the 'standard' menu in media queries. This makes sense if you take a mobile-first approach, but it is non-standard and against best practice scenarios. When advanced features like this are included in a theme, they should be added as progressive enhancements, i.e. the mobile menu should be added for browsers that can handle it while the 'regular' menu is served up for the rest.

Twenty Twelve is a great theme and definitely a step in the right direction when it comes to making the shipped themes bases for further development. However, the menu approach is sub-optimal and I would urge that in future development this issue be avoided. Though IE bashing is fun, it is not our job to police the browser world.

Last edited 22 months ago by mor10 (previous) (diff)

comment:34 emiluzelac22 months ago

  • Cc emil@… added

comment:35 bpetty22 months ago

For what it's worth, there is a thread about this problem on the WP.com support forums too that was started back on Sept 18th:
http://en.forums.wordpress.com/topic/twenty-twelve-menu-problem

comment:36 follow-up: lancewillett22 months ago

Thanks @obenland, 22044.css-only.diff is looking good, a few notes:

  • Why remove the .ie7 .ie8 hooks? I think those are still useful.
  • Did you mean to bump the theme to 1.1? :)

With the amount of style fixes I guess it makes sense to move to a new file.

comment:37 in reply to: ↑ 36 ; follow-ups: obenland22 months ago

Replying to lancewillett:

  • Why remove the .ie7 .ie8 hooks? I think those are still useful.

With the current conditional (both IE 7 & 8), they don't really add a benefit since we can overwrite any style in ie.css. I'd agree with you, if we'd split them up like @SergeyBiryukov proposed in 22044.4.patch. Then we could use the classes in ie.css, to target the browsers individually. But as long as it is not necessary, why keep them?


  • Did you mean to bump the theme to 1.1? :)

Yes, but not with this patch. I was planning to ask you how this is usually handled (ticket yes/no, point in time etc).

comment:38 in reply to: ↑ 37 lancewillett22 months ago

Replying to obenland:

Replying to lancewillett:

  • Why remove the .ie7 .ie8 hooks? I think those are still useful.

But as long as it is not necessary, why keep them?

Because themers might want to target a specific IE version in their child theme or fork. And it shipped with them, so removing now could break things.

  • Did you mean to bump the theme to 1.1? :)

Yes, but not with this patch. I was planning to ask you how this is usually handled (ticket yes/no, point in time etc).

Nacin usually handles that when pushing out new WP beta or RC or stable releases. Let's leave it out and let him bump it when the time comes. I think 1.1 will be bumped with the first 3.5 RC1 (or maybe the stable).

comment:39 in reply to: ↑ 37 lancewillett22 months ago

Replying to obenland:

With the current conditional (both IE 7 & 8), they don't really add a benefit since we can overwrite any style in ie.css. I'd agree with you, if we'd split them up like @SergeyBiryukov proposed in 22044.4.patch. Then we could use the classes in ie.css, to target the browsers individually. But as long as it is not necessary, why keep them?

@obenland I think keeping the specific class values is useful, and can be used in ie.css for things like the IE7 clip property.

comment:40 mindctrl22 months ago

Err, that patch didn't pick up the CSS. Sorry about that. I'll try another one.

mindctrl22 months ago

Add IE7 support to @obenland's patch. Needs IE8/9 testing.

comment:41 mindctrl22 months ago

In case it's not clear, this is what I added to @obenland's patch.

.main-navigation li a,
.main-navigation li {
	display: inline-block;
	*display: inline !important;
	*zoom: 1;
}

.main-navigation > div > ul > li:hover > ul {
	left: 0;
}

comment:42 emiluzelac22 months ago

*property: value is not recommended and this applies only to IE7 and bellow and it's ugly :)
What happened with good old conditional tags?

.ie7 .main-navigation li a,
.ie7 .main-navigation li {
    display: inline;
}

is much cleaner "hack free" approach.

comment:43 obenland22 months ago

@emiluzelac is right, let's add @SergeyBiryukov's IE7/8 switch to the patch, as noted by @lancewillett, and use the browser-specific classes.

Last edited 22 months ago by obenland (previous) (diff)

SergeyBiryukov22 months ago

comment:44 SergeyBiryukov22 months ago

22044.5.patch combines 22044.css-only.diff with the separate .ie7/.ie8 classes and IE7 fixes from 22044.4.patch.

  1. The overlapping issue in IE7 (comment:26) still needs to be resolved.
  2. RTL needs more work, see the differences between 22044.rtl.firefox.png, 22044.rtl.ie7.png, and 22044.rtl.ie8.png:
    • In IE7/8, the menu is left-aligned.
    • The submenus are incorrectly positioned. The page hierarchy on the screenshots is: page-15665 > qILy1-k0st8 > test. 22044.rtl.firefox.png shows the correct positioning.
    • It's also weird that sidebar is on the right in IE7/8. It should be on the left for RTL.
Last edited 21 months ago by SergeyBiryukov (previous) (diff)

comment:45 lancewillett21 months ago

Discussed a bunch in IRC #wordpress-dev today (log). Going to proceed with 22044.5.patch noting the things it does not yet fix (IE7 menu overlap, which is also broken in Twenty Ten and Eleven; RTL issues).

This gets IE8 the full menu experience; and we can continue working on the other issues separately.

comment:46 lancewillett21 months ago

  • Keywords needs-testing removed
  • Summary changed from Twenty Twelve: Support Media Queries on IE7/8 (don't use mobile menu) to Twenty Twelve: better support for IE7 and IE8 (don't use mobile menu)

comment:47 lancewillett21 months ago

In [22201]:

Twenty Twelve: implement better support for IE7 and IE8 (don't use mobile menu). See #22044.

comment:48 follow-up: lancewillett21 months ago

  • Keywords needs-patch needs-testing rtl-feedback added; has-patch removed

Next steps:

  1. Work more on RTL support
  2. Try again for IE7 menu overlap issue

Both are detailed above in http://core.trac.wordpress.org/ticket/22044#comment:44 by Sergey.

bpetty21 months ago

comment:49 bpetty21 months ago

Having just done a bit more research into this, there's another possibly acceptable approach we could use. In the book "Head First Mobile Web", this issue is addressed by breaking out the full desktop size styles into it's own stylesheet, and using the "media" attribute on the link tag which is used by all modern browsers, and a conditional for IE7/8 on the same file without the media attribute since the one with is ignored by IE7/8.

I've implemented this in 22044.6.patch, but also note that I did this from code before r22201, so it's still missing the other IE7-specific style fixes handled earlier on this ticket.

The upside to this approach is that it keeps the approach of mobile styles first, but the downside is that any responsive styles defined for screens between the smallest and largest still need to be duplicated in the desktop.css file just like they are in ie.css in r22201.

comment:50 bpetty21 months ago

  • Cc bpetty added

comment:51 lancewillett21 months ago

@bpetty Thanks for the notes and patches. It's too late to rework the stylesheets -- but something to keep in mind for the future default themes.

comment:52 in reply to: ↑ 48 lancewillett20 months ago

Replying to lancewillett:

Final steps, to close this ticket

  1. Work more on RTL support

Let's do a final testing pass at RTL in IE7 and IE8, fix up anything broken.

  1. Try again for IE7 menu overlap issue

Since this is broken also in Twenty Ten and Eleven, I vote to leave as is.

comment:53 DrewAPicture20 months ago

Making a note to check why text-align right on the nav ul isn't applying properly in IE7 RTL.

DrewAPicture20 months ago

IE nav RTL first pass

comment:54 DrewAPicture20 months ago

22044.7-rtl.diff is a first pass at RTL fixes for the nav. Seems like IE7/8 don't play nice with inheriting .main-navigation ul li more than one level deep without explicitly defining each level.

comment:55 DrewAPicture20 months ago

OK, 22044.7-rtl.diff is rubbish. I realized if we want to make it true to the others the sub-sub menus need to be on the left edge of the sub menus. We can do this with positioning I guess but it seems a little overkill. The other option of course, is to allow the sub-sub menu to open to the right just as happens in LTR.

Thoughts?

comment:56 lancewillett20 months ago

I think to solve the RTL + IE overrides we'll need a new file, rtl-ie.css that gets enqueued for IE browsers when is_rtl() is true (using same conditional logic as ie.css).

Otherwise the IE styles will trump the RTL fixes in rtl.css.

comment:57 follow-up: nacin20 months ago

WordPress has an .rtl body class, so I'd just toss the few IE RTL rules you have directly into ie.css with that extra selector.

comment:58 in reply to: ↑ 57 lancewillett20 months ago

Replying to nacin:

WordPress has an .rtl body class, so I'd just toss the few IE RTL rules you have directly into ie.css with that extra selector.

Great point, I'd forgotten about the body class. @DrewAPicture how's your patch coming?

DrewAPicture20 months ago

comment:59 follow-up: DrewAPicture20 months ago

  • Keywords has-patch added; needs-patch removed

22044.8-rtl.diff Handles the IE7/8 submenu alignment problems, incorporating one declaration from 22044.7-rtl.diff.

Still having issues with the submenu z-indexes in IE7. I tried the trick of setting the first level to z-index: 99 and lower levels to z-index: 1, but no dice. I'm stumped.

comment:60 in reply to: ↑ 59 lancewillett20 months ago

Replying to DrewAPicture:

22044.8-rtl.diff Handles the IE7/8 submenu alignment problems, incorporating one declaration from 22044.7-rtl.diff.

Thanks, testing it now.

Still having issues with the submenu z-indexes in IE7. I tried the trick of setting the first level to z-index: 99 and lower levels to z-index: 1, but no dice. I'm stumped.

As noted above, let's punt on that fix and leave it as is.

lancewillett20 months ago

A few more RTL IE7,8 fixes; move styles to bottom and a typo fix

comment:61 lancewillett20 months ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 22688:

Twenty Twelve: add RTL overrides for IE7 and IE8, props DrewAPicture. Closes #22044.

comment:62 lancewillett20 months ago

  • Keywords needs-testing rtl-feedback has-patch removed

IE-specific issues should be opened in new tickets.

Note: See TracTickets for help on using tickets.