Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#24736 closed defect (bug) (fixed)

Revisions: IE compatibilty

Reported by: ocean90's profile ocean90 Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Revisions Keywords: needs-patch
Focuses: Cc:

Description (last modified by ocean90)

  • [IE 7-9] While loading the revisions UI it suddenly redirects to the frontend: http://wp.dev/?revision=5514#wp-admin/revision.php
  • [IE 7] http://cl.ly/QDfc (Double scrollbar, loading indicator doesn't hide, tickmarks over slider width)
  • [IE 8] loading indicator doesn't hide
  • [IE 8] http://cl.ly/QDqI Tooltip arrow missing, tickmarks

Attachments (43)

24736.patch (894 bytes) - added by ocean90 11 years ago.
24736.2.patch (1.3 KB) - added by ocean90 11 years ago.
Use :first-child and opacity filter for IE
24736.3.patch (3.8 KB) - added by adamsilverstein 11 years ago.
corrects ie 8- not hiding loading spinner, adds filter: alpha(opacity=x) to css; also, lined up some '=' signs
24736-combined.diff (1.6 KB) - added by markjaquith 11 years ago.
24736-combined.2.diff (2.5 KB) - added by markjaquith 11 years ago.
24736-combined.3.diff (2.2 KB) - added by ocean90 11 years ago.
removes one opacity filter, was twice for .revisions .loading-indicator
24736.4.patch (2.8 KB) - added by adamsilverstein 11 years ago.
switch to right borders for tickmarks, leave off last div, fixes some overflow issues for ie7; also removed >div:first-child selector, no longer needed, not supported in ie 7
24736.5.patch (5.1 KB) - added by adamsilverstein 11 years ago.
use span instead of :after psuedo element for ie transform support; correct placement of tooltip in IE8 & RTL
24736.6.patch (5.6 KB) - added by adamsilverstein 11 years ago.
corrects tooltips for IE7
24736.7.patch (6.4 KB) - added by markjaquith 11 years ago.
Fixes tooltip alignment for modern browsers.
24736.8.patch (6.0 KB) - added by adamsilverstein 11 years ago.
fix tooltip placement in ie8 rtl
24736.9.patch (7.6 KB) - added by adamsilverstein 11 years ago.
add some ie7 rtl tweaks
24736.10.patch (22.2 KB) - added by markjaquith 11 years ago.
24736.11.patch (22.2 KB) - added by markjaquith 11 years ago.
24736.7b.patch (8.1 KB) - added by adamsilverstein 11 years ago.
put tootltips inside centered 70% div that matches slider
24736.12.patch (22.2 KB) - added by markjaquith 11 years ago.
24736.13.patch (21.3 KB) - added by adamsilverstein 11 years ago.
switch tooltip from fixed to absolute positioning, otherwise doesn't scroll when page scrolled
24736.14.patch (426 bytes) - added by adamsilverstein 11 years ago.
standalone patch: hide tooltips and tickmarks in ie7
24736.15.patch (21.7 KB) - added by adamsilverstein 11 years ago.
ensure tooltip vertical position correct regardless of vertical page scroll
24736.16.patch (22.6 KB) - added by markjaquith 11 years ago.
24736.17.patch (22.8 KB) - added by markjaquith 11 years ago.
fix z-index issue.
24736.18.patch (22.2 KB) - added by adamsilverstein 11 years ago.
slight tooltip position adjustment for ie8 rtl mode
24736.z-index.sanity.diff (1.3 KB) - added by markjaquith 11 years ago.
24736.z-index.sanity.2.diff (2.5 KB) - added by markjaquith 11 years ago.
24736.z-index.sanity.3.diff (1.3 KB) - added by aaroncampbell 11 years ago.
ie7-revisions-1.jpg (46.4 KB) - added by aaroncampbell 11 years ago.
ie7-revisions-2.jpg (124.5 KB) - added by aaroncampbell 11 years ago.
ie7-revisions-3.jpg (262.4 KB) - added by aaroncampbell 11 years ago.
ie7-revisions-4.jpg (205.9 KB) - added by aaroncampbell 11 years ago.
24736.19.patch (801 bytes) - added by adamsilverstein 11 years ago.
refreshed removes tickmarks, tooltips in ie7
24736.diff (3.2 KB) - added by adamsilverstein 11 years ago.
switch tickmarks to absolute positioning
24736.patch.diff (3.2 KB) - added by adamsilverstein 11 years ago.
better patch name
24736.20.patch (4.0 KB) - added by adamsilverstein 11 years ago.
*works this time
24736.21.patch (4.1 KB) - added by adamsilverstein 11 years ago.
adjust ie8 rtl tooltip position
24736.22.patch (3.6 KB) - added by markjaquith 11 years ago.
24736.23.patch (3.7 KB) - added by markjaquith 11 years ago.
24736.24.patch (4.4 KB) - added by markjaquith 11 years ago.
24736.25.patch (4.3 KB) - added by adamsilverstein 11 years ago.
builds on .24 patch; fixes ie8 rtl tooltip placement, lowers tooltips 1 pixel, fixes gravatar vertical alignment in rtl mode
24736.26.patch (4.6 KB) - added by markjaquith 11 years ago.
Bump IE RTL flipped tooltip placement by one pixel.
24736.27.patch (739 bytes) - added by markjaquith 11 years ago.
fix 1px hover-loop issue in Firefox
fixie7.diff (321 bytes) - added by adamsilverstein 11 years ago.
don't pin controls when scrolling in ie7; move to open ticket
24736.28.patch (426 bytes) - added by adamsilverstein 11 years ago.
fix alternating on/off hovering issue with tooltips in firefox
24736.29.patch (601 bytes) - added by markjaquith 11 years ago.

Download all attachments as: .zip

Change History (103)

@ocean90
11 years ago

#1 @ocean90
11 years ago

In 24689:

Revisions: Disable URL updating if a browser doesn't support the History API. Like IE < 10.

props markjaquith. see #24736.

#2 @ocean90
11 years ago

  • Description modified (diff)

@ocean90
11 years ago

Use :first-child and opacity filter for IE

#3 @ocean90
11 years ago

  • Description modified (diff)

#4 @ocean90
11 years ago

Re double scrollbar in IE 7: This comes through [24688]. For IE 7 it has to be applied to the HTML element directly.

@adamsilverstein
11 years ago

corrects ie 8- not hiding loading spinner, adds filter: alpha(opacity=x) to css; also, lined up some '=' signs

#5 @markjaquith
11 years ago

Combined the patches and removed the whitespace changes (at this point, we need to carefully consider changes, so let's make our patches as succinct as possible).

For the IE7 double scrollbar, we have two options: override the rule for IE7, or apply the fix using JS (which we can 100% count on, given that this is a JS app).

#6 follow-up: @markjaquith
11 years ago

Went with the overflow-y-in-JS route. Untested on IE.

@ocean90
11 years ago

removes one opacity filter, was twice for .revisions .loading-indicator

#7 in reply to: ↑ 6 @ocean90
11 years ago

Replying to markjaquith:

Went with the overflow-y-in-JS route. Untested on IE.

Tested in IE. Works for me.

#8 @adamsilverstein
11 years ago

testing on browserstack ie8 looks ok (still issue with tooltip arrow); on ie tickmarks overflow. scrollbars are gone. no arrow on tooltips.

#9 follow-ups: @markjaquith
11 years ago

The tooltip arrow thing is because of the transform, probably. The overflow is probably due to lack of support for border-box, so the border that makes the tick mark is causing it to equal more than 100%.

@adamsilverstein
11 years ago

switch to right borders for tickmarks, leave off last div, fixes some overflow issues for ie7; also removed >div:first-child selector, no longer needed, not supported in ie 7

#10 in reply to: ↑ 9 @adamsilverstein
11 years ago

Replying to markjaquith:

The tooltip arrow thing is because of the transform, probably. The overflow is probably due to lack of support for border-box, so the border that makes the tick mark is causing it to equal more than 100%.

apparently ie 7 also has a hard time adding percentages up to 100%, it is apparently rounding up and adding pixels in the process. latest patch shifts to a right border and leaves off the last tickmark, fixing overflow for ie7 at least until you have a lot of revisions.

#11 in reply to: ↑ 9 @adamsilverstein
11 years ago

Replying to markjaquith:

The tooltip arrow thing is because of the transform, probably. The overflow is probably due to lack of support for border-box, so the border that makes the tick mark is causing it to equal more than 100%.

tried adding the correct ie 7/8 transform code, but apparently older ie doesn't support transforming psuedo elements.

#12 @adamsilverstein
11 years ago

note: in 24736.5.patch tooltips are missing in ie7. needs fixing

Last edited 11 years ago by adamsilverstein (previous) (diff)

@adamsilverstein
11 years ago

use span instead of :after psuedo element for ie transform support; correct placement of tooltip in IE8 & RTL

#13 follow-up: @helen
11 years ago

I care approximately not at all about IE7. Customizer flat out doesn't support it, if I recall correctly.

#14 in reply to: ↑ 13 @adamsilverstein
11 years ago

Replying to helen:

I care approximately not at all about IE7. Customizer flat out doesn't support it, if I recall correctly.

with the latest patches works fine in ie8 other than lack of history.

#15 @azaozz
11 years ago

Also there is ie8 class on html and ie.css stylesheet that's loaded only in IE <= 7, if specific tweaks are needed for these browsers.

@adamsilverstein
11 years ago

corrects tooltips for IE7

#16 @adamsilverstein
11 years ago

24736.6.patch

  • fixes placement of tooltips and arrow transform for IE7
  • more specific > span clild selectors
  • noticed extra scrollbars in IE7 in RTL mode, somethings slightly messed up there

@markjaquith
11 years ago

Fixes tooltip alignment for modern browsers.

#17 @markjaquith
11 years ago

24736.7.patch

Fixes the tooltip alignment in modern browsers (Firefox, Chrome). If that breaks it in IE7, then put the fix in the IE stylesheet.

#18 @adamsilverstein
11 years ago

i think a bit from patch.6 got left out (ie.css); will test again later today to see how it looks.

Last edited 11 years ago by adamsilverstein (previous) (diff)

#19 follow-up: @markjaquith
11 years ago

asamsilverstein — .7 should have the same IE bit as .6 ... I started from your patch.

@adamsilverstein
11 years ago

fix tooltip placement in ie8 rtl

#20 in reply to: ↑ 19 @adamsilverstein
11 years ago

Replying to markjaquith:

asamsilverstein — .7 should have the same IE bit as .6 ... I started from your patch.

somehow missed that.

tooltips look fine in ie7/8, except in ie7 rtl mode; looking into it, see screencasts:

ie 7 - http://f.cl.ly/items/2V2b2M1I17450g2r1F2r/ie7%20-%20Broadband.m4v
ie 8 - http://f.cl.ly/items/2z3R0j0c3g0n2W003P2Y/ie8%20-%20Broadband.m4v
ie 7 rtl - http://f.cl.ly/items/1D2K243i2S0k2G463U1t/ie7rtl%20-%20Broadband.m4v
ie 8 rtl - http://f.cl.ly/items/0T0B2C0G1j42233r2u1s/ie8rtl%20-%20Broadband.m4v

#21 @adamsilverstein
11 years ago

after 24736.9.patch ie7 rtl works fine, only issue is with meta line not showing 'TO:' also meta info cut off

i'm noticing an issue with the meta timestamp/time ago strings in RTL mode the parentheses don't match!

ie7 after latest: http://f.cl.ly/items/192b231n3N3e3y2m3H3x/ie72%20-%20Broadband.m4v

@adamsilverstein
11 years ago

add some ie7 rtl tweaks

#22 @markjaquith
11 years ago

24736.9.patch contains too many regressions in modern browsers. Pointer positioning, meta bugs. The farther back you get from IE 10, the less we care about getting it pixel-perfect. 24736.7.diff doesn't have any regressions that I can see.

#23 follow-up: @ocean90
11 years ago

That are a lot of unnecessary changes IMO.

If IE can't display the arrow we can/should simply hide it. It's will not be functionally broken. See also ticket:18199:17.

#24 in reply to: ↑ 23 @adamsilverstein
11 years ago

Replying to ocean90:

That are a lot of unnecessary changes IMO.

If IE can't display the arrow we can/should simply hide it. It's will not be functionally broken. See also ticket:18199:17.

i think mark is working back from patch.7 version; changes seem fine if they are assigned to .ie8 class or are in the ie.css file, hiding tooltips also fine with me.

#25 @adamsilverstein
11 years ago

I'm going to rejig the tooltip HTML so its calculations match the tickmarks more exactly, starting back at patch 7; going to look at hiding tickmarks and/or tooltips for older non-standards compliant browsers.

#26 @markjaquith
11 years ago

So, tooltips were misaligned. The math was just wrong. And it wasn't ever going to work, due to the way browsers have to make sacrifices to fit items into a pixel grid. Turns out adamsilverstein partially had the right idea earlier, when he was pixel-aligning the tooltips. That's how it should be done, because otherwise it's always going to be janky.

Additionally, the way tooltips and tickmarks were done for RTL was... odd. It rendered the tick marks LTR, and did lots of backflips in the JS.

One problem is that the slider doesn't have an RTL mode. So the values and the handles are always going to be the same. So some amount of backflips are necessary, say then setting or reading from the slider.

So 24736.10.patch changes some stuff around:

Building on .7.patch, it makes it so that the tick marks use the tooltips model as its model. It then can communicate the exact pixel position of a particular tickmark when the hovered revision changes. The tooltip can that read that, and align itself.

I also removed a lot of the unnecessary and complicated RTL backflips by making the tick marks flow RTL in RTL mode.

Still chasing down one last issue in Firefox RTL.

@adamsilverstein
11 years ago

put tootltips inside centered 70% div that matches slider

#27 @adamsilverstein
11 years ago

i'm assuming your approach in 11.patch is better, but wanted to post what i was working on for review - i was putting the tooltips inside a div that matched the slider, letting the percentage values for the offset match.

i will test latest patch later today and test in ie7/8 to address remaining issues.

#28 follow-up: @markjaquith
11 years ago

adamsilverstein — yeah, I tried that too. But it doesn't work, because the percentage used for the tooltip is a single percentage. The percentage used for the relevant tickmark is the addition of many percentages. So, due to rounding and pixel grid restraints, it won't always match. It can be off by one or even two pixels, in some cases.

#29 @markjaquith
11 years ago

Refreshed after a conflict.

@adamsilverstein
11 years ago

switch tooltip from fixed to absolute positioning, otherwise doesn't scroll when page scrolled

#30 @adamsilverstein
11 years ago

removed JS bottom set, added back to css. fixed positioning works until you scroll down and the tooltip follows; horizontal positioning seems spot on in ff/chrome, testing in ie next.

#31 in reply to: ↑ 28 @adamsilverstein
11 years ago

Replying to markjaquith:

adamsilverstein — yeah, I tried that too. But it doesn't work, because the percentage used for the tooltip is a single percentage. The percentage used for the relevant tickmark is the addition of many percentages. So, due to rounding and pixel grid restraints, it won't always match. It can be off by one or even two pixels, in some cases.

reviewed how jquery ui slider decides where its 'steps' are (exactly where we want the ticks). interesting stuff:

_trimAlignValue - https://github.com/jquery/jquery-ui/blob/master/ui/jquery.ui.slider.js#L514

#32 @adamsilverstein
11 years ago

mmm... rechecking i see my change to position absolute messed up positioning, if we stick with fixed we need to address the scrolling issue.

@adamsilverstein
11 years ago

standalone patch: hide tooltips and tickmarks in ie7

@adamsilverstein
11 years ago

ensure tooltip vertical position correct regardless of vertical page scroll

#33 @adamsilverstein
11 years ago

24736.15.patch builds on .12, added correction for vertical offset for tickmark when page scrolled, and if window scrolled while tickmark visible.

#34 @markjaquith
11 years ago

24736.16.patch Builds on .13. It makes the tick marks 100% with a 15% left and right border. This makes the math work out better, and lets us return to absolute rather then relative positioning. We're back to bottom-based vertical alignment, but using CSS only, so this means the tooltip arrow lines up regardless of the height of the tooltip. Boom.

@markjaquith
11 years ago

fix z-index issue.

@adamsilverstein
11 years ago

slight tooltip position adjustment for ie8 rtl mode

#35 @adamsilverstein
11 years ago

positioning is pixel perfect with .17 patch! tooltips were slightly off in ie8 rtl mode, added CSS to fix that in 24736.18.patch; ie7 ticks and tooltips still off, just apply 24736.14.patch to hide. ie 9 and everything else i checked looked perfect.

#36 @markjaquith
11 years ago

In 24751:

Revisions: Re-work how tick marks and tooltips are aligned. IE fixes.

  • Pixel-based alignment of tooltips.
  • Bottom-based alignment, so arrow lines up using CSS only.
  • Better RTL styles (mostly mirror-imaged).
  • Better RTL calculations in revisions.js (less logic).
  • Better IE support.

See #24736. Props markjaquith, adamsilverstein, ocean90.

#37 @markjaquith
11 years ago

TO-DO:

  • Ensure no regressions in alternative modern browsers (so, IE10, Opera, Safari).
  • Make sure it's working reasonably well in IE8 and IE9.
  • Look at IE7. If the pixel alignment didn't help and it can't be salvaged, kill tooltips (and maybe tick marks) for IE7.

#38 @aaroncampbell
11 years ago

24736.z-index.sanity.3.diff fixes the issue from z-index.sanity where the handle of the slider shows through the active menu flyout

#39 @markjaquith
11 years ago

In 24753:

WORLD WAR Z-INDEX: Restoration of sanity to revisions/slider/menu z-index values.

The year is 2013. It is a period of DOM unrest. Sliders have hoarded
all available z-index, leading to an arms race. What started as a local
squabble soon escalated into a global conflict. The flyout menus were
the first of the collateral damage, trodden underfoot by mighty warring
enemies they couldn't effectively challenge. It was said they were
betrayed by one of their own: the z-index-power-hungry non-active
currently-hovered submenu, whose z-index-power level was over 9000!

But there is hope. A small band of heroes has emerged. Using their
powers of "what were you thinking?" and "chill, no one needs a z-index
THAT high", they have begun to restore order and harmony to the DOM.

See #24736. Props markjaquith, aaroncampbell, helen.

#40 @adamsilverstein
11 years ago

my vote for best commit message! will test in various browsers shortly.

#41 @markjaquith
11 years ago

IE 10 works perfectly. Waiting on info for IE 7–9.

#42 @aaroncampbell
11 years ago

I did some IE7 testing on XP:

First, the ticks are in the wrong place. They're shifted right so much that the rightmost one is off the screen and requires a horizontal scroll. The tooltip shows still a little further right. the post in the screenshots has 3 revisions.


Everything else seems to work.

@adamsilverstein
11 years ago

refreshed removes tickmarks, tooltips in ie7

#43 @adamsilverstein
11 years ago

here are my screenshots, safari next; everything seems fine except IE7!

ie7 - http://cl.ly/QN8m
ie7rtl - http://cl.ly/QMK0
ie7 patched - http://cl.ly/QMGC
ie7rtl patched - http://cl.ly/QMKI
ie8rtl - http://cl.ly/QLzs
ie8 - http://cl.ly/QNJt
ie9 - http://cl.ly/QMb6
ie9rtl - http://cl.ly/QM5n
ie10 - http://cl.ly/QMSH
opera10 - http://cl.ly/QM5n
opera10rtl - http://cl.ly/QM6w
opera12.5 - http://cl.ly/QMSF
opera12.5rtl - http://cl.ly/QMOH

#44 @adamsilverstein
11 years ago

safari test out ok:
safari 4 - http://cl.ly/QLwn
safari 4 rtl - http://cl.ly/QNH0
safari 5 - http://cl.ly/QMWi
safari 5 rtl - http://cl.ly/QMNh

#45 @markjaquith
11 years ago

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

In 24756:

Revisions: hide tick marks and tooltips for IE 7.

Not worth the effort it would take to get them to work.

Props adamsilverstein. Fixes #24736.

#46 @markjaquith
11 years ago

adamsilverstein — thanks so much for all that legwork!

Note that I went with a more direct and forceful removal from IE 7. Just less code, and an !important is fine here — an ideal use case, really.

#47 follow-up: @ocean90
11 years ago

Pixel-based alignment of tooltips.

Seems like we are getting now into trouble in Opera and Safari with some more revisions (>20).

#48 in reply to: ↑ 47 @adamsilverstein
11 years ago

Replying to ocean90:

Pixel-based alignment of tooltips.

Seems like we are getting now into trouble in Opera and Safari with some more revisions (>20).

good catch, i missed that somehow. i think we are running into classic sub-pixel problems - http://ejohn.org/blog/sub-pixel-problems-in-css; we are still using percentages and these browsers are rounding down vs the rest that round up.

#49 @adamsilverstein
11 years ago

working on a solution...

@adamsilverstein
11 years ago

switch tickmarks to absolute positioning

@adamsilverstein
11 years ago

better patch name

#50 @adamsilverstein
11 years ago

24736.patch.diff​

  • switches tickmarks to absolute and position to a calculated percentage (multiplication) - resolves an issue where math rounding down on partial pixels in safari and opera accumulated for each tick, with many ticks this meant they got further and further off. new method calculates a total percentage to avoid this issue. safari/opera may still show marks one pixel off, but the miscalculation is no longer cumulative and even with 200 revisions the tick marks work correctly.
  • reduce tooltip max width so it fits better near screen edges.
  • initial testing in safari LTR and RTL mode.

@adamsilverstein
11 years ago

*works this time

#51 @adamsilverstein
11 years ago

need to retest ie, may need adjustment...

@adamsilverstein
11 years ago

adjust ie8 rtl tooltip position

#52 @markjaquith
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#53 follow-up: @markjaquith
11 years ago

24736.24.patch Everything looks good to me except for tooltip alignment for the left half of the revisions in IE 8 in RTL mode. Not a blocker, but if someone can fix that up, go for it. Otherwise, please review!

#54 in reply to: ↑ 53 @adamsilverstein
11 years ago

Replying to markjaquith:

24736.24.patch Everything looks good to me except for tooltip alignment for the left half of the revisions in IE 8 in RTL mode. Not a blocker, but if someone can fix that up, go for it. Otherwise, please review!

this patch looks good. actually cleaned up the spacing of the tickmarks in ie8 a bit too (before - http://cl.ly/QNfR, after - http://cl.ly/QNgs); safari looks perfect. one issue i noticed was the tooltip seems one pixel too high;

attaching patch that fixes ie rtl tooltip placement and gravatar alignment issue (http://cl.ly/QNMo vs http://cl.ly/QNlA)

@adamsilverstein
11 years ago

builds on .24 patch; fixes ie8 rtl tooltip placement, lowers tooltips 1 pixel, fixes gravatar vertical alignment in rtl mode

@markjaquith
11 years ago

Bump IE RTL flipped tooltip placement by one pixel.

#55 @markjaquith
11 years ago

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

In 24768:

Revisions: tweak how tickmarks are rendered, to play nice with browsers who always round subpixel values down (Safari, Opera).

Also, some IE tweaks.

Fixes #24736. Props adamsilverstein, markjaquith, ocean90.

#56 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for final IE review of #24804.

@markjaquith
11 years ago

fix 1px hover-loop issue in Firefox

#57 @markjaquith
11 years ago

In 24770:

Revisions: Bump the tooltips up one pixel to prevent a Firefox hover-loop bug on the upper-border of the scrubber.

See #24736.

@adamsilverstein
11 years ago

don't pin controls when scrolling in ie7; move to open ticket

#58 @markjaquith
11 years ago

In 24772:

Revisions: Don't pin controls to the top in IE 7.

Seriously IE 7 users, this is more consideration than your browser deserves. You can thank us by upgrading.

See #24736. Props adamsilverstein.

@adamsilverstein
11 years ago

fix alternating on/off hovering issue with tooltips in firefox

#59 @markjaquith
11 years ago

In 24775:

Revisions: a tiny one-pixel and z-index tweak for Firefox.

Props adamsilverstein. See #24736.

#60 @nacin
11 years ago

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

This can be re-opened with further issues.

Note: See TracTickets for help on using tickets.