290081 - font size can override <hN> tags if more than one <font> is opened incorrectly

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Florian R. Plachinski]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: ver 1.0.1 and 1.0.2

Firefox doesn't display text modified by <Hn>text</Hn> codes correctly. Found 
by accident when writing pages in code. Display file link to demonstrate. IE 
displays text properly formatted. Netscape 4.72 also displays text correctly.

Reproducible: Always

Steps to Reproduce:
1. use the <H1>text</H1> html code on a web page. 
2. display the page. 
3. Text won't display in LARGER font as it should.

Actual Results:  
affects all pages.

Expected Results:  
displayed the text in a larger typesize.

Comment 1

[Jo Hermans]

Ofcourse we support headings. But you left a <font size="4"> open at the top ...
That's why everything is displayed in size 4.

Comment 2

[Jo Hermans]

see also bug 79549

Comment 3

[Florian R. Plachinski]

The <font size="4"> open at the top of the document is for the ENTIRE document 
with the </Font> close at the bottom of the page. Open the file in IE and 
you'll see that it formats correctly!

Comment 4

[Jo Hermans]

(In reply to comment #3)
> The <font size="4"> open at the top of the document is for the ENTIRE document 
> with the </Font> close at the bottom of the page. Open the file in IE and 
> you'll see that it formats correctly!

But it also influences the headings ...

Comment 5

[Jo Hermans]

Attachment: your test-file, with 4 headings inserted at the top

I've added 4 headings to your testfile, just before the <font> tag. Each
heading gets displayed correctly.

Comment 6

[Josh Birnbaum]

"The <font size="4"> open at the top of the document is for the ENTIRE document"  

HTML forbids this.  Any block-level element causes the <font> to be implicitly
closed.  Mozilla tries to "do the right thing" by reopening the <font> later
on--this is known as "residual style" and helps fix up web pages with invalid
HTML code.

In this case you end up with Mozilla seeing something like: <h1><font
size="4">text</font></h1> so whatever size is implied by the <h1> is lost.

You can use http://validator.w3.org/ to see that the URL you provided has
invalid HTML in it.

Comment 7

[Josh Birnbaum]

Over to parser.  Probably a wontfix, I don't think residual style is being
tweaked anymore.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Taking. The problem here is that there are *two* <font> tags running around. The
code that was added for bug 77352 only checks the immediate parent tag to
special-case this, so it's still broken.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This just makes us check the whole context stack instead of the
previously-opened tag when determining whether or not to use the magical
attribute.

I wonder if we could do this only for <font> instead of all residual style
tags...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180559 [details] [diff] [review]
patch v1

So how often do we get into the case where we need the |isHeadingOpen| boolean?
 If it's not that common, perhaps we should do the HasOpenTagOfType() check
on-demand and have a boolean indicating whether we've done it?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I'm not sure what you mean. This only happens when there's a (malformed) inline
element trying to contain a <hN> tag that has a size applied to it. We need the
isHeadingOpen boolean whenever we're opening residual style tags (to make sure
their size doesn't override the heading's).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Looking at the code, it looks like we only use the isHeadingOpen boolean in the
following conditions:

1)  Something on the stack has an mUseCount == 1.
2)  The same something can contain aChildTag.

So the question is, if we get to the line where we do

+          PRBool isHeadingOpen = HasOpenTagOfType(kHeading, *mBodyContext);

in that patch, what is the likelhood of hitting the line where we do:

+                if(isHeadingOpen) {

? If it's close to 1, this is fine; if it's close to 0 then perhaps we should
lazily set the boolean.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180559 [details] [diff] [review]
patch v1

r=bzbarsky per discussion on irc

Comment 14

[rbs]

Comment on attachment 180559 [details] [diff] [review]
patch v1

sr=rbs
+  while (count-- > 0) {

To speedup understanding, I would have preferred (in this context) 
+  while (--count >= 0) {
but...

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 180559 [details] [diff] [review]
patch v1

This is a small fix that simply extends a quirk fix to more cases, seeking
approval for 1.8b2.

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 180559 [details] [diff] [review]
patch v1

a=asa

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.