Skip to:
Content

Opened 3 years ago

Closed 2 years ago

#3595 closed defect (bug) (fixed)

Menu still not working with 1.5 and IE7

Reported by: blogsterfiles Owned by:
Milestone: 1.6 Priority: normal
Severity: major Version: 1.5
Component: Theme Keywords: needs-patch
Cc:

Description

In default mode, BP 1.5 menus are still malformed. If you click the blue compatibility button (see img), it seems to correct the problem when the button turns from blue to gray, but it's like pulling hen's teeth to get site visitors to take that simple step with their browser. Image is from testbp.org, but same thing is still happening on 1.5 release just uploaded.

Attachments (6)

ie9.jpg (81.6 KB) - added by blogsterfiles 3 years ago.
IE9 in default mode (not compatibility mode)
3595.01.patch (659 bytes) - added by r-a-y 3 years ago.
IE9(2).jpg (22.1 KB) - added by blogsterfiles 3 years ago.
3595.02.patch (1.3 KB) - added by r-a-y 3 years ago.
3595.03.patch (760 bytes) - added by r-a-y 3 years ago.
fr6.patch (523 bytes) - added by DJPaul 2 years ago.

Download all attachments as: .zip

Change History (54)

blogsterfiles3 years ago

IE9 in default mode (not compatibility mode)

comment:1 johnjamesjacoby3 years ago

  • Component changed from Core to Theme
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 1.5.1
  • Version set to 1.5

comment:2 blogsterfiles3 years ago

  • Severity changed from normal to major

Just wondering if anyone has any ideas on this. 1.5 menus still do not work with IE unless in compatibility mode! Am I the only one who thinks this is a major issue (not being facetious, just wondering if I'm missing something)? Anyway, BP 1.5 is awesome and I blame Microsoft for having a crappy browser, but still, BP is useless to me as long as 100% of visitors using IE see a broken site when they visit. Much appreciated if anyone has any suggestions or even a temp fix or some idea if it will be addressed in the future or not. Thanks!

comment:3 boonebgorges3 years ago

I think it's important, but it will not get fixed unless someone who understands IE9's rendering is able to provide a patch. Do you have any suggestions?

comment:4 cnorris233 years ago

I'm seeing menus appear the same way with IE7. Hopefully, I can investigate this weekend.

comment:5 DJPaul3 years ago

Did the responsive styles break this?

comment:6 r-a-y3 years ago

Yes, the responsive styles broke this.

Looking into it.

Last edited 3 years ago by r-a-y (previous) (diff)

r-a-y3 years ago

comment:7 follow-up: r-a-y3 years ago

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

This patch addresses the issue for IE7; needs testing in IE9.

When I have a chance, I'll test on IE9 (on my other com that doesn't have my debug tools) unless someone beats me to it!

---

EDIT: You can change the height for #navigation to 63px and that will address #3588 as well.

Last edited 3 years ago by r-a-y (previous) (diff)

comment:8 in reply to: ↑ 7 blogsterfiles3 years ago

I'll leave it to someone more quailfied than me to determine if this is resolved or not, but a quick test with IE9 seems to work fine with r-a-y's patch! Thanks very very much to r-a-y and everyone who responded! What a great community...awesome work!

I noticed radius corners are still broken with IE9 in default mode, but work now in compatability mode. I'm off to see if someone has a ticket going on that now...thanks again everyone!

blogsterfiles3 years ago

comment:9 blogsterfiles3 years ago

May have spoke too soon. When I added drop down menus and went to the "Home" tab, focus was on both "Home" and "Blogs" tabs. Pointing to the tabs with drop down menus, they appear and work perfectly, and as soon as I click on any other tab, everything works fine, including focus leaving the "Home" tab and being only on whichever top level tab is clicked. (logged in or logged out makes no differance)

Update: Focus is not an issue; upon deleting and recreating the menu, the problem vanished.

Last edited 3 years ago by blogsterfiles (previous) (diff)

comment:10 r-a-y3 years ago

I don't believe this is the fault of the patch, but rather a BuddyPress theme issue.

If you can test this on another browser like Firefox or Chrome and can duplicate the issue, then please open another ticket about this.

comment:11 r-a-y3 years ago

  • Keywords needs-testing removed
  • Summary changed from Menu still not working with 1.5 and IE9 to Menu still not working with 1.5 and IE7

This bug only exists in IE7. The OP has IE9 set up to run any page in compatibility mode, which defaults to IE7 mode.

comment:12 boonebgorges3 years ago

r-a-y (or anyone else with IE7 knowledge) - any leads on a fix? This is the one ticket left in the milestone that I would *really* prefer not to punt.

comment:13 r-a-y3 years ago

01.patch works from the testing I've done.

Try the patch on all other browsers as well. I've tested in Firefox, Chrome, Safari and Opera and haven't found any conflicts (thus far). Even works in IE6 (except for the dropdown menu since IE6 doesn't support the :hover CSS selector, but that's just IE6 being IE6!).

The problem blogsterfiles notes above could be an issue with WP menus and I believe is separate.

The only minor issue that could arise is how this affects child themes; BP TPack users won't be affected at all.

The declaration that I think would affect child themes is:
#navigation {height:70px;}

I needed to do this so the height of the #header stays intact since I'm using position:absolute in #nav.

Of course, feel free to tweak the CSS!

comment:14 boonebgorges3 years ago

Oh, cool. I have tested 01.patch on a slew of browsers and it all looks good to me.

Not sure what to say about the #navigation height issue. I'm sure it will affect at least some child themes. The change means that the vertical space between the tabs and the search box is dictated by the height of the #navigation element rather than the top margin on #nav (you can change 40px to whatever you want since it's now absolutely positioned). That seems like a pretty significant change the way that the layout is determined. And the switch to absolute positioning seems like a change for the worse.

So. A couple questions.

  • How committed are we to IE7 support? Enough to make a significant CSS change mid-cycle, just to support it?
  • Are there other approaches? Seems like the root of the problem is display: inline-block on #nav li, which IE7 doesn't know what to do with. It's possible that we could work around this with some browser hacks in our existing stylesheet (see eg http://grasshopperpebbles.com/css/css-inline-block-ie7-hack/), or with a conditionally loaded < IE7 stylesheet. Both of these options are icky in themselves, of course, but is it worth considering the trade-offs?
  • Am I overreacting to r-a-y's proposed change? I'm not much of a front-end guy, maybe I'm making a mountain out of a molehill and we should just go with his proposal :)

comment:15 DJPaul3 years ago

I'm a bit concerned about setting the height on #navigation w.r.t #3588 - the impact on themes that have already set custom header images.

comment:16 r-a-y3 years ago

Seems like the root of the problem is display: inline-block on #nav li, which IE7 doesn't know what to do with. It's possible that we could work around this with some browser hacks in our existing stylesheet (see eg http://grasshopperpebbles.com/css/css-inline-block-ie7-hack/), or with a conditionally loaded < IE7 stylesheet. Both of these options are icky in themselves, of course, but is it worth considering the trade-offs?

That's correct. Though the browser hacks create problems for the tabs and dropdown menus when you hover hover them.

---

My approach uses absolute positioning because I'm using float in #nav li. A potential workaround in my patch instead of setting the height is adding a clear div before closing the #navigation div.

EDIT: Spoke too soon! Let me experiment a bit more.

Last edited 3 years ago by r-a-y (previous) (diff)

r-a-y3 years ago

comment:17 r-a-y3 years ago

02.patch is a little cleaner, but introduces an empty div to clear the float.

No absolute positioning here! Cons are the empty div and child themes that have overridden header.php

Last edited 3 years ago by r-a-y (previous) (diff)

r-a-y3 years ago

comment:18 r-a-y3 years ago

03.patch uses the same approach as 02, but uses the CSS :after selector to clear #navigation. Pure CSS!

comment:19 boonebgorges3 years ago

3595.03.patch looks nice in all browsers I've checked, and it seems like the most elegant solution presented so far. The change in the styling here is, IMO, less intrusive than in 01.patch, as it doesn't change the height of any elements.

Feedback from another dev, who knows their browsers better than I do?

comment:20 DJPaul3 years ago

I thought :after isn't supported in IE7?

comment:21 r-a-y3 years ago

The good browsers will use :after.

The workaround for IE7 is the usage of float: left in #nav li. Due to IE7's shoddy rendering, this makes it work without having to clear anything.

comment:22 DJPaul2 years ago

  • Milestone changed from 1.5.2 to 1.6

comment:24 karmatosed2 years ago

Just delved a little into this and seems 2 issues to me:

  1. We don't use a respond.js but have media queries- really should be as not all use full width experience of a browser.

However.. this is not the main reason :

  1. Totally agree with Ray it's because we don't have a float left or right (or ie 7 friendly equivalent) for the menu items we use other methods from what I can see. I fudged a fix by adding float right to the li - probably not a full fix but it shows issue.
Last edited 2 years ago by karmatosed (previous) (diff)

comment:25 hnla2 years ago

  • Keywords dev-feedback added

Your patch .03 provides best solution, if inline-block was required then the correct approach would be to add a CC sheet for IE and setting display: inline - not hugely easy with WP - given the issue changing to 'float' is the best solution.

  • 'display: inline-block;' is largely redundant on the li elements now, however it is a trigger for the MS construct hasLayout forcing it to '= true' which will then act to contain floats so may be left in place at no particular cost.
  • For safety font-size: 0; line-height:0; ought to be added to that pseudo element to ensure no browsers try and preserve line-height/height for the element. There is an updated approach to the time honoured 'clearfix' approach which uses the display:table property to effect clearing and is somewhat lighter in properties required.
  • While true that IE7 =< 7 does not grok pseudo element :after float containment is provided for on the parent #navigation element due to it having a stated dimension 'width' which is a 'hasLayout = true' trigger.

I would add this patch and close ticket.

comment:26 blogsterfiles2 years ago

"I would add this patch and close ticket."

I agree. The patch seems to work just fine. So how/who does that? This seems like such a serious issue I'm surprised it hasn't been added already. Would love to see it in the core so I don't have to keep adding it, but in any event, thanks very much for the patch.

comment:27 johnjamesjacoby2 years ago

What's the verdict team? Is .03 good to go?

comment:28 blogsterfiles2 years ago

One in five are still using IE...is there anything I can do to help get this patch added? It's fairly important I think. Thanks :)

comment:29 follow-up: r-a-y2 years ago

I believe 03.patch is good to go.

hnla outlined some other declarations that could be added as well:
https://buddypress.trac.wordpress.org/ticket/3595#comment:25 (point 2)

comment:30 in reply to: ↑ 29 mdpane2 years ago

Replying to r-a-y:

I believe 03.patch is good to go.

hnla outlined some other declarations that could be added as well:
https://buddypress.trac.wordpress.org/ticket/3595#comment:25 (point 2)

Indeed, 03.patch fixed the problems I was having in IE9's IE7 compatibility mode.

Thanks for the patch, r-a-y.

Last edited 2 years ago by mdpane (previous) (diff)

comment:31 boonebgorges2 years ago

(In [5839]) Changes the way that bp-default main navigation tabs are positioned.
Fixes malformed navigation in IE default mode.
See #3595. Props r-a-y

comment:32 boonebgorges2 years ago

  • Keywords has-patch removed

I've applied .03.patch to trunk.

This is an important enough fix that it seems worth putting into the 1.5 branch, but in the case of a minor release, I'm far more concerned about breaking existing child themes. r-a-y or others, do you have an opinion about how safe this particular fix is for existing themes? (Looks to me like there will only be problems if the child theme makes major changes to the positioning of the parent element, which will result in odd behavior for the floated #nav, but I'm not sure how likely that is.)

comment:33 DJPaul2 years ago

Does this need a change to the RTL stylesheet?

comment:34 r-a-y2 years ago

I haven't tested any child themes. The one declaration that might be troublesome is:
#nav {float:right;}

Should probably test against some popular child themes like Frisco and, probably, the upcoming "Status" theme.

DJPaul is correct that a change probably needs to be made to the RTL stylesheet as well.

---

If this patch indeed becomes troublesome, we can revert the patch and perhaps I'll release this as a minor plugin. I would rather existing child themes work as they're supposed to than supporting IE7 (which is losing browser market share every month).

Last edited 2 years ago by r-a-y (previous) (diff)

comment:35 hnla2 years ago

Floats without widths, historically, can cause issues, user agents have to know how to correctly shrinkwrap non dimensioned floats, and guess which never could do that :}

I'll try and check the patch against 'Status' and see how it stands up.

comment:36 DJPaul2 years ago

  • Keywords needs-patch added; dev-feedback removed

We need an IE7 RTL fix for this. Attaching a patch that comes close, but seems to do something wrong with the width of the page.

DJPaul2 years ago

comment:37 hnla2 years ago

Ouch just had a look at the patch on IE7 and there appears a lot wrong and not easy to figure where the collapse is and spent a little time with it.

Not sure if this is going to be fixable without spending a fair bit of time on it.

comment:38 sooskriszta2 years ago

Guys, do you really want to spend so much time trying to create a fix for IE7, which even Microsoft doesn't support anymore, and whose usage numbers are dwindling (5.4% at last count http://www.w3counter.com/trends )? IMHO, it is great that the issue has been fixed for IE9

comment:39 fanquake2 years ago

I agree with sooskriszta . IE7 is old, lets not spend to much time trying to fix something thats been broken for what seems like a long time, and is quickly becoming less and less of a issue.

comment:40 r-a-y2 years ago

I agree with everyone who says we should drop support for IE7 entirely.

However, we still have to deal with IE8/9/10 and users who unknowingly use IE's Compatibility View, which uses IE7's rendering engine and is the main reason that the OP started this ticket.

The easiest solution is to add the following meta tag into the bp-default theme:

<meta http-equiv="X-UA-Compatible" content="IE=edge" >

This will tell IE to use the latest engine available on IE and will hence resolve issues on IE8+. If everyone likes this idea, I'll work up a patch. We'll have to be considerate to see if a child theme already uses this meta tag.

comment:41 hnla2 years ago

I would have to agree with r-a-y I think, I'm not really a fan of adding these IE meta tags and hiss everytime I see the used for no good reason on sites where you know the dev hasn't really understood their purpose.

BP has not historically paid much more than lip service to IE so it is a bit late to be taking too much time out on this, I had a look and was planning to spend a little further time with it but think we ought to go with r-a-y's suggestions especially as it means we tell IE to correctly use the best rendering engine available for given version in other words NOT forcing => IE8 to use a sub standard engine.

However if we really want to pull IE7 into shape we can, I can set aside a bit of time to sort some of the issues.

comment:42 boonebgorges2 years ago

It would be nice to solve this with a meta tag rather than more css hacks. r-a-y, do you have any ideas how we could check for the existence of said tag without resorting to a dirty ob_start() trick?

comment:43 r-a-y2 years ago

In PHP, I don't think there's a way around using object buffering to check for the meta tag.
It might be possible to do this with jQuery, but is this a good idea?

It might be better to just force the <meta> tag and let developers opt-out of this with remove_action. This is akin to how WordPress forces a ton of <meta> bloat (like RSS feeds, link relationships, Windows Live Writer support, etc.), but allows devs to unhook them.

Chances are slim that there are bp-default child themes using this tag anyway.

comment:44 johnjamesjacoby2 years ago

7 months is awful long time to have several people working on this. Let's make some decisions.

  • No solving this with PHP or jQuery
  • No IE7 specific hacks
  • No browser specific meta tags in the theme, because we'll have to support it forever
  • Maintain backwards compatibility as much as possible

If there's absolutely no way to do all of these things, it's okay to leave it as it is. If it's more broken than we left it in 1.5, let's revert and deal with it later, or announce on the dev blog we're breaking backpat for some CSS.

comment:45 johnjamesjacoby2 years ago

Check out "The Default Theme" for a little more insight into what's in the pipeline. I agree this individual CSS quick is a pain, and I love that we all care so much about it. Let's take this energy and direct it towards a more long term solution. :)

comment:46 hnla2 years ago

Lets draw a line under this firmly then based on the above; if we adhere to the list of 'don'ts' above we have a pretty hard time of it.

This is a theme issue and as such it is an issue that can be managed by devs or users if absolutely necessary.

I've also realised that the original ticket premise was/is flawed it states:

"IE9 in default mode (not compatibility mode)"

But shows a view of IE IN compatibility mode.

That there are issues in older IE browsers we have always been aware of, I'm pretty sure there was a general consensus not to spend too much time with attempting to get bp-default working with 6/7 in the past - if we were to spend the time then the only real accepted practise is to filter rules via MC CC styles.

bp-default has served well but doesn't warrant the time involved, especially given your post JJ on the next gen of bp skeletal theme files.

comment:47 blogsterfiles2 years ago

First, it seems to me the best solution to this problem was derailed over concerns about IE7. Is that really a valid concern when IE7 represents a mere 5% and diminishing segment of browser use? Plus consider that most users who would be confused by the compatibility view button are likely to be using more recent versions of IE anyway, perhaps leveraging the merit of focusing on newer versions of IE to the point concerns over IE7 effectively are moot. I think simple triage logic would compel reasonable minds to look forward and not strain against forward progress by clinging to an arbitrary point of backwards compatibility.

Second, the genius of the current state of BuddyPress is that with the current BP-Default theme, BP is a practical out of the box solution for thousands of businesses, organizations, and individuals who are not able themselves, or can't justify the cost of hiring developers to customize a theme. While the original ticket image illustrated "IE IN compatibility mode" as hnla indicated above, I suggest that is a moot point. The fact remains that unless you manually install R-A-Y's patch, all visitors to a BP site using the BP-Default theme on IE will see the compatibility button in IE browsers, and if they click it (and somehow it seems they always do), they will see a broken site until they click it again, or use another browser, or click their tongue and leave never to return again. It is what it is.

While this debate has gone on, I've been using the patch without a single complaint, so is there any real reason to disagree with R-A-Y's comment 2 months ago that the patch is good to go? BP is awesome... ...please just put this patch in the theme so we don't have to reload it with every update and we can spend our time contemplating and enjoying the wonders to come! Life is good, you guys are great! Thanks for listening. :)

Last edited 2 years ago by blogsterfiles (previous) (diff)

comment:48 DJPaul2 years ago

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

Things are better for IE7 on 1.6 than they were in 1.5. RTL in IE7 on 1.6 still has issues, but so did RTL in IE7 on 1.5. I'm going to close this ticket and open a new one for the specific RTL problem.

Note: See TracTickets for help on using tickets.