Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

@Kris,
Thanks for redirect me to this issue.

This bug only happen to tabledrag tables.

According to #674082: Table borders are not correctly drawn, it was a Webkit bug but I can't reproduce with Chrome and Safari 5. so remove the webkit-specified comment.

(But don't remove it in D7 patch, D7 still support old browsers with 0% usage. **Sigh**)
(and don't combine with that selector, it don't work in IE8, guess it is a jQuery bug)
(will send D7 patch soon)

droplet’s picture

Status: Active » Needs review
Issue tags: +IE, +Needs manual testing, +Needs backport to D7, +Save IE
KrisBulman’s picture

rtl was broke in this patch, it was adding an extra border to the affected area. On inspection, there was no need for an ie.css modification in Seven, just a replacement of "tr td:last-child" with ".cell-last-child", which is all that needed to be done to the rtl stylesheet.

please review

KrisBulman’s picture

whoops, forgot LTR comments

droplet’s picture

Issue tags: +Novice

Kris,

From my tests, it's for IE8 only that why move to IE stylesheet. but keep it in normal sheet is no hurt for other browsers :)

hehe. Sorry, I didn't test for RTL in #1 patch, just do it with my exp.

tagging Novice for tester. It's a easy one.

EDIT: #4 is almost no differetn to #1. I think #1 is better for D8 (IE) and #4 for D7 (all browsers, e.g webkit 4)

KrisBulman’s picture

yeah, since it causes no regressions in other browsers, i see no need to add an ie specific stylesheet.

if an ie specific version is required, #1 will need to be re-rolled to fix RTL.

kristiaanvandeneynde’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Needs work
Issue tags: -Needs backport to D7, -Save IE

Perhaps this issue should be put on hold as far as D8 goes? See #1465948: [meta] Drop some IE8 coddling from Drupal core. In a nutshell: people are currently trying to figure out whether we still want additional code in D8 that would only fix display errors in IE8.

Seeing as D7 very clearly supports IE8 (and older webkit versions), I see no reason why this couldn't be implemented in D7. In that case, the comment about webkit should not just be overwritten with your comment about IE8. Please combine both comments into one.

droplet’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Save IE

although this is a border line only, it is a layout fix. It doesn't add any eye-candy effects or non-standard hacks. #1465948 is taking about don't let it break. I don't understand it.

kristiaanvandeneynde’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Save IE

Check #1507960: [meta] Isolate and/or remove IE-specific hacks in core markup, CSS and JavaScript, a follow-up on the previously linked issue.
In a nutshell: If it doesn't break anything, but just looks ugly in IE8, don't add extra code for it.

The suggested patch actually removes valid CSS3 selectors in favor of extra JavaScript and plain CSS2 classes.
Even though this is perfectly valid for D7, we should avoid such code at all costs in D8.

Also, I don't think a "Save IE" tag is necessary. You don't see people slapping "Save Safari" or "Save FF" on every thread about a browser-specific bug. We want to get rid of shoddy browsers. By painstakingly supporting them or, as you call it, "saving" them, we'll just keep them around for longer than they should be.

chanderbhushan’s picture

check the css file set proper width in % not in px .

chanderbhushan’s picture

Issue summary: View changes

clarification

diego21’s picture

Assigned: Unassigned » diego21
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.85 KB

Is this patch ok? I couldn't apply the patch correctly, and I don't know why yet. I did it with -v option and didn't show any error.

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

The latest patch looks fine to me.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Has anyone actually tested the latest patch, and are we sure it doesn't cause regressions?

I'm also not totally clear on why we need to rely on JavaScript to add the class (rather than doing it in HTML).

kristiaanvandeneynde’s picture

Since IE8 doesn't support the :last-child pseudo selector, it makes sense to add a class through JavaScript to achieve the requested result. However, the patch in #11 seems to add said class to every browser. At least, it does as far as Dreditor shows me.

I know browser-specific code isn't considered best practices (feature detect, don't browser detect), but can we make sure only 'broken' browsers get the extra class?

mimes’s picture

FileSize
151.36 KB

I've tested this. It works (see screenshot), but there should be a way of achieving this without the use of Javascript.
jQuery and Selectivizr both support pseudo-selection. We could just add some jQuery to do this and remove the CSS in the patch in comment 11, perhaps with $("tr td:last-child").css();, but that seems excessive for what should be a simple patch to support an older browser.
I agree, feature detection rather than browser detection would probably be the best way to achieve this.

droplet’s picture

jQuery and Selectivizr are Javascript.

Don't make thing more complex when it can be fix in a easy way. It's for D7.

droplet’s picture

Status: Needs review » Reviewed & tested by the community
kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Needs review

See #14:

  • Doesn't this add the class to all browsers?
  • Can we make sure only 'broken' browsers get the class?

As David Rothstein pointed out in #13: We need to avoid breaking the lay-out in browsers that actually worked before. Please only RTBC after the above questions are answered.

JohnAlbin’s picture

Assigned: diego21 » Unassigned
Status: Needs review » Closed (outdated)

IE8 is no longer supported