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

RESOLVED FIXED

Status

()

RESOLVED FIXED
18 years ago
8 years ago

People

(Reporter: jruderman, Assigned: m_kato)

Tracking

(Blocks: 1 bug, {html5, testcase})

Trunk
html5, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: relnote-devel, URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Keywords: 4xp, testcase
(Reporter)

Comment 1

18 years ago
Created attachment 14333 [details]
testcase

Comment 2

18 years ago
Created attachment 14436 [details]
Updated the output to reflect the difference between "0.5" and ".5"

Comment 3

18 years ago
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

Updated

18 years ago
Assignee: rods → harishd

Comment 5

18 years ago
This HAS to be a parser issue.

Comment 6

18 years ago
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

Comment 7

18 years ago
-> team content
Assignee: waterson → nisheeth

Comment 8

18 years ago
Nominating for rtm
Status: NEW → ASSIGNED
Keywords: rtm

Comment 9

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

Comment 10

18 years ago
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

18 years ago
Marking [rtm need info] and re-assigning to harish.
Assignee: nisheeth → harishd
Status: ASSIGNED → NEW
Whiteboard: [rtm need info]

Comment 12

18 years ago
Attribtue value parsing does not happen in the parser. It happens in layout.
I will take a look into it anyway.
Status: NEW → ASSIGNED

Comment 13

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

Giving bug to Scott.
Assignee: harishd → scc
Status: ASSIGNED → NEW
(Reporter)

Comment 14

18 years ago
See also bug 54142.

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 15

18 years ago
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

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

Comment 17

18 years ago
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

18 years ago
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

18 years ago
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

18 years ago
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
(Reporter)

Comment 21

18 years ago
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

18 years ago
rtm-
Whiteboard: [rtm need info] relnote-devel → [rtm-] relnote-devel

Comment 23

18 years ago
argh, thought this was my bug for some reason. restoring to original state.
Whiteboard: [rtm-] relnote-devel → [rtm need info] relnote-devel

Comment 24

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

Comment 25

18 years ago
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

18 years ago
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

Comment 27

18 years ago
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

Comment 29

18 years ago
qa contact updated.
QA Contact: gerardok → bsharma

Comment 30

17 years ago
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

Comment 31

17 years ago
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

Updated

17 years ago
QA Contact: bsharma → moied

Comment 33

17 years ago
-> 0.9.8
Target Milestone: mozilla1.0 → mozilla0.9.8

Comment 34

17 years ago
-> 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?

Comment 36

16 years ago
CC bz who fixed a similar bug yesterday.

Comment 37

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

Comment 38

16 years ago
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?

Comment 40

15 years ago
Created attachment 125208 [details] [diff] [review]
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 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-

Comment 42

15 years ago
retargeting
Target Milestone: mozilla1.1alpha → Future
Target Milestone: Future → mozilla1.1alpha

Comment 43

15 years ago
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.

Comment 47

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

Comment 48

13 years ago
Created attachment 203506 [details] [diff] [review]
patch

I think that the the value containing a decimal point should be converted to an integer.
Attachment #125208 - Attachment is obsolete: true

Comment 49

13 years ago
Hardware/OS -> All/All
Status: NEW → ASSIGNED
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: mozilla1.1alpha → mozilla1.9beta

Comment 50

13 years ago
<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"...

Comment 52

13 years ago
Created attachment 203520 [details] [diff] [review]
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">.
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-

Comment 54

13 years ago
Created attachment 203666 [details] [diff] [review]
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?
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.
(Reporter)

Comment 57

11 years ago
Hideo, are you going to make a new patch addressing bz's comments, or does this bug need a new owner?
(Reporter)

Updated

11 years ago
Attachment #14333 - Attachment is obsolete: true
QA Contact: moied → layout
(Reporter)

Comment 58

9 years ago
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 → ---
(Assignee)

Comment 59

9 years ago
Created attachment 425409 [details] [diff] [review]
patch
(Assignee)

Updated

9 years ago
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)?
(Assignee)

Comment 61

9 years ago
(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
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.