95336 - position CSS not recognized without units when element not in document

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, enhancement)

Description

[Ryan Frishberg]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows 98; Win 9x 4.90)
BuildID:    2001080110

When an element is in the document, when you change a CSS property like top or 
left without a unit, it defaults to "px"

However, when an element is not in the document, and you change a CSS property, 
such as top or left without the unit, the value is not assigned at all.

Reproducible: Always
Steps to Reproduce:
1.  Dynamically create an element with document.createElement()
2.  Assign it's "top" CSS property to a number with no unit appended

Actual Results:  No value is given to the "top" CSS property for that element

Expected Results:  "px" should be appended to the value you tried to assign to 
that element's "top" CSS property, and it should be assigned.

Comment 1

[Greg K.]

Since it's not legal to leave units off CSS values (please, don't actually make
web pages that do this), this is an enhancement request. (I presume it works
fine when the units are included.)

Comment 2

[Ryan Frishberg]

Because I assume this should have worked (if they have it working in such a way 
in the document), I assume it's more of a bug that it doesn't work while not in 
the document.  However, the whole thing (not including units) is an enhancement.

Comment 3

[Hixie (not reading bugmail)]

The bug will be that we are getting the quirks mode from the document of the
node instead of getting the document of the nodeinfo of the node. Should be
relatively easy to fix.

Reassigning to pierre -- pierre, you should be able to fix this in a couple of
minutes, it's just a matter of finding the part which does the unit-assumption
and then using the code I used in bug 93371 to get the quirks mode.

Comment 4

[Pierre Saslawsky]

Ian, Johnny: please review

Comment 5

[Pierre Saslawsky]

Attachment: patch

Comment 6

[Pierre Saslawsky]

Attachment: same testcase as the URL above but in quirks mode

Comment 7

[Hixie (not reading bugmail)]

Shouldn't the check be inside the |else| clause after the

    result = NS_NewCSSParser(&cssParser);

...line? At the moment, your patch would do weird things to the existing parser
if for some reason the mode had been manually switched.

Comment 8

[Pierre Saslawsky]

I hesitated to do it that way but the thing that convinced me otherwise is that 
it's how GetParserFor() works already:
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/
nsCSSLoader.cpp#558

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Fixing database corruption caused by bug 95743.  Pay no attention to the man
behind the curtain.

Comment 10

[Johnny Stenback (:jst)]

Do you really wanto call SetQuirkMode() if we're in strict mode? Changing:

+            cssParser->SetQuirkMode(eDTDMode_strict != mode);

to:

+            if (mode != eDTDMode_strict) {+             
cssParser->SetQuirkMode(PR_TRUE);
+            }

would avoid the call which I'm guessing is not needed if we're in strict mode, no?

Your call, sr=jst either way.

Comment 11

[Pierre Saslawsky]

I'll leave the code as-is because the real problem is that we don't do similar 
initializations when we create a parser.  In CSSLoaderImpl::GetParserFor(), 
regardless of whether the parser is created or re-used, we always call:
    (*aParser)->SetCaseSensitive(mCaseSensitive);
    (*aParser)->SetQuirkMode(mNavQuirkMode);
    (*aParser)->SetCharset(mCharset);

I am pretty much sure we should do the same in the other places where we have 
NS_NewCSSParser(), provided these 3 parameters can be known:
  http://lxr.mozilla.org/seamonkey/search?string=NS_NewCSSParser

I'll look into it and see if it deserves opening a separate bug.

Comment 12

[Pierre Saslawsky]

-> 0.9.4 since there is a fix (r=ianh/sr=jst)

Comment 13

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

a=dbaron on behalf of drivers

Comment 14

[Hixie (not reading bugmail)]

Er, for the record, I never did say r=ianh. But whatever.

Pierre: Did you determine if a bug had to be opened for those issues you raised?

Comment 15

[Pierre Saslawsky]

Not yet.  I guess I'll check in this small patch and move the bug to 0.9.5.

Comment 16

[Pierre Saslawsky]

Fix checked in nsGenericHTMLElement.cpp.  Moving the bug to 0.9.5 to check what I 
described on [2001-08-17 14:45].

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK...  Per mail from Pierre taking this bug.

Once the patch to bug 99797 is checked in I'll work on this (the code involved
is the same code and at the moment I'd just generate CVS conflicts with myself)....

Here's what I've found so far:

1)  Whenever we have a document (which we should get from the nodeinfo) we
    should be able to get a CSS loader.  Having a document but no css loader is
    worthy of asserting and returning an error (per conversation with jst).

2)  Getting a parser from the CSS loader will set the quirks mode and
    case-sensitivity correctly.  It will _not_ set the charset correctly unless
    the charset of the CSS loader is first set.

3)  nodeinfo can have a null document pointer if the document that created an
    object no longer exists.  In this case we should just create a new css
    parser using NS_NewCSSParser.  If we do this we have no good way of setting
    its charset, quirkiness, or case-sensitivity (all that information comes
    from the document).  If we're in HTML for a fact, we can set the
    case-sensitivity, but nothing else.

4)  The CSS loader holds on to charset values from the last sheet that loaded
    over http.  So the charset value set on a parser by the CSSLoader cannot be
    trusted.  Style sheets should remember their own charset values and set them 
    on the CSS parser in CSSOM operations on CSS rules.

5)  Similarly, elements whose style attribute is being modified should have a
    way of setting the default inline style charset for the document on a
    parser.  Again, per conversation with jst, it seems like a good idea to add
    a getInlineStyleParser to nsICSSLoader.  The CSS loader can then ask the
    document for the inline charset, do alias translation on it as it already
    does for sheets loaded over http, and cache it for future use.

So I'll hopefully be attaching a patch implementing this within a few days of
bug 99797 getting checked in.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: first-stab patch at getting charsets right

Comment 19

[Boris Zbarsky [:bzbarsky]]

The patch I attached tries to get the charsets and other stuff right.  In
particular:

1)  Style sheets store their own charset 
2)  The CSS loader caches the inline style charset
3)  We get the css loader from the right document

Possible issues:

1)  There is code duplication between ParseDeclaration and ParseProperty.  I can
    create a helper function to reduce the code duplication, but there was a
    thought to try to handle correctness and simplification separately on this
    one.  This patch is trying for correctness.  Pierre, if you feel that I
    should just go ahead and get rid of the code duplications, let me know.
2)  Should the CSS loader check that the default inline style content type is
    indeed text/css before grabbing the charset from it?  It seems to me that
    there is no way that function can get called if that content type is not
    text/css

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, one more note. That patch depends on the patch in bug 102096....

Comment 21

[Pierre Saslawsky]

1) Go ahead and simplify.
2) If it's not text/css, I think we can't even have a cssLoader - so no need to 
check.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Cleaned-up patch

Comment 23

[Boris Zbarsky [:bzbarsky]]

Pierre, what do you think about that one?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 51432 [details] [diff] [review]
Cleaned-up patch

This doesn't do quite the
right thing as far as nulling
out pointers.  Attaching better
patch.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that nulls things out correctly

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch based on some style comments from jst

Comment 27

[Boris Zbarsky [:bzbarsky]]

pushing out.  Hopefully I can get it all reviewed and stuff during the freeze. 
:)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 52136 [details] [diff] [review]
Patch based on some style comments from jst

OK.  More talking to jst.  We _can_ use namespaces to tell
whether we're in XHTML.  Though that stuff is all pretty
messed up.... :)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do case-sensitivity better in cases without document.

Comment 30

[Pierre Saslawsky]

The latest changes make the parser case-sensitive when we are in HTML.  It should 
be the opposite.  Note that NamespaceEquals() returns 'true', not NS_OK, when the 
strings are equal.

Comment 31

[Boris Zbarsky [:bzbarsky]]

The nodeinfo has kNameSpaceID_None as its namespace if we are in HTML.  It has
kNameSpaceID_HTML if we are in XHTML (or so jst says).  So the code as written
works correctly.

Yes, this is incredibly broken.  Yes, this means the namespace in the nodeinfo
does not match the namespace the node returns when asked for its namespace. 
That's why I added the kNameSpaceID_XHTML define.  With that, once the mess in
nsNameSpaceManager gets sorted out this will still continue working.

Comment 32

[Pierre Saslawsky]

Boris: do you have any testcases for this bug?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Not yet... I've been working on generating some, but I don't know much about
charsets to be truthful.  Any help on that would be greatly appreciated.

I can attach some testcases for quirkiness and case-sensitivity.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase: quirky HTML

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase: strict HTML

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: XHTML as xml

Comment 37

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

What's the bug with charsets that you're trying to fix, and how does this do it?
 I'd think the character encoding of the HTMLCSSStyleSheet ought to be handled
by the HTML parser, I don't see how the HTMLStyleSheet would have issues
(although what there would be ought to be handled by the HTML parser), and the
CSSStyleSheet's character encoding ought to be handled by the
UnicharInputStream.    Why should CSSOM operations depend on the charset? 
Shouldn't the scripting language or the markup language that contains the script
already have converted everything to Unicode?  Do you have testcases in various
charsets and with various @charset rules to show that this is the right thing to do?

Comment 38

[Boris Zbarsky [:bzbarsky]]

OK. I seem to have had a slightly wrong impression of how charsets work in the
DOM.  A few questions just to clarify things:

1)  what's the point of
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#3200
 (getting the Content-Style-Type charset in
nsGenericHTMLElement::ParseStyleAttribute)?  That seems like a fairly expensive
operation that we do every time for no reason.

2)  I've looked more carefully and the CSS parser does not ever use the charset
that's set on it.  Is there a reason to set this charset at all?

All the charset stuff in this patch is aimed at addressing #2 (setting the
charset on the parser to something appropriate), which I assumed was necessary
due to the code cited in #1.  If it is not in fact necessary, we should just
clean this code up not to do all this extra pointless work.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Ok.  Looks like the code in nsGenericHTMLElement was checked in as part of the
fix in bug 63502.

The important part of that fix in that file was that it changed 

-      isCSS = styleType.EqualsIgnoreCase("text/css");
+      isCSS = styleType.EqualsIgnoreCase("text/css", 8);

I don't see the reason for adding the charset-parsing code there (the result is
never used, as far as I can tell).

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 52189 [details] [diff] [review]
This should do case-sensitivity better in cases without  document.

OK.  Talked to dbaron a bit .  The charset stuff in this patch is pointless
since all that data is already in UCS2 by the time we get to it. I'll attach a simplified patch.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to just simplify getting of parsers (and handle case-sensitivity a little more correctly)

Comment 42

[Boris Zbarsky [:bzbarsky]]

OK.  That patch should me much more sane.  Reviews?

Comment 43

[Johnny Stenback (:jst)]

Comment on attachment 53516 [details] [diff] [review]
Patch to just simplify getting of parsers (and handle case-sensitivity a little more correctly)

sr=jst

Comment 44

[Pierre Saslawsky]

Comment on attachment 53516 [details] [diff] [review]
Patch to just simplify getting of parsers (and handle case-sensitivity a little more correctly)

r=pierre

Comment 45

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks, all, especially dbaron for his advice and help.