Closed Bug 214077 Opened 21 years ago Closed 21 years ago

input type=image with vspace misaligned when attributes set in wrong order

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: bugzilla, Assigned: dbaron)

References

()

Details

(Keywords: testcase, Whiteboard: [patch])

Attachments

(4 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030726 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030726 Mozilla Firebird/0.6.1

When you scroll down to the bottom of the page (you can press End), you'll
notice the "continue" button is misaligned vertically in recent Fb nightlies,
even though Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a)
Gecko/20030718 and older versions have always displayed it properly.
(I know my code isn't brill, but it used to work.)

Reproducible: Always

Steps to Reproduce:
Confirmed PC/WinXP
Works 2003072204
Broken 2003072404
2003072304 suffered from a regression (bug 79315) that caused input elements to
not render at all, and that includes this site when viewed.
->Layout. This may not necessarily be block/inline.  Could be form controls.
Assignee: block-and-inline → other
Status: UNCONFIRMED → NEW
Component: Layout: Block & Inline → Layout
Ever confirmed: true
Attached file testcase
One of those is an IMG, one is INPUT.

According to the W3C, the vspace attribute doesn't apply to INPUT elements
(http://www.w3.org/TR/html4/struct/objects.html#adef-vspace).

I don't know why it worked before, so I'm just going to leave this for someone
else to figure out.
Component: Layout → Layout: Form Controls
Keywords: testcase
Ah, thanks Jason, that would explain it. I'm not sure if users will appreciate
this strict implementation though.

I updated my code, so the example no longer applies.
I'm guessing this is the same thing as the regression caused by bug 79315 -- the
view is probably getting sized by the code I put back in that I thought we
didn't need, but never positioned...
I'm not sure why the patch I just attached to bug 213591 doesn't fix this.
Depends on: 213591
Question: why do you want to fix this? If it says in the W3C HTML specs that
that's working exactly as it should...
The W3C _recommends_, it doesn't enforce. Well, that's how I see it anyway.
The reason I want to fix this is that the change I made shouldn't have broken
it.  If we want to stop supporting an attribute, we should remove the code that
handles that attribute rather than preventing it from working by some other bug.
Actually, we don't implement code that maps the vspace attribute into vertical
margins, so I'm actually not sure how this worked before.
Er, rather, we have the code, but the margin isn't in the computed style.
Comment on attachment 129264 [details]
testcase that alert()s the computed margin-top

I want to figure out when this testcase regressed (correct: "2px"; incorrect:
"0px").  I know it worked in 1.5alpha and it was broken in 2003-07-27-05-trunk,
and presumably it worked in 2003-07-22-04 per comment 1.
[23:34:37] <placenickhere> dbaron: 072403/os x trunk... 0px ... unpacking 072303 now
[23:39:56] <placenickhere> dbaron: 072303: 0px... 072203: 2px ... that's as good
asl you'll get from  me
-> All/All... as i'm obviously seeing this on OS X as well
OS: Windows XP → All
Hardware: PC → All
This is actually a regression from bug 213347.
This testcase has a third input that has the attributes in the reverse order.
So the problem here is that there's a bug when the attributes are set in one
order and not a bug when they're set in another order, and the patch in bug
213347 reversed the order.
Summary: input type=image misaligned → input type=image with vspace misaligned when attributes set in wrong order
I think the underlying problem here is that
nsHTMLInputElement::StringToAttribute checks |IsImage()|.  (There might also be
problems with AttributeToString doing so, although maybe not.)
Assignee: other → dbaron
Component: Layout: Form Controls → Style System
Status: NEW → ASSIGNED
Priority: -- → P4
Whiteboard: [patch]
Target Milestone: --- → mozilla1.6alpha
Attachment #129427 - Flags: superreview?(bz-vacation)
Attachment #129427 - Flags: review?(bz-vacation)
Comment on attachment 129427 [details] [diff] [review]
patch

Nice catch.. r+sr=bzbarsky
Attachment #129427 - Flags: superreview?(bz-vacation)
Attachment #129427 - Flags: superreview+
Attachment #129427 - Flags: review?(bz-vacation)
Attachment #129427 - Flags: review+
Comment on attachment 129427 [details] [diff] [review]
patch

Requesting 1.5b approval -- this is pretty low risk and fixes a recent
regression.  (I suspect specifying the type attribute first is more common, so
bz's change probably broke more than it fixed, but this fixes both cases for
the first time.)
Attachment #129427 - Flags: approval1.5b?
Flags: blocking1.5b+
Comment on attachment 129427 [details] [diff] [review]
patch

Approved for 1.5b.

/be
Attachment #129427 - Flags: approval1.5b? → approval1.5b+
Fix checked in to trunk, 2003-08-11 14:53 -0700.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Just seen a ng post for someone saying that type=radio only works if it's first
- so I'm assuimg he's got a pre-bug 213347 version - could there be other input
code that checks for attributes that haven't been set yet?
Not that I know of, and I don't know of there ever being any relevant to type=radio.
Depends on: 213347
Checked in regression test to reftest.
Flags: in-testsuite+
Blocks: 474494
Note that this bug's tests currently fail on some linux machines (bug 474494) and on fennec (bug 471911).
You need to log in before you can comment on or make changes to this bug.