Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#14433 closed defect (fixed)

[regression] TextBox: layout quirks on IE7/quirksmode (TextBox_sizes.html test failures)

Reported by: bill Owned by: Douglas Hays
Priority: high Milestone: 1.8
Component: Dijit - Form Version: 1.7.0
Keywords: Cc: Douglas Hays
Blocked By: Blocking:

Description

[26417] added a new CSS rule to dijit_rtl.css:

.dj_ie7 .dijitInputContainer {
	/* to fix wrong text alignment in rtl text box in IE */
	display: inline-block;
}

Unfortunately it broke all the quirks-mode tests on IE7, such as:

dijit/tests/quirks.html?file=form/TextBox_sizes.html&a11y=1&dir=ltr

Note that the CSS above is applying in LTR mode too, which is suspicious.

Adding this rule seems to fix the problem, although I haven't finished testing it yet:

.dj_iequirks .dijitInputContainer {
    /* inline-block above breaks quirksmode, see dijit/tests/quirks.html?file=form/TextBox_sizes.html */
    display: inline;
}

Attachments (3)

correct.gif (10.3 KB) - added by bill 8 years ago.
how tests/_BidiSupport/form/test_TextBoxes.html should look
brokenPatch.patch (1.0 KB) - added by bill 8 years ago.
attempted patch according to Doug's suggestion, but it causes alignment test failures for me
14433.patch (63.7 KB) - added by Douglas Hays 8 years ago.
possible fix

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by bill

In [27393]:

Fix TextBox layout problems on IE7 quirks mode, fix on trunk/, refs #14433.

comment:2 Changed 8 years ago by bill

Cc: Douglas Hays added; dohfail removed
Keywords: dohfail added

That rules in dijit_rtl.css can also apply in LTR mode (i.e. dir=ltr), if textdir=rtl. So, it should be moved ti dijit.css. But, Doug was going to take a look to see if he could get rid of it altogether, so will wait on him.

comment:3 Changed 8 years ago by Douglas Hays

It seems that the IE specific display rule in dijit.css

.dijitInputContainer {
    #display: inline;
}

should be removed along with the other dijitInputContainer/display workarounds in dijit_rtl.css. I can't figure out why it was added in the first place.

Last edited 8 years ago by bill (previous) (diff)

Changed 8 years ago by bill

Attachment: correct.gif added

how tests/_BidiSupport/form/test_TextBoxes.html should look

comment:4 Changed 8 years ago by bill

I tried removing the following lines from dijit_rtl.css:

.dj_ie7 .dijitInputContainer {
	/* to fix wrong text alignment in rtl text box in IE */
	display: inline-block;
}
.dj_iequirks .dijitInputContainer {
    /* inline-block above breaks quirksmode, see dijit/tests/quirks.html?file=form/TextBox_sizes.html */
    display: inline;
}

Then when running dijit/tests/_BidiSupport/form/test_TextBoxes.html on IE7, the alignment on the second textbox is wrong. It should look like:

how tests/_BidiSupport/form/test_TextBoxes.html should look

But instead "hi" is left-aligned.

I then tried removing the #display inline you mentioned above (dijit.css line 66), and it fixed the alignment problem, like you said.

However, then I get errors on the sizing tests, at least on dijit/tests/quirks.html?file=form/TextBox_sizes.html&a11y=1&dir=ltr run standalone on IE7:

_AssertFailure: assertTrue('false') failed with hint: validationPos h 42 vs 46 
ERROR IN: function(){ compareToTextBox("h", true); }
FAILED test: y 0 ms
_AssertFailure: assertTrue('false') failed with hint: validationPos h 38 vs 42 
ERROR IN: function(){ compareToTextBox("h"); }
FAILED test: h 0 ms
_AssertFailure: assertTrue('false') failed with hint: padded textbox height: 40 vs 36 
ERROR IN: function(){ var domNodePos = dojo.position(widget.domNode); var paddedBoxPos = dojo.position(widget.focusNode.parentNode); var clientHeight = widget.domNode.clientHeight; var clientWidth = widget.domNode.clientWidth; doh.t(Math.abs(clientHeight-paddedBoxPos.h) <= 1, "padded textbox height: " + clientHeight + " vs " + paddedBoxPos.h); var border = (domNodePos.w - clientWidth) >> 1; // average border width var offset = Math.min(Math.abs(domNodePos.x + border - paddedBoxPos.x), Math.abs(domNodePos.x + domNodePos.w - border - paddedBoxPos.x - paddedBoxPos.w)); doh.t(offset <= 1, "padded textbox horizontally just left or right (" + offset + ") of domNode: domNode x/w/b = " + domNodePos.x+"/"+domNodePos.w+"/"+border + ", pad box x/w = " + paddedBoxPos.x+"/"+paddedBoxPos.w); }
FAILED test: padded box: dijitNumberTextBox 0 ms

The same test runs fine for me on a clean checkout.

Changed 8 years ago by bill

Attachment: brokenPatch.patch added

attempted patch according to Doug's suggestion, but it causes alignment test failures for me

comment:5 Changed 8 years ago by Douglas Hays

vertical-align:middle; also needs to be removed from the same rule in dijit.css

comment:6 Changed 8 years ago by bill

Owner: changed from bill to Douglas Hays
Status: newassigned

I tried removing the vertical-align:middle and I start getting failures in IE6, for example !TextBox_sizes.html?a11y=1&dir=ltr.

comment:7 Changed 8 years ago by Colin Snover

Milestone: 1.7.21.7.3

1.7.2 RC released, bumping milestone on remaining tickets.

comment:8 Changed 8 years ago by Douglas Hays

bill I ran TextBox_sizes.js on IE6/7/8/9, Chrome 17, FF10 in both strict and quirks mode and all tests pass for me. I thought you might want to review this though before I commit it. Also I'm not sure this should be fixed for 1.7.3 or not.

comment:9 Changed 8 years ago by bill

Unfortunately, with this patch on IE7, in dijit/tests/_BidiSupport/form/test_TextBoxes.html the second TextBox (the RTL one) puts the text at the left rather than at the right.

Changed 8 years ago by Douglas Hays

Attachment: 14433.patch added

possible fix

comment:10 Changed 8 years ago by Douglas Hays

Milestone: 1.7.31.8

Refreshed the attach patch by adding IE-specific CSS to fix related RTL IE bugs, and created an automated test to ensure RTL is rendering as expected.

comment:11 Changed 8 years ago by bill

Cool, looks good to me.

comment:12 Changed 8 years ago by Douglas Hays

Resolution: fixed
Status: assignedclosed

In [28279]:

Fixes #14433. CSS workarounds to IE text direction bugs. Added automated tests.

comment:13 Changed 7 years ago by bill

Keywords: dohfail removed
Resolution: fixed
Status: closedreopened

[28279] modified the compiled NumberSpinner.css file without modifying the source NumberSpinner.less file. Either NumberSpinner.less should be updated, or the NumberSpinner.css change should be rolled back.

comment:14 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [28653]:

Fixes #14433. Sync claro NumberSpinner?.less and .css files.

comment:15 Changed 7 years ago by bill

In [28708]:

check in actual file generated from less, refs #14433

comment:16 Changed 7 years ago by bill

In [30856]:

Tweaking comment. Helena said the issue was when textdir=rtl, not dir=rtl. Refs #14433.

Note: See TracTickets for help on using tickets.