24390 - html:input styles in global.css are ignored

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[hangas]

For about a month, html:input styles in global.css have been ignored. Instead the
styles defined in html.css are used.  This includes all types of html:input
(buttons, checkboxes, radio buttons, text input fields, etc.).  Other html styles
in global.css are working fine.  The problem first appeared on a weekend that had
landings by hyatt with code that changed how global.css is loaded, and by ben who
landed changes to global.css and html.css.  This bug blocks development on a
number of bugs in my list.  I have not done much testing on platforms besides
Mac.

Comment 1

[Brendan Eich [:brendan]]

Why is this assigned to me?  Reassigning to hyatt.

/be

Comment 2

[David Hyatt]

Got a specific example? I tried a couple of changes, and they all worked.

Comment 3

[Pierre Saslawsky]

I'm going to forward to hyatt a couple of mails that we exchanged with hangas on
that problem last month.

Comment 4

[hangas]

I just tried a simple test on my Mac with a debug build from today, changing:
html|input[type=checkbox]:hover { border: 1px solid black; }
to:
html|input[type=checkbox]:hover { border: 1px solid red; }
in global.css and it did not work.  The border was still black.  In fact it was 
2 pixels of black, as it is defined in html.css. 

Comment 5

[David Hyatt]

Ummm.... 

I'm going to suggest something, and because I think I'm right, you're going to 
be really irritated when you find out why this wasn't working. :)

Attribute values are case-sensitive when used anywhere, even in CSS.  That means 
that a Web page that declared a text field by saying:

<INPUT type=CHECKBOX>

would not have the following rule applied:

html|input[type=checkbox] { ... }

You'd need to say

html|input[type=CHECKBOX] { ... }

In fact, attribute names may even be case-sensitive as far as CSS is concerned. 
I'm not sure.  

My guess is that you're using as your test case HTML pages that have 
attributes/tags/etc. etc. declared in a different case, and this is causing the 
rules to not match.

Comment 6

[David Hyatt]

Ben, buddy, pal.  Merry Christmas!  This one's all yours to verify.  If after 
further inspection it still looks like a C++ bug, give it back to me.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

I'm seeing this problem on windows too. I've changed the CSS in global and used 
the upper case naming, and it still does not have any effect. 

Comment 8

[David Hyatt]

This appears, then, to be a bug in CSS's import code.  It might also have to do 
with the namespace declarations (maybe CSS is getting confused about that).  
Let's get more test cases and see, but for now, I suspect there is a real CSS 
bug here.

Comment 9

[hangas]

Based on a suggestion by hyatt, I created  a global.css file that contained only:

input[type=checkbox] {
  border: 1px solid red;
  background-color: white;
}

nothing else...no comments, no namespace, no nothing.  This style is ignored for 
the checkboxes.  Simply bring up a dialog with checkboxes (e..g. Preferences) and 
you will see that they have a two pixel (not one) border than is inset rather 
than red, and the background is gray rather than white.  I hope this helps narrow 
the problem down because I would really like to land my widget changes to get our 
product to finally look like the spec.

Comment 10

[hangas]

Changing platform to All based on further testing.  Setting to Critical since we 
cannot move on a number of issues until this is fixed.

Comment 11

[rickg]

Pierre -- this bug concerns me. Can you take a quick look at tell me what your 
diagnosis turns up? Is it trivial or not?

Comment 12

[Pierre Saslawsky]

In response to hyatt in his comments from 2000-01-24 23:14, I'm going to suggest 
something too, and because I think I'm right, everybody's going to be really 
irritated when they find out why this wasn't working. :) Here it is...

I added the following line at the end of ua.css and it worked (Mac + Win):
@import url(resource:/chrome/global/skin/default/global.css);

Was it really all we had to do?

Comment 13

[David Hyatt]

Nope.  You can't do that.  The global skin file cannot be loaded explicitly by 
the user agent style sheet.

Comment 14

[Pierre Saslawsky]

Fine... I'm seeing that style works correctly when the file is visible and not 

overriden (which is what I did when I included it in last position in ua.css). It 

means that it's an XPToolkit/skins issue. Reassigned to hyatt.

Comment 15

[David Hyatt]

You can't juggle around the import order and then claim that CSS has no bug.  
There is clearly a bug here related to the order of importation or perhaps with 
namespacing in imported files, so I don't believe this is an XPToolkit/skins 
issue.

Another thought that occurred to me is that perhaps something in ua.css is being 
incorrectly labeled as being more important than rules loaded by subsequent 
style sheets.  This also wouldn't show up if you import global.css into ua.css 
directly, since then it is part of the user agent style sheet and might have 
equal importance.

The point here is this: we have a perfectly well-formed (as far as I can tell) 
test case in which rules are not being matched that should be.  Just because 
you're juggling the import order around and making those rules be matched does 
not mean that there's not a bug here.

If it's a lack of test cases that's scaring you off, I'm sure hangas would be 
happy to distill it down to a tiny global.css and a tiny html.css (e.g., one 
rule apiece in each) that would demonstrate the bug.

Comment 16

[David Hyatt]

Nominating as a beta1 stopper, since this bug blocks us from implementing the 
"chrome" Netscape look and feel for HTML widgets.

Comment 17

[rickg]

Adding rickg to cc list.

Comment 18

[Pierre Saslawsky]

I'll work on it some more but I based my conclusion on hangas' comments from 
2000-01-25 17:53 saying that a simple style rule in global.css ("nothing 
else...no comments, no namespace, no nothing") was still not applied. My idea was 
that global.css either wasn't included at all, either was overriden by html.css, 
and the fact that it works when it's included in ua.css (even though it's not the 
right thing to do) seemed to support this idea.

I understood from a previous mail from hangas that global.css was included using 
"@import url(chrome://global/skin);". Is this correct? It would be a good point 
to start.

Also, what is the testcase you mentioned? Any little time saver is appreciated 
these days.

Comment 19

[Pierre Saslawsky]

Interestingly enough, I just received a bug that mentions problems with linked 
style sheets: bug 24819. Still, thanks (hyatt? hangas?) for answering the 2 
questions above.

Comment 20

[hangas]

Yes, @import url(chrome://global/skin/); is in every component's css file.

The testcase was the one from 1-25-00 17:53, but here is another:

input[type=checkbox] {
  border: 1px solid red;
  background-color: white;
}

select {
  border: 1px solid red;
  background-color: green;
}

In this test, the input style is ignored but the select style is applied.  Proof 
(in a simple case) that the global.css file is loaded but something is broken.

Comment 21

[leger]

Putting on PDT+ radar for beta1.

Comment 22

[Simon Fraser [no longer active]]

This sounds like something that rods needs to fix.

Comment 23

[Sjoerd Visscher]

I've found it!
this works:
html|input[type=Checkbox]
html|input[type=Radio]

A little javascript gave the answer.
Somehow the type value is capitalized.

Now bring on the new widgets! I'm curious!

Comment 24

[hangas]

Well done sjoerd, so now we know the source of the problem but the styles should 
not use "Checkbox" they should be "checkbox".  I checked the xul and we use lower 
case in the xul but the DOM is built with an upper case C (changing checkbox to 
Checkbox).  Maybe now that we know the DOM is built this way we can solve the 
problem.

Comment 25

[David Hyatt]

Dude, I am nominating you as friend of the tree.  I'd never have caught this.

Comment 26

[Pierre Saslawsky]

Actually this is not the right fix but I would have never found it without you 
putting us back on track. You're my friend-of-the-tree too. Thanks a bunch!

The problem is that the DOM canonizes attribute strings; it was checked in by 
vidur on 99-12-21 and it broke SelectorMatches() in nsCSSStyleSheet.cpp. The 
correct fix is to do case-insensitive string comparisons in SelectorMatches() 
when we have a HTML content, in conformance to the HTML 4.0 specifications on
http://www.w3.org/TR/html4/intro/sgmltut.html#idx-attribute-7

Fix checked-in.

Comment 27

[leger]

Updating QA Contact.  desale, can you verify please.

Comment 28

[Christine Hoffman]

Attachment: HTML file to verify fix

Comment 29

[Christine Hoffman]

Using 2/17 build with testcase provided, verified fixed.