Improve tabledrag grippie CSS (and fix an IE7 alignment issue)

tagging for novice queue

this would be good for a "JS/jQuery/CSS/IE-specific hacks" guru :)

For whatever reason, IE6/7 doesn't pick up on the padding-top of the handle's anchor element. Since margin-top is almost equal to the negative of the padding, I just zeroed both. Seems to achieve the desired visual effect.

I've attached a screenshot of IE7. Someone please confirm it works for IE6. I'm pretty sure it does, but I don't have IE6 running in this box.

Forgot to add that the CSS is still valid CSS 2.1. ^_^

That's strange. The patch passed last time I checked. Let's re-test!

I just tested in IE6 and it doesn't work. It looks like the CSS hack isn't being picked up as a valid selector, though I'm not sure how much I trust the IE6 webdeveloper toolbar. Perhaps this would be better fixed in the jQuery side of things? It does work as expected in IE7 and IE8 (which should ignore the CSS anyways).

FYI MS has IE VPC images available for website testing: http://www.microsoft.com/downloads/details.aspx?FamilyId=21EABB90-958F-4...

Thanks for the IE VPC link!

I rendered it with IE6 and - you're right - the misalignment is still there. The CSS fix does work for IE7 though.

I've been playing around with margins and paddings and can't seem to effect the right change. I've managed to vertically centre the handle icon but the extra pixel still pops up when I hover over the handle. =(

I can't figure out the IE6 fix. I've been using IETester to render my tests in IE6 (and IE7).

This patch is a re-roll of just the IE7 fix. To get this committed, I've forked the IE6 part to its own issue: #616620: The tabledrag grippie is misaligned in IE6. I'm not particularly interested in IE6 support, and don't want to spend more time on this.

I tested in IE7, works fine to me (Vista, IE7, Drupal-6 version).

Bugs are first fixed in Drupal 7, then backported to Drupal 6. I suspect IE7 specific hacks might not be added to Drupal core, but that is up to Drupal 7 maintainers after all :)

Moving "drupal.css" component issues to "markup".

This issue may be impacted by this issue: #735628: Resizable textarea behavior leads to unpredictable results

This is still not fixed.

Can we make this our next markup patch jacine or sun?
Can we fix this without using hacks in core?

subscribing :)

@aspilicious Awesome! Yes, we should definitely try to fix it, sans hacks.

Clearly a task for display: inline-block.

It's true that /* vertical-align: middle; IE? */ doesn't work in IE...

#15: drupal.tabledrag.15.patch queued for re-testing.

#15: drupal.tabledrag.15.patch queued for re-testing.

This looks good in IE7 can't test IE6.
Sun can you fix those todo's?

Needs a reroll.

Attached is a re-roll of comment #15. Since then, the filenames have changed:

system/system-behavior-rtl.css --> system.base-rtl.css
system/system-behavior.css --> system.base.css

This worked for me on IE8 (before and after). Would need someone to verify on IE7 and/or work on the TODO in the patch. Have fun!

Let the testbot test it first.

Tagging as "Needs manual testing" for cross-browser testing. I guess we should check:

1. Whether the IE bug is fixed by the patch.
2. Whether the RTL behavior is affected.
3. General cross-browser testing to ensure everything's correct before and after the patch.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

+++ b/modules/system/system.base.cssundefined
@@ -103,19 +103,17 @@ a.tabledrag-handle:hover {
 div.indentation {
...
+  ¶

Also, trailing whitespace here.

Patch re-rolled to take care of D8 core dir and comment #27 taken care of. From #26:
Tagging as "Needs manual testing" for cross-browser testing. If someone could now check:

Whether the IE bug is fixed by the patch.
Whether the RTL behavior is affected.
General cross-browser testing to ensure everything's correct before and after the patch.

+++ b/core/modules/system/system.base-rtl.cssundefined
@@ -35,14 +35,11 @@ html.js input.throbbing {
 div.indentation {
-  float: right;
-  margin: -0.4em -0.4em -0.4em 0.2em;
-  padding: 0.42em 0.6em 0.42em 0;
+   /* @todo Floats were used to not fiddle with RTL in JS. */

Why do we need this?
And we have to be sure the float thingie is adressed.

I tested in rtl and the grippie looks fine, not sure if this affects more stuff

+++ b/core/modules/system/system.base.cssundefined
@@ -91,11 +91,11 @@ body.drag {
+  /* vertical-align: middle; IE? */

Should go as it's not need

IE7 works
RTL works
Chrome and firefox works

28 days to next Drupal core point release.

Updated per #29.

Alright, let's get some more testers.

I'll test this tonight.

Testing now.

Pre-patch, on admin/structure/block andadmin/structure/menu/manage/management the "grippie" icon does indeed appear to be placed too high so that it is not on the same horizontal plane as the the text it accompanies.

When I tried to apply the patch to an up-to-date repository I received the following errors:
error: core/modules/system/system.base-rtl.css: No such file or directory
error: core/modules/system/system.base.css: No such file or directory

Going to test anyway.

No change

Changing status to needs work.

#30: drupal.tabledrag.30.patch queued for re-testing.

This is because of #916424: Merge system.messages.css and system.menus.css into system.theme.css. The patch will need to be rerolled again, but this time it's not as easy. :)

Sorry, I was mistaken. Those two files weren't a part of the change and they still exist. Are you testing against the latest 8.x code?

Just realized that my repo wasn't as up to date as it should have been after doing a pull. Testing again with patch applied.

Result:

As an IE7 user, I notice that the "grippie" images are well aligned. It appears that the horizonal arrows of the "grippie" are in the same horizontal plane as the proceeding text's middle.

As an IE8 user, I notice that the "grippie"'s middle is slightly lower that it's text's middle.

As an IE9 user, I notice that I get the same result as IE8.

In general I would like to see these changes:

Encode the IE7 specific changes in a file that is clearly marked as containing IE7 fixes. Then use drupal_add_css's ability to target this browser specific css at IE7. This will give us two things.

1. The ability to fix IE7 without degrading IE8 and IE9's experience.
2. Makes it easier to gut this browser specific override when we decide to drop support for IE7.

So we aren't supporting IE7 for D8 is this an issue for D7? If not we should just close(won't fix) it.

Correct, now that IE7 support has been dropped for D8 this becomes a D7-only issue. Thanks @cosmicdreams. However, for backport purposes, we also should not create new CSS files, because this interferes with custom themes that may already have overridden the main CSS. (See #1015798-82: Fieldsets inside vertical tabs have no title and can't be collapsed for an example of where browser-specific stylesheets have been rejected for backport.)

However, the issue is still NW to fix the regressions the current patch causes in IE8/9. We also should ensure there are no regressions in IE6.

Thanks!

fixed in ie6, 7, 8+. no css hacks needed, this is because of a margin-top/padding-top bug in ie versions 5,6 & 7. I moved the padding/margin out of the float, set an overflow:hidden there instead, and adjusted the positioning of the background image.

please review.

Looks great to me codewise; do we need any changes to the RTL CSS?

tagging "IE", we can't search 2 chars in d.org project :(

Assuming an xpost.

yep, thanks for catching the rtl. i will reroll, forgot to move the margin/padding out of the float there

ok, fixed up rtl css. tested in Arabic for rtl positioning

works in all supported browsers

Awesome! Let's get one other pair of eyes testing and then I think this is RTBC.

Our code doesn't so well that why has problem in IE7. I think it also a task for D8.

I don't know the history and why we do this:
<a class="tabledrag-handle" href="#" title="Drag to re-order"><div class="handle">&nbsp;</div></a>
a is inline-element, div is block element. it shouldn't pull block-level element inside a inline-element. (but in HTML5, it's okay)

better:
<a class="tabledrag-handle" href="#" title="Drag to re-order"><span class="handle"></span></a>

best (also for D7 if markup changes allowed):
<a class="tabledrag-handle" href="#" title="Drag to re-order">Drag to re-order</a>

also, making assumption and adding padding/margin to drag icon doesn't the best practice imo.

Attached a patch, it's not the ideal solution but solving this issue.

I'm not sure if you noticed droplet, but I posted a solution on #47, which includes rtl fixes and requires no markup changes. Regarding the way the sprite was added, this is a fairly common method, and it isn't desirable to have the text "drag to re-order" appear in the output.

@droplet, can you confirm whether the patch in #47 resolves the issue for you?

@#50,
Markup changes keep it better (accessibility). eg. home icon

<a class="" title="Home" href="/drupal7x/"><span class="home-link">Home</span></a>

@#51,
#47 works.

I can see why we adding padding / margin now. it enlarges the click area. here is my new patch (needs someone help to add RLT style)

+++ b/modules/system/system.base.cssundefined
 a.tabledrag-handle .handle {
+  display: block;

remove it, DIV is block level

+++ b/modules/system/system.base.cssundefined
@@ -93,21 +93,23 @@ body.drag {
+  margin: -0.4em 0 -0.4em -0.5em; /* LTR */
+  padding: 0.42em 1.5em 0.42em 0.5em; /* LTR */
   width: 13px;
 }

padding-right to 0.5, remove right region. and keeps top&bottom / left & right same click region

before

after

No IE specified code. why not patch D8 too ?

3 days to next Drupal core point release.

@droplet, please open a separate feature request for adding the accessibility text. This issue is only about fixing the display bug in IE7 and less, and therefore is only against D7 because we do not support IE7 in D8.

@xjm,

for any reason, it is needs work. e.g. remove display: block;. I haven't bring accessibility into this issue. see my patch.

No IE specified code, it will be an improve for all browsers, not only a fix for "IE7". Whenever we do some changes in D8, it's easier to port it back.

I'm okay with any actions. but a separate issue is totally duplicated to this one.

A separate issue is not duplicated. Adding additional markup and strings has nothing to do with the fact that the image is misaligned in IE7, as per the issue title. If there are separate adjustments to be made to the grippie, they should be in a separate issue (and they're not part of the functional bug).

That said, I'll ping catch and ask if he'd rather this CSS change go into D8 as well.

OK, i've read this over a few times, and checked out the code. I've re-rolled the patch without the display:block, added LTR comments and added the css for ltr. Tested in Arabic and all supported browsers.

Since there are no CSS hacks, and this is a better solution then what's in d8 core, I've rolled a patch for it as well.

hmm.. rebased, rerolled d7core patch

ah, it's trying to apply it against d8 because of the version set in the ticket :) d7 patch should work when the time comes.

please review

Good stuff. Retitling and tagging for backport.

Fail was on the D7 patch; the correct D8 patch is in #56. Please test/confirm.

Thanks

LTR part is RTBC to me. RTL looks fine but I haven't test it.

Committed and pushed to 8.x and 7.x. Thanks!