35847 - [Patch] implied universal selector not using default namespace [NAMESPACE]Pseudo styles are not being resolved correctly.

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[rods (gone)]

if I have a rule:

select *:-moz-display-comboboxcontrol-frame {
  cursor: default;
  user-select: none;
  overflow:hidden;
	white-space:nowrap;
	background-color: inherit;
	color: inherit;
	text-align: inherit;
  border: none;
  padding-left:   4px;
  padding-right:  5px;
  padding-bottom: 1px;
  padding-top:    1px;
}

And my select contains a block frame that contains a text frame, the block will 
pick up this style if this rules is in the html document, but it won't pick it 
up from html.css.

Comment 1

[rods (gone)]

changing to M16 and beta2

Comment 2

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

Rules in html.css are at a different level of the cascade than rules in an HTML
document.  This may or may not be a bug.  A testcase would help...

Comment 3

[rods (gone)]

The best testcase is the combobox code I have been waiting days to check in, but 
everytime I can, the tree is either closed or I have to rebuild and miss it 
before it closes (like this morning).

Comment 4

[Marc Attinasi]

Hey Rod, does making these rules important make them work from html.css? 

I will get to this soon, but I was thinking it would be an easy thing for you to 
check...

Comment 5

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

I'm not sure if !important does anything at the UA level of stylesheets, but if
it does, I'd be very careful to see exactly what that is (since it's not defined
by the spec).  You might be looking at something that will override the author's
stylesheets, which is probably not what you want.

Comment 6

[Marc Attinasi]

!important will work in the ua style sheet - we are using it elsewhere for 
similar purposes. And it does override non-!important author-defined rules, 
hence the need to be very careful. I agree that it is not a good general 
practice, however we may use it for internal purposes, in cases where an 
author's style would not be affected (like to make the button part of a select 
'static' positioned: an author cannot put a button in a select on their own. 
There is on in there for framesets, forcing their display to be 'block' too, but 
I don't know why).

At any rate, I was looking more for additional information, not necessarily a 
solution (though a possible workaround to give me more time would be nice).

I wonder why the CSS2 spec restricts !important to user and author style 
sheets... 

Comment 7

[leger]

Putting on [nsbeta2-] radar.

Comment 8

[Marc Attinasi]

Pushing forward to M17: we have a temporary workaround for Rod's blockage for 
M16, however there is still a real problem here to fix.

Comment 9

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

rods noted on mozilla-layout-checkins:

# Site index #63 crashes in the layout of the combobox dropdown, this is
# the same as bug 36558. The code is actually checked in that fixes this.
# It cannot be "turned on" until Bug 35847 is fixed.
#
# http://bugzilla.mozilla.org/show_bug.cgi?id=35847
#
# If you want to turn on the new code to get by these errors, set the
# global variable kGoodToGo to PR_TRUE at the top of
# nsComboboxControlFrame.

Comment 10

[Marc Attinasi]

bug 33960 looks related, possibly a duplicate.

Comment 11

[Marc Attinasi]

Rods says this is not needed for Beta2 - his workaround is doing the job just 
fine. I think we should remove the nsbeta2 kwd and plan on addressing after 
beta.

Comment 12

[Marc Attinasi]

resetting severity and platform/OS

Also, I have been playing with this and I have a simplified testcase to start 
from. In the testcase is a SELECT with 3 options. I put a rule in the testcase 
that looks like:

select *:-moz-display-comboboxcontrol-frame {
 border: solid red 1px;
}

The red-border shows up (until you click on the select, at which time it gets 
whacked). If you put the rule into html.css instead, no red border.

I'll attach the testcase and keep trying to create one outside of SELECT's

Comment 13

[Marc Attinasi]

Attachment: Testcase: rule in document is OK, in html.css is not OK

Comment 14

[Pierre Saslawsky]

Two interesting things:
1) It works as-is in ua.css but not in html.css.
2) To make it work in html.css, remove the universal selector and add the child 
thingy as in: 
    select > :-moz-display-comboboxcontrol-frame {
     border: solid red 1px;
    }
or just remove the 'select' and the universal selector altogether:
    :-moz-display-comboboxcontrol-frame {
     border: solid red 1px;
    }

Now the *really* interesting thing is that if you remove the default namespace 
from html.css, your original declaration works... which raises the question to 
know whether the universal selector *always* fails to match when a default 
namespace is specified. Don't panic though: we have only three such instances in 
html.css.

(Legacy note: I looked into that problem only because I still live in the terror 
of bug 24390 which finally could be solved because an external contributor found 
by chance a syntax that made it work).

Comment 15

[Pierre Saslawsky]

To make it clear, this is a problem with the universal selector when a default 
namespace is specified. It could be in the CSSParser or in SelectorMatches(). I'd 
vote to put that bug under the beta2 radar again when a fix is in hand.

Comment 16

[Marc Attinasi]

Pierre, I'm not so sure the problem is as general as you have proposed.

I tried some simpler cases using universal selectors and they all work:

1) Universal selector, no pseudo styles
  DIV * P { color: red; } 
in html.css this DOES make the text red in:
  <div><div><p>Red Baby, Yea!</p></div></div>

2) Universal selector with pseudostyle
  DIV * :link { color: red; } 
does make the link red from html.css in:
  <div><div><a href="http://www.foo.com">Red Baby, Yea!</a></div></div>

3) universal selector and moz-pseudo
  DIV * :-moz-radio { background-color: red; } 
does make the radiobutton red fro html.css in
  <div><div><input type=radio name=rad1>Radio1</input></div></div> 

I'm having a hard time understanding what is different about the combobox 
control frame... Since the combobox control frame is getting the pseudostyle 
applied to *anonymous* content I'm wondering if that could be a clue as to why 
the default nemespace is an issue: maybe anonymous content does not have the 
same namespace as html content? SelectorMatches has this interesting chortcut 
that seems highly relevant:

  // Bail out early if we can
  if(kNameSpaceID_Unknown != aSelector->mNameSpace) {
    PRInt32 nameSpaceID;
    aContent->GetNameSpaceID(nameSpaceID);
    if(nameSpaceID != aSelector->mNameSpace) {
      return result;
    }
  }
but I never see the return statement hit for the 
:-moz-display-comboboxcontrol-frame selector. I'll have to get back at this with 
a fresh perspective.

Comment 17

[Marc Attinasi]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 18

[Hixie (not reading bugmail)]

Since Marc has put this down as Future, I'm taking it so that I can have a look
at it and try to narrow it down.

Comment 19

[Marc Attinasi]

Ian, may the *force* be with you.

Comment 20

[Zach Lipton [:zach]]

Removing nsbeta2 keyword.

Comment 21

[Daniel Veditz [:dveditz]]

Adding nsbeta2 keyword to bugs with nsbeta2 triage value in status field so the 
queries don't get screwed up

Comment 22

[Hixie (not reading bugmail)]

(Dave, this might affect you.)

Just looking at this, I think Marc is right. The "*:-moz-display-
comboboxcontrol-frame" is probably not in the HTML namespace. In html.css, that
selector would be equivalent to:

   html|select html|*:-moz-display-comboboxcontrol-frame 

So if the :-moz-display-comboboxcontrol-frame is not in the HTML namespace, it
would not match.

Food for thought - does this work in html.css? (untried):
   select *|*:-moz-display-comboboxcontrol-frame { ... }

Comment 23

[Hixie (not reading bugmail)]

I have found the problem, and it should be a easy fix.

If you have a simple selector with no element type selector, such as:
   .author
...or, say:
   :-moz-display-comboboxcontrol-frame 
...then according to the CSS2 spec, a universal selector is implied:
   *.author
   *:-moz-display-comboboxcontrol-frame 

According to the CSS3 namespace proposal, '*' only matches elements in the 
default namespace, if a default namespace is given. What WE are doing, is 
treating the _implied_ universal selector as '*|*' regardless of the presence
of a default namespace.

In addition to all this, the frame that :-moz-display-comboboxcontrol-frame 
matches is not in the HTML namespace, and in html.css the default namespace is
the HTML namespace.

This is why
   :-moz-display-comboboxcontrol-frame 
...works, but
   *:-moz-display-comboboxcontrol-frame 
...does not.

The bug is that we are treating the implied universal selector as matching all
namespaces. This also affects author stylesheets, as shown by the testcase.

Comment 24

[Hixie (not reading bugmail)]

Attachment: Simple and self explanatory testcase

Comment 25

[Hixie (not reading bugmail)]

Pierre, back to you.

Comment 26

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

Are you sure the *bug* isn't the other way around?  This should probably be
clarified by the WG, since the namespaces stuff is a draft.

Comment 27

[Hixie (not reading bugmail)]

Yes, I agree it should be cleared up.

However, I thought about this, and the way you posit is, IMHO, illogical.
This about how these:

   @namespace url(xhtml);
   p[author] {}
   p {}
   [author] {}
   * {}

Which would match the following?:

   <p author="TBL" xmlns="unknown"></p>

I would say none of them. Per the draft, the first two and the last one would 
not, and the third is debatable. Per your idea, the first two and the last would
not, but the third _would_. How is this useful? Seems wrong to me.

Comment 28

[rods (gone)]

marc, if I remember right we had to use :-moz-display-comboboxcontrol-frame
and NOT *:-moz-display-comboboxcontrol-frame because of a different style bug.

Comment 29

[karnaze (gone)]

Marking nsbeta3-. There are other more important nsbeta3+ bugs to work on.

Comment 30

[karnaze (gone)]

Marking nsbeta3- this time.

Comment 31

[Marc Attinasi]

bug 53796 is related, if not a dup...

Comment 32

[Pierre Saslawsky]

Reassigned to Marc, like bug 53796

Comment 33

[Hixie (not reading bugmail)]

BTW, the draft has been cleared up and now says what I proposed (since I wrote
that section of the spec).

Comment 34

[Marc Attinasi]

*** Bug 53796 has been marked as a duplicate of this bug. ***

Comment 35

[Marc Attinasi]

Moving to P1 - this is a nasty problem that needs to be fixed. We can
potentially get some performance benefits too, by restricting rule matching to
the relevant namespaces (i.e. keeping HTML rules out of XUL).

We do need to take care to ensure that the cover the cases where we actually
_want_ the rules to apply to all namespaces (like :link --> *|*:link)

Comment 36

[Marc Attinasi]

Attachment: Patch: fixes the CSSParser and also patches up html.css so that non-HTML rules are given the univesal namespace

Comment 37

[Marc Attinasi]

I have attached a patch that fixes teh CSSParser such that it now assigned the 
default namespace when the selector has an implied universal selector. Also, the 
patch has a first-stab at fixing up html.css: since there are rules in html.css 
that need to apply to all namespaces, we need some universal namespaces. It 
would probably be better to move those rules out of html.css and into a 
global.css file that has no default namespace. I am not sure where all of the 
pseudos in html.css need to apply, so I was a bit liberal in the application of 
the *|* for now.

Comment 38

[Marc Attinasi]

removed helpwanted kwd and added perf (since restricting namespaces will help 
style resolution performance)

Comment 39

[Hixie (not reading bugmail)]

attinasi: I'm guessing all the table pseudos will also need this, have you 
checked if CSS tables still still work in XML with your patch?

Comment 40

[Marc Attinasi]

No, I have not Ian - thanks, I will.

Comment 41

[Marc Attinasi]

Downgrading to P2, adding patch kwd

Comment 42

[Marc Attinasi]

Patch seems on track. Some final testing to complete still...

Comment 43

[Hixie (not reading bugmail)]

Marc: Is the attached patch up to date, or did you make some changes? If it's
up to date I'll apply it and Test Some Stuff for ya'.

Comment 44

[Marc Attinasi]

The patch is not complete. I added the wildcard namespace to the :table pseudos
too, and I'll attach that patch. coming up...

Comment 45

[Marc Attinasi]

Attachment: New patch: handles the :table pseudos too (previous patch is obsolete)

Comment 46

[Marc Attinasi]

New patch is good to test, Ian. Thanks, and let me know if you are having any
problems. BTW: you will probably have to break the patch into two files,
dropping them into the directory where the corresponding source is.

Comment 47

[Marc Attinasi]

Yo, wait - the parser patch is trash - sorry. Fixing now.

Comment 48

[Marc Attinasi]

Attachment: CORRECT new patch: last one was putting the new code into the wrong spot in the CSS parser (?)

Comment 49

[Marc Attinasi]

I just noticed that the location bar is not tall enough with this patch applied,
in Modern and Classic skin. Other text controls in the chrome have the same
problem (pref: home page location, for example) but HTML forms seem OK. I'll
check the xul-form css files for un-namespaced implicit universal selectors.

Comment 50

[Hixie (not reading bugmail)]

Ok, I'll look at this tomorrow when I get in.

Comment 51

[Hixie (not reading bugmail)]

ok, I'm building this now. (Not quite "when I get in" but...)

Comment 52

[Hixie (not reading bugmail)]

I found one problem: the scope of @namespace rules seems to span multiple
<style> blocks. See: 
   http://www.hixie.ch/tests/adhoc/css/selectors/namespace/005.xml
...for a test case. However this seems to be an already existing bug. I'll file
it separately once I've finished testing this patch.

Comment 53

[Hixie (not reading bugmail)]

So... I couldn't get XML based markup to break, and yet that means that the
following pseudo-classes were not important:
   :-moz-anonymous-block, :cell-content, :-moz-list-bullet
...however I suspect that's just because I don't know where to look -- they
are still used by the code (I checked).

I compiled with MathML enabled and had a play around with that but found no
regressions there. cc'ing rbs for his input though; this patch might affect him.

Comment 54

[Marc Attinasi]

Yea, I was thinking that I could put the good ol' *|* in front of ALL of the 
existing universal pseudos, to get us to parity with when the bug existed, but I 
don;t see how that helps us any performance-wise. The idea is to restrict the 
application of those selectors where possible. However, at this late date it 
might be safer to start there and then slowly peel off the universal namespace 
where we thik it might not need to apply.

Comment 55

[Marc Attinasi]

OK: chrome uses HTML textarea and input controls, and rules for those have to be 
put in teh HTML namespace to fix the address bar problem. I have modified 6 css 
files for XUL (chrome and skins) and will attach patch for those mods for 
review. That takes care of all of the problems that have been detected by this 
patch as far as I know.

Comment 56

[Marc Attinasi]

Attachment: PATCH for chrome/skin CSS files: textbox-input and textbox-textarea now in html namespace

Comment 57

[Marc Attinasi]

Moving out to Mozilla 0.9.1: patch is done but reviews are still pending and
time has run out. Also, this is not as critical as some other issues I have at
the moment.

Comment 58

[Marc Attinasi]

Moving P2 and P3 bugs to 0.9.2

Comment 59

[Hixie (not reading bugmail)]

We have a patch, it's easy to get reviewed... We really should push this before
the 0.9.1 freeze, come on! :-)

Comment 60

[Marc Attinasi]

I agree! I wonder how valid the patches are now. Maybe Daniel can help out???

Comment 61

[David Hyatt]

Seems like this could wait until 0.9.2.... 

Comment 62

[rbs]

This is a nice one to get in when appropriate. It reminds me of bug 21890 
(documents can style their scrollbars). I have several universal rules in 
mathml.css. These rules are defined under the MathML namespace:

@namespace url(http://www.w3.org/1998/Math/MathML); 

So in principle, they should only apply inside the <math>...</math> environment.

However, they get applied everywhere. Not only is this unexpected, but it also
means that the style resolution is slower since this suggests that all those 
tags outside <math>...</math> are checked against these rules. Hence in addition 
to correctness, there is a also performance issue here. Many precious cycles are 
wasted if all the universal rules in all stylesheets are checked everywhere, 
regardless their namespace.

Comment 63

[Marc Attinasi]

Reassigning to glazman in hopes that he can do something with it. In my list, it
would have to be pushed out since it is not a crasher :(

Comment 64

[Hixie (not reading bugmail)]

I strongly feel that we should check this in mozilla0.9.2.

Comment 65

[Marc Attinasi]

I totally agree. There is some data in bug 83482 suggesting that this help
performance enought to matter too, I think.

Comment 66

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

I'm going to take this and try to get it in this week.  It will be a major
performance impromevent to selector matching now that my first patch on
bug 83482 was checked in.

Comment 67

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

Attachment: above patches brought up to date, plus fix for bug 83839

Comment 68

[rbs]

r=rbs

Comment 69

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

I asked hyatt if he thought there were any other rules in chrome stylesheets
that would be broken by this, and he said the only ones should be the ones for
the textarea or input inside a textbox, which are fixed in the above patch, so
this should be fine.

Comment 70

[Marc Attinasi]

sr=attinasi for patchID 37262. David, I'm curious what testing you have done
with it. I'm guessing that Ian's previous testing is still relevant anyway.

Comment 71

[rbs]

I had this thought (re:the explicit listing of the rules) if it couldn't be 
one of these cases where it is OK to sacrifice a bit of speed for the benefit of
maitainance & correcness in the long run. I.e., in principle, if it is too
complex then it is easy to break (or make mistakes).

Comment 72

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

The thing is, it's a quirk, so for any new elements that we give margins there
wouldn't be pages that depend on the quirk.  (And it was really a rather *major*
performance issue on some pages -- most of the time in SelectorMatches.)

Comment 73

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

FWIW, the testing I've done has been:
 * using the browser
 * running through Daniel's selector test suite.  We pass all the tests except for: 
    + the ones of selectors we don't support
    + the ones caused by bug 75700
   since I have the patch to bug 83616 in my tree as well.

Comment 74

[Chris Waterson]

dbaron: if ``using the browser'' means you've done a best-effort at grovelling
through both modern and classic skins, then sr=waterson. (Alternatively, can we
grep through the skin CSS files for "^:" to find any pseudo-selectors that
might've been missed?)

Comment 75

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers).  

Comment 76

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

Patch is checked in, but not sure if all the issues in the bug are dealt with,
so leaving open for now.

Comment 77

[Charles Manske]

This fix has caused a bad side-effect for "editable" menulists, but only
in the MODERN theme.
Unfortunately, this is hard to test right now: Composer will use this widget
once the fix for bug 71743 is checked in. Currently, Wallet UI uses this:
Use "Tasks | Privacy and Security | Form Manager | View Stored Form Data"
to bring up a dialog that uses editable menulists. But there is
currently bug 62730, which prevents these widgets from appearing.
You can apply the patch on that bug to fix that temporarily so you can see
example of the following:

The "inputField" (HTML "input" widget, aka, XUL textbox) is now picking up
styles that are probably intended just for an HTML "input" element in the
browser, not what we'd like to see in the XUL widget. The "threeD" shadow is
thicker (2px), and this is causing the height of the content area of input
field to be too short, cutting off half the text that the user types.
Note that this problem does not occur for regular XUL textbox widgets.
Is this because the "html:input" in the menulist is (supposed to be) annonymous
content? (But maybe it's not *really* annonymous? See below...)
This is preventing the CSS defined for the menulist input widge
(class="menulist-editable-box") from being used.

Note that the root of this problem may be the difference between Modern and
Classic themes that shows up as other annoyances:
In the editable menulist in modern, after clicking in the inputField, if you
then use tab key, focus goes the menulist part of the widget, when it should
move focus out of the widget altogether. This doesn't happen with Classic.
Thus I think there's a bug so that child content of XBL widgets are not being
treated as truly annonymous *only in Modern!* I bet this is the cause of many
other focus-related problems!

See "xpfe/global/resources/content/bindings/menulist.xml" for definition of
menulist, editable="true".

Comment 78

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

Attachment: patch that should fix regression (not yet tested)

Comment 79

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

Oops...htat patch should say "html|*", not "html|" in 3 places.  It's very hard
for me to test right now since I'm using a slow remote X connection...

Comment 80

[Charles Manske]

Bingo! I actually tried
html|input.menulist-editable-text
before seeing your last comment.
Which is more efficient for CSS rules matching: "html|input" or "html|*" ?

Comment 81

[Charles Manske]

I tested both patterns ("html|input" ane "html|*") and both work
r=cmanske
I think using "html|input.menulist-editable-text" is preferable; I'll let the
CSS experts decide.
Let's get this checked in soon!
Thanks, though I still wish we could resolve the deeper problem that I described
(the difference between Modern and Classic's events handling.)

Comment 82

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

Attachment: the real patch to fix the regression

Comment 83

[rbs]

What about also using 'input' since cmanske hinted earlier that the classes
under consideration are only meant for the 'input' element? (i.e., 'input'
should have been specified there in the first place).

Comment 84

[David Hyatt]

sr=hyatt

Comment 85

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

> What about also using 'input' since cmanske hinted earlier that the classes
> under consideration are only meant for the 'input' element? (i.e., 'input'
> should have been specified there in the first place).

It doesn't really matter -- I think the only substantive reason to prefer html|*
is that checkin the tag name requires one more test (i.e., probably two
instructions).  More importantly, though, it's the going style in our chrome
stylesheets.

Comment 86

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 87

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

Fix for regression checked in, 2001-06-12, 16:07 PDT.

Comment 88

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

Based on Pierre's comments above, I think the original problem Rod reported was
related to the namespaces issue that was fixed here.  Marking this bug fixed
since the fix to the namespaces *bug* has been checked in.

Rod, is your original problem fixed?

Comment 89

[Hixie (not reading bugmail)]

verified per my testcase above