Closed Bug 52019 Opened 20 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: m_kato)

References

(Blocks 1 open bug, )

Details

(Keywords: html5, testcase, Whiteboard: relnote-devel)

Attachments

(3 files, 4 obsolete files)

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.
Keywords: 4xp, testcase
Attached file testcase (obsolete) —
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.
Dividing up Claytons bugs to triage
Assignee: clayton → rods
Assignee: rods → harishd
This HAS to be a parser issue.
This is definitely not a parser problem. Parser just consumes attribute content 
and passes it over to the layout. Reassigning bug to waterson.
Assignee: harishd → waterson
-> team content
Assignee: waterson → nisheeth
Nominating for rtm
Status: NEW → ASSIGNED
Keywords: rtm
*** Bug 54949 has been marked as a duplicate of this bug. ***
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?
Marking [rtm need info] and re-assigning to harish.
Assignee: nisheeth → harishd
Status: ASSIGNED → NEW
Whiteboard: [rtm need info]
Attribtue value parsing does not happen in the parser. It happens in layout.
I will take a look into it anyway.
Status: NEW → ASSIGNED
As I suspected the problem seems to be in nsString::ToInteger(). 

Giving bug to Scott.
Assignee: harishd → scc
Status: ASSIGNED → NEW
See also bug 54142.
Status: NEW → ASSIGNED
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 :-)
Reporter said IE and NN ignore string after '.'. Mozilla also should
ignore it as an illegal value.
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();|
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?
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?
No comments/updates on this bug for 19 days now. Should fall off of the rtm radar.
Adding relnoteRTM keyword.
Keywords: relnoteRTM
Whiteboard: [rtm need info] → [rtm need info] relnote-devel
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.
rtm-
Whiteboard: [rtm need info] relnote-devel → [rtm-] relnote-devel
argh, thought this was my bug for some reason. restoring to original state.
Whiteboard: [rtm-] relnote-devel → [rtm need info] relnote-devel
scc, can we either fix this bug or take it off of the rtm radar?
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?
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|?
Keywords: rtm
Whiteboard: [rtm need info] relnote-devel → relnote-devel
Nominating for Mozilla0.9, so we can get this figured out once and for all.
Keywords: mozilla0.9
Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok
qa contact updated.
QA Contact: gerardok → bsharma
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.
Assignee: scc → jag
Status: ASSIGNED → NEW
Moving to my netscape account, for now targetting at moz 1.0.
Assignee: jag → jaggernaut
Target Milestone: --- → mozilla1.0
SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.
Component: HTML Element → Layout
QA Contact: bsharma → moied
-> 0.9.8
Target Milestone: mozilla1.0 → mozilla0.9.8
-> 1.1
Target Milestone: mozilla0.9.8 → mozilla1.1
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?
CC bz who fixed a similar bug yesterday.
*** Bug 165539 has been marked as a duplicate of this bug. ***
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.
we _could_ just change ToInteger() to not skip over leading '.' chars... (and
instead to parse "t.6" as "0" instead of "6"...  Jag?  Thoughts?
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 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.
Attachment #125208 - Flags: review-
retargeting
Target Milestone: mozilla1.1alpha → Future
Target Milestone: Future → mozilla1.1alpha
Shouldn't target be 1.6a (or any other build not yet cut)?
Flags: blocking1.6a?
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?
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
We could fix it in attribute parsing code in general.  Fixing it in font size
attribute parsing only is not really warranted, imo.
*** Bug 256174 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — Splinter Review
I think that the the value containing a decimal point should be converted to an integer.
Attachment #125208 - Attachment is obsolete: true
Hardware/OS -> All/All
Status: NEW → ASSIGNED
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: mozilla1.1alpha → mozilla1.9beta
<font size="+.5"> interpreted as <font size="5">
Summary: <font size="+.5"> interpreted as <font size="+5"> → <font size="+.5"> interpreted as <font size="5">
For what it's worth, the right thing to do is to fix attribute parsing in general, not just the "size" attribute of "font"...
Attached patch patch (obsolete) — Splinter Review
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">.
Attachment #203506 - Attachment is obsolete: true
Attachment #203520 - Flags: review?(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.
Attachment #203520 - Flags: review?(bzbarsky) → review-
Attached patch patchSplinter Review
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?
Attachment #203520 - Attachment is obsolete: true
Attachment #203666 - Flags: review?(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.
Attachment #203666 - Flags: review?(bzbarsky) → review-
Possibly several one-liners if we have several ToString() calls that should use the new method.
Hideo, are you going to make a new patch addressing bz's comments, or does this bug need a new owner?
Attachment #14333 - Attachment is obsolete: true
QA Contact: moied → layout
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
Assignee: jag → nobody
Keywords: html5
Priority: P3 → --
Target Milestone: mozilla1.9alpha8 → ---
Attached patch patch Splinter Review
Attachment #425409 - Flags: review?(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)?
(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)
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.
Status: ASSIGNED → NEW
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.
Attachment #425409 - Flags: review?(bzbarsky) → review+
Assignee: nobody → m_kato
http://hg.mozilla.org/mozilla-central/rev/ed5fdfcd9e68

Please don't forget to file that followup on <font size=".5">!
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.