WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#24736 closed defect (bug) (fixed)

Revisions: IE compatibilty

Reported by: ocean90 Owned by: 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 5 years ago.
24736.2.patch (1.3 KB) - added by ocean90 5 years ago.
Use :first-child and opacity filter for IE
24736.3.patch (3.8 KB) - added by adamsilverstein 5 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 5 years ago.
24736-combined.2.diff (2.5 KB) - added by markjaquith 5 years ago.
24736-combined.3.diff (2.2 KB) - added by ocean90 5 years ago.
removes one opacity filter, was twice for .revisions .loading-indicator
24736.4.patch (2.8 KB) - added by adamsilverstein 5 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 5 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 5 years ago.
corrects tooltips for IE7
24736.7.patch (6.4 KB) - added by markjaquith 5 years ago.
Fixes tooltip alignment for modern browsers.
24736.8.patch (6.0 KB) - added by adamsilverstein 5 years ago.
fix tooltip placement in ie8 rtl
24736.9.patch (7.6 KB) - added by adamsilverstein 5 years ago.
add some ie7 rtl tweaks
24736.10.patch (22.2 KB) - added by markjaquith 5 years ago.
24736.11.patch (22.2 KB) - added by markjaquith 5 years ago.
24736.7b.patch (8.1 KB) - added by adamsilverstein 5 years ago.
put tootltips inside centered 70% div that matches slider
24736.12.patch (22.2 KB) - added by markjaquith 5 years ago.
24736.13.patch (21.3 KB) - added by adamsilverstein 5 years ago.
switch tooltip from fixed to absolute positioning, otherwise doesn't scroll when page scrolled
24736.14.patch (426 bytes) - added by adamsilverstein 5 years ago.
standalone patch: hide tooltips and tickmarks in ie7
24736.15.patch (21.7 KB) - added by adamsilverstein 5 years ago.
ensure tooltip vertical position correct regardless of vertical page scroll
24736.16.patch (22.6 KB) - added by markjaquith 5 years ago.
24736.17.patch (22.8 KB) - added by markjaquith 5 years ago.
fix z-index issue.
24736.18.patch (22.2 KB) - added by adamsilverstein 5 years ago.
slight tooltip position adjustment for ie8 rtl mode
24736.z-index.sanity.diff (1.3 KB) - added by markjaquith 5 years ago.
24736.z-index.sanity.2.diff (2.5 KB) - added by markjaquith 5 years ago.
24736.z-index.sanity.3.diff (1.3 KB) - added by aaroncampbell 5 years ago.
ie7-revisions-1.jpg (46.4 KB) - added by aaroncampbell 5 years ago.
ie7-revisions-2.jpg (124.5 KB) - added by aaroncampbell 5 years ago.
ie7-revisions-3.jpg (262.4 KB) - added by aaroncampbell 5 years ago.
ie7-revisions-4.jpg (205.9 KB) - added by aaroncampbell 5 years ago.
24736.19.patch (801 bytes) - added by adamsilverstein 5 years ago.
refreshed removes tickmarks, tooltips in ie7
24736.diff (3.2 KB) - added by adamsilverstein 5 years ago.
switch tickmarks to absolute positioning
24736.patch.diff (3.2 KB) - added by adamsilverstein 5 years ago.
better patch name
24736.20.patch (4.0 KB) - added by adamsilverstein 5 years ago.
*works this time
24736.21.patch (4.1 KB) - added by adamsilverstein 5 years ago.
adjust ie8 rtl tooltip position
24736.22.patch (3.6 KB) - added by markjaquith 5 years ago.
24736.23.patch (3.7 KB) - added by markjaquith 5 years ago.
24736.24.patch (4.4 KB) - added by markjaquith 5 years ago.
24736.25.patch (4.3 KB) - added by adamsilverstein 5 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 5 years ago.
Bump IE RTL flipped tooltip placement by one pixel.
24736.27.patch (739 bytes) - added by markjaquith 5 years ago.
fix 1px hover-loop issue in Firefox
fixie7.diff (321 bytes) - added by adamsilverstein 5 years ago.
don't pin controls when scrolling in ie7; move to open ticket
24736.28.patch (426 bytes) - added by adamsilverstein 5 years ago.
fix alternating on/off hovering issue with tooltips in firefox
24736.29.patch (601 bytes) - added by markjaquith 5 years ago.

Download all attachments as: .zip

Change History (103)

@ocean90
5 years ago

#1 @ocean90
5 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
5 years ago

  • Description modified (diff)

@ocean90
5 years ago

Use :first-child and opacity filter for IE

#3 @ocean90
5 years ago

  • Description modified (diff)

#4 @ocean90
5 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
5 years ago

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

#5 @markjaquith
5 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
5 years ago

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

@ocean90
5 years ago

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

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

Replying to markjaquith:

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

Tested in IE. Works for me.

#8 @adamsilverstein
5 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
5 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
5 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
5 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
5 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
5 years ago

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

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

@adamsilverstein
5 years ago

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

#13 follow-up: @helen
5 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
5 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
5 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
5 years ago

corrects tooltips for IE7

#16 @adamsilverstein
5 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
5 years ago

Fixes tooltip alignment for modern browsers.

#17 @markjaquith
5 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
5 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 5 years ago by adamsilverstein (previous) (diff)

#19 follow-up: @markjaquith
5 years ago

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

@adamsilverstein
5 years ago

fix tooltip placement in ie8 rtl

#20 in reply to: ↑ 19 @adamsilverstein
5 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
5 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
5 years ago

add some ie7 rtl tweaks

#22 @markjaquith
5 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
5 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
5 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
5 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
5 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
5 years ago

put tootltips inside centered 70% div that matches slider

#27 @adamsilverstein
5 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
5 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
5 years ago

Refreshed after a conflict.

@adamsilverstein
5 years ago

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

#30 @adamsilverstein
5 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
5 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
5 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
5 years ago

standalone patch: hide tooltips and tickmarks in ie7

@adamsilverstein
5 years ago

ensure tooltip vertical position correct regardless of vertical page scroll

#33 @adamsilverstein
5 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
5 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
5 years ago

fix z-index issue.

@adamsilverstein
5 years ago

slight tooltip position adjustment for ie8 rtl mode

#35 @adamsilverstein
5 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
5 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
5 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
5 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
5 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
5 years ago

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

#41 @markjaquith
5 years ago

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

#42 @aaroncampbell
5 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
5 years ago

refreshed removes tickmarks, tooltips in ie7

#43 @adamsilverstein
5 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
5 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
5 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
5 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
5 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
5 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
5 years ago

working on a solution...

@adamsilverstein
5 years ago

switch tickmarks to absolute positioning

@adamsilverstein
5 years ago

better patch name

#50 @adamsilverstein
5 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
5 years ago

*works this time

#51 @adamsilverstein
5 years ago

need to retest ie, may need adjustment...

@adamsilverstein
5 years ago

adjust ie8 rtl tooltip position

#52 @markjaquith
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#53 follow-up: @markjaquith
5 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
5 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
5 years ago

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

@markjaquith
5 years ago

Bump IE RTL flipped tooltip placement by one pixel.

#55 @markjaquith
5 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
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for final IE review of #24804.

@markjaquith
5 years ago

fix 1px hover-loop issue in Firefox

#57 @markjaquith
5 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
5 years ago

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

#58 @markjaquith
5 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
5 years ago

fix alternating on/off hovering issue with tooltips in firefox

#59 @markjaquith
5 years ago

In 24775:

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

Props adamsilverstein. See #24736.

#60 @nacin
5 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.