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

Status: RESOLVED FIXED

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

Description

[Tim De Pauw]

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:

Comment 1

[Bill Mason]

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.

Comment 2

[Jason Barnabe (np)]

Attachment: testcase

Comment 3

[Jason Barnabe (np)]

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.

Comment 4

[Tim De Pauw]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure why the patch I just attached to bug 213591 doesn't fix this.

Comment 7

[Jason Barnabe (np)]

Question: why do you want to fix this? If it says in the W3C HTML specs that
that's working exactly as it should...

Comment 8

[Tim De Pauw]

The W3C _recommends_, it doesn't enforce. Well, that's how I see it anyway.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, we don't implement code that maps the vspace attribute into vertical
margins, so I'm actually not sure how this worked before.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, rather, we have the code, but the margin isn't in the computed style.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase that alert()s the computed margin-top

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

[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

Comment 15

[Chris Casciano]

-> All/All... as i'm obviously seeing this on OS X as well

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is actually a regression from bug 213347.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase, with added input with attributes in reverse order

This testcase has a third input that has the attributes in the reverse order.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the underlying problem here is that
nsHTMLInputElement::StringToAttribute checks |IsImage()|.  (There might also be
problems with AttributeToString doing so, although maybe not.)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 129427 [details] [diff] [review]
patch

Nice catch.. r+sr=bzbarsky

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 129427 [details] [diff] [review]
patch

Approved for 1.5b.

/be

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-08-11 14:53 -0700.

Comment 25

[[e-mail]]

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?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not that I know of, and I don't know of there ever being any relevant to type=radio.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in regression test to reftest.

Comment 28

[Daniel Holbert [:dholbert]]

Note that this bug's tests currently fail on some linux machines (bug 474494) and on fennec (bug 471911).