52019 - <font size="+.5"> interpreted as <font size="5">

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Mozilla handles <font size="+.5"> differently from Netscape 4.75.

                         IE    NS   Moz    treats it as...
<font size="+.5">        +0    +0   +5
<font size="+2.5">       +2    +2   +2

The HTML 3.2 spec requires integer values for font size 
(http://www.w3.org/TR/REC-html32#font), but I don't know what it says to do if 
it doesn't find an integer.  The w3c validator (http://validator.w3.org) 
doesn't check whether value is even a number.

Mozilla nightly build 2000 090908 on Windows 98.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Loco]

Attachment: Updated the output to reflect the difference between "0.5" and ".5"

Comment 3

[Loco]

Seems like our code here is ignoring the "." (<--decimal point) unless it is 
after a number.  Our parser is looking for a digit to start parsing a number, and 
not realizing that it perhaps should be looking for a decimal point as well.  
Test case seems to show that in any event.

Comment 4

[Kevin McCluskey (gone)]

Dividing up Claytons bugs to triage

Comment 5

[rods (gone)]

This HAS to be a parser issue.

Comment 6

[harishd]

This is definitely not a parser problem. Parser just consumes attribute content 
and passes it over to the layout. Reassigning bug to waterson.

Comment 7

[Chris Waterson]

-> team content

Comment 8

[Nisheeth Ranjan]

Nominating for rtm

Comment 9

[Loco]

*** Bug 54949 has been marked as a duplicate of this bug. ***

Comment 10

[Loco]

Uh-correct me if I'm wrong, but isn't the parser responsible for taking in the 
arguments?  The problem here is that the argument "0.5" is being seen as 
sifferent (by the parser?) from".5" for some crazy reason.  Basically my guess is 
that the engine (the parser?) is waiting for a NUMBER in order to start oassing 
the instructions over or making any decisions about it.  No?

Comment 11

[Nisheeth Ranjan]

Marking [rtm need info] and re-assigning to harish.

Comment 12

[harishd]

Attribtue value parsing does not happen in the parser. It happens in layout.
I will take a look into it anyway.

Comment 13

[harishd]

As I suspected the problem seems to be in nsString::ToInteger(). 

Giving bug to Scott.

Comment 14

[Jesse Ruderman]

See also bug 54142.

Comment 15

[Scott Collins]

All right, so here's my problem with this bug: "|ToInteger| doesn't work because 
it doesn't recognize non-integers".  Well, what is the correct fix for that?  
It's certainly not to make |ToInteger| parse floating point numbers instead!  It 
is completely unnacceptable to make |ToInteger| `succeed' in parsing by actually 
parsing a floating point number and rounding.  If you want this behavior, you 
need to put it higher up ... replacing the call to |ToInteger| with something 
else.  Now, if your problem is |ToInteger| shouldn't skip the |"."|, then that's 
a legitimate bug that can probably be fixed.  Essentially, though, this is 
erroneous input, and it probably needs to be dealt with both in |ToInteger| and 
in |ToInteger|s caller, right?

The spec doesn't say what to do when we encounter this erroneous input, therefore 
our behavior is allowed to be undefined.  What is the most reasonable course of 
action for us to take here?  As I currently see it, the standard allows our 
current behavior.  In that light, would we take a patch for RTM to fix it?

cc'ing jag, as this is his code :-)

Comment 16

[Koike Kazuhiko]

Reporter said IE and NN ignore string after '.'. Mozilla also should
ignore it as an illegal value.

Comment 17

[Peter "jag" Annema]

Well, I can quick hack |ToInteger| to throw away everything starting at a '.',
unfortunately the "skip garbage before number" part prevents that, since
somewhere in mozilla someone's bound to depend on |ToInteger| skipping that
point and parsing ``.5'' as 5.

So, what needs to be done is fix up all callers and fix |ToInteger| to be a bit
more sane, I'll discuss that with scc. In the mean time the caller will just
need to do something like |str1.Left(str2, str1.FindChar('.'));| and then
|str2.ToInteger();|

Comment 18

[Scott Collins]

I don't think this is necessarily the right fix.  Let's examine the callers and
see if someone depends on skipping garbage.  I'll wager they don't.  If they do,
let's fix them.  Then let's make |ToInteger| error out on leading garbage.  You
can see there aren't that many callers

  <http://lxr.mozilla.org/seamonkey/search?string=ToInteger>

Many of these sites are assertions; several are the implementation.  I'll start
looking.  What do the standards weenies say?  Can we get dbaron's opinion on
this?  CC'ing him.  David: what should the behavior be in this case?

Comment 19

[Peter "jag" Annema]

scc, that is what I meant. "fix up all callers" (which depend on the leading
garbage skipping). Sounds very good to me.

Basically, I'd like ToInteger to just return a number on valid input and return
an error code otherwise. What the caller does with that (i.e. remove cruft and
then try again) is their business. Also, should ToInteger be able to guess a
base, or would that be up to another function? In other words, what's the
specification of ToInteger?

Comment 20

[Jason Eager]

No comments/updates on this bug for 19 days now. Should fall off of the rtm radar.
Adding relnoteRTM keyword.

Comment 21

[Jesse Ruderman]

Draft release note:

A leading "." is dropped when parsing some attributes where an integer is 
expected.  This causes <font size="+.5"> to be interpreted as <font size="+5">, 
which is most likely not what the page author intended.  Additionally (bug 
54142), a "+" inside an integer is dropped, so <font size="3+0"> is interpreted 
as <font size="30">.

Gerv or someone, please add something about those examples not being valid html 
if other relnotes do the same, but try not to imply that Netscape 6.1 will have 
the same behavior.

Comment 22

[Chris Waterson]

rtm-

Comment 23

[Chris Waterson]

argh, thought this was my bug for some reason. restoring to original state.

Comment 24

[Jason Eager]

scc, can we either fix this bug or take it off of the rtm radar?

Comment 25

[Scott Collins]

What do you say, jag?  Do you want me to fix this?  Or do you want to do it?  In
any case, I'm sure this won't get in for RTM, and it already has the relnoteRTM
keyword ... time to remove rtm keyword?

Comment 26

[Peter "jag" Annema]

Removing the nomination (though I doubt it currently was on rtm radar, only rtm+
in the whiteboard does that).

So repeating my question, what is the desired contract for |ToInteger|?

Comment 27

[Jason Eager]

Nominating for Mozilla0.9, so we can get this figured out once and for all.

Comment 28

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 29

[bsharma]

qa contact updated.

Comment 30

[Scott Collins]

I'm making this bug your baby, jag :-)  You've done most of the work on it. 
There're more callers than there once were

  http://lxr.mozilla.org/mozilla/search?string=.ToInteger%28

I'm still not crazy about |ToInteger| being in the general string interface, and
but I don't mind a global that takes a readable as an argument.  I'm starting to
mellow on the direct use of iterators, since a dependent substring is usually
easier to come by for clients, so something like

  PRUint32 ToInteger(const nsAString&, PRUint32 aDefault, nsresult* aError=0 )

If we believed in iostreams, there could have been a more general solution based
on |locale|s and |facet|s.

Comment 31

[jag (Peter Annema)]

Moving to my netscape account, for now targetting at moz 1.0.

Comment 32

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 33

[jag (Peter Annema)]

-> 0.9.8

Comment 34

[jag (Peter Annema)]

-> 1.1

Comment 35

[Kai Lahmann (is there, where MNG is)]

for me both +2.5 and +.5 should end as +0, as it's simply invalid.

Are there really that much pages doing such nonsense?

Comment 36

[Håkan Waara]

CC bz who fixed a similar bug yesterday.

Comment 37

[Bill Mason]

*** Bug 165539 has been marked as a duplicate of this bug. ***

Comment 38

[Jeremy M. Dolan]

To whomever +/-'s the "blocking1.3b": Since this is listed in Netscape's
developer release notes, it's somewhat high profile and fairly reflective of
Gecko's IE compatibility.

Comment 39

[Boris Zbarsky [:bzbarsky]]

we _could_ just change ToInteger() to not skip over leading '.' chars... (and
instead to parse "t.6" as "0" instead of "6"...  Jag?  Thoughts?

Comment 40

[Hideo Saito]

Attachment: patch for converting the value of ".5" to 0

The following operations are required in order to test this patch.
1. run a 'rm xpfe/bootstrap/mozilla-bin' command.
2. run 'make' at root directory.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125208 [details] [diff] [review]
patch for converting the value of ".5" to 0

If nothing else, this only fixes one of the two ToInteger functions (there is
one on nsString and one on nsCString).	In any case, "done = PR_TRUE" means a
valid integer was found; that can't be what you want to happen here....

Finally, the string owner still needs to decide whether this change should even
be made; right now ToInteger is being pretty reasonable on invalid inputs, and
it's not clear that we want to change that.

Comment 42

[Vedran Miletic]

retargeting

Comment 43

[Michael Baffoni]

Shouldn't target be 1.6a (or any other build not yet cut)?

Comment 44

[Boris Zbarsky [:bzbarsky]]

OK, we just need a decision here from whoever knows this code.  Is the
suggestion in comment 39 acceptable?  If not, the DOM needs to stop using
ToInteger and roll its own function or something.

Brendan, who's the nominal owner for this stuff now?

Comment 45

[Brendan Eich [:brendan]]

Who owns string these days?  Darin has made himself a target.  Not sure he wants
to own obsolete ToInteger stuff, though.  Can't we fix this in the specific font
size attribute parsing code if we want to play it safe?

/be

Comment 46

[Boris Zbarsky [:bzbarsky]]

We could fix it in attribute parsing code in general.  Fixing it in font size
attribute parsing only is not really warranted, imo.

Comment 47

[Gabriel Sjoberg]

*** Bug 256174 has been marked as a duplicate of this bug. ***

Comment 48

[Hideo Saito]

Attachment: patch

I think that the the value containing a decimal point should be converted to an integer.

Comment 49

[Hideo Saito]

Hardware/OS -> All/All

Comment 50

[Hideo Saito]

<font size="+.5"> interpreted as <font size="5">

Comment 51

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the right thing to do is to fix attribute parsing in general, not just the "size" attribute of "font"...

Comment 52

[Hideo Saito]

Attachment: patch

The effect of this patch is that <font size="+05">,<font size="+5."> and <font size="+5 "> are interpreted as <font size="+5">, and <font size="+.5"> is interpreted as <font size="+0">.

Comment 53

[Boris Zbarsky [:bzbarsky]]

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

See comment 51.  Last jag and I talked about this (and he was thinking of doing this, iirc, which is why the bug is still assigned to him, if you will note) we decided that what we need is a StrictToInteger function or so on nsAString/nsACString, to be used in the various attr-parsing methods on nsAttrValue.

Comment 54

[Hideo Saito]

Attachment: patch

As to changes for the "size" attribute of "font", I think that the original numeric string should not be parsed directly by using |EnumTable kRelFontSizeTable|.
As to changes for |nsAttrValue::ParseIntWithBounds|, is this wrong position for attribute parsing in general?

Comment 55

[Boris Zbarsky [:bzbarsky]]

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

> I think that the original numeric string should not be parsed directly by using
> |EnumTable kRelFontSizeTable|.

I suspect you think wrong, but can you explain why you don't?

> is this wrong position for attribute parsing in general?

It's the right place, but your changes here don't make sense to me.  And should not be needed.  Again, what we should have is a new method on ns(C)String that does what we want here and just use it.  The content/ part of this patch should be a one-liner, as far as I can see.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Possibly several one-liners if we have several ToString() calls that should use the new method.

Comment 57

[Jesse Ruderman]

Hideo, are you going to make a new patch addressing bz's comments, or does this bug need a new owner?

Comment 58

[Jesse Ruderman]

HTML4 does not specify the parsing of <font size>, but HTML5 does.

http://www.w3.org/TR/html401/present/graphics.html#h-15.2.2

http://www.whatwg.org/specs/web-apps/current-work/#fonts-and-colors

Comment 59

[Makoto Kato [:m_kato]]

Attachment: patch

Comment 60

[Boris Zbarsky [:bzbarsky]]

Makoto Kato, can you please point me to a description of the parsing algorithm you're implementing here (ideally with a brief justification for why it's the right algorithm)?

Comment 61

[Makoto Kato [:m_kato]]

(In reply to comment #60)
> Makoto Kato, can you please point me to a description of the parsing algorithm
> you're implementing here (ideally with a brief justification for why it's the
> right algorithm)?

Boris, It is simple implementation of http://www.whatwg.org/specs/web-apps/current-work/#fonts-and-colors.

1. If first character is "+" or "-", use fast path using ParseEnumValue() at first.
2. If return error, analyze characters until it isn't digit (the range U+0030 DIGIT ZERO (0) to U+0039 DIGIT NINE (9)).
3. truncate after digit.
4. ParseEnumValue() again.  If parse error again, return error.  (maybe it is too big/small value)

Comment 62

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 63

[Boris Zbarsky [:bzbarsky]]

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

Sorry for the lag here, but I finally got a chance to sort through the spec. 

It looks like you're not in fact implementing that spec, since <font size=".5"> will continue to parse as <font size="5"> with your patch.  Followup bug, I guess?

>+      if ((ch == '+' || ch == '-')) {
>+          if (aResult.ParseEnumValue(aValue, kRelFontSizeTable))
>+              return PR_TRUE;

OK.  So this is where we used to fall through to ParseIntValue, which we don't want.  But with your new code, for the case of size="+.5", we have:

>+          // truncate after digit, then parse it again.
>+          PRUint32 i;
>+          for (i = 1; i < tmp.Length(); i++) {
>+              ch = tmp.CharAt(i);
>+              if (ch < PRUnichar('0') || ch > PRUnichar('9')) {
>+                  tmp.SetLength(i);

So this will call SetLength(1).

>+          return aResult.ParseEnumValue(tmp, kRelFontSizeTable);

And then we'll return false, right?

I guess that makes sense.  And we want this to handle the +2.5 cases.  I see.

r=bzbarsky with Truncate() used instead of SetLength().  I can update the patch to do that.

Comment 64

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/ed5fdfcd9e68

Please don't forget to file that followup on <font size=".5">!