Open Bug 436170 Opened 16 years ago Updated 2 years ago

CSS:Adjacent sibling selectors don't work on elements added via bindings

Categories

(Core :: CSS Parsing and Computation, defect)

defect

Tracking

()

People

(Reporter: morac, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

According to the Dom Inspector, the urlbar contains 3 elements: identity-box, a textbox-input-box and the urlbar-icons.  I decided to implement an Internet Explorer type coloring system using the userChrome.css file using the following rules:

#identity-box.verifiedDomain ~ * {
  background-color: #F5F6BE !important;
  color: #000000 !important;
}

#identity-box.verifiedIdentity ~ * {
  background-color: lightgreen !important;
  color: #000000 !important;
}


This resulted in the urlbar-icons element changing color, but not the textbox-input-box element.  The textbox-input-box element is added via XML binding.  So even though it is considered a sibling in the DOM, the CSS style is not applied.

Reproducible: Always

Steps to Reproduce:
Add the following to userChrome.css and restart Firefox.

#identity-box + hbox {
  background-color: #F5F6BE !important;
  color: #000000 !important;
}
Actual Results:  
urlbar-icons in urlbar turns yellow

Expected Results:  
textbox-input-box in urlbar should turn yellow
Hey...
I was brought to here, from your post on mozillaZine forums:
http://forums.mozillazine.org/viewtopic.php?p=3388784#3388784

I can confirm what you describe, happens when you try out the code. 
I'm on Windows XP, Firefox 3 RC2:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906
Firefox/3.0
I'm no coder, but to me it sounds that your argument is reasonable. It should
be possible to have different colored locationbars depending the level of https
security.

I have also uploaded a screenshot showing how it looks with the code enabled.
<= only the 'bookmark star's background has been colored.

But when it all comes down: all I am confirming, is that what you say happens,
does happen.
Attached image illustration of how it should look (obsolete) —
With https://bugzilla.mozilla.org/show_bug.cgi?id=448939 landed, this were the only way to touch these elements at urlbar.
Maybe it helps... I've noticed the sibling selector just doesn't work if one of them (or both) is an accesible node.
Attached file testcase
reproduced on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre, couldn't find duplicates, confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: testcase
Version: unspecified → Trunk
It seems that switching to ChildIterator fixes this bug. Looks like this is the right thing to do, too, but please check.
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Attachment #360016 - Flags: superreview?(bzbarsky)
Attachment #360016 - Flags: review?(bzbarsky)
Comment on attachment 360016 [details] [diff] [review]
make adjacent selector matching code XBL-aware

Given <A/><anonymous node/><B/>, does this handle A + B correctly?
(In reply to comment #9)
> (From update of attachment 360016 [details] [diff] [review])
> Given <A/><anonymous node/><B/>, does this handle A + B correctly?

Could you define "correctly"?

I think this bug could use a description of what the expected behavior is (and agreement about that description).
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 360016 [details] [diff] [review] [details])
> > Given <A/><anonymous node/><B/>, does this handle A + B correctly?
> 
> Could you define "correctly"?

I would expect it to match B, but I'm not sure if that's what http://www.w3.org/TR/xbl/#selectors says.
According to <https://developer.mozilla.org/en/XBL/XBL_1.0_Reference/Anonymous_Content#Selectors_and_Scopes>, "style rules using the child, descendant or sibling selectors transparently cross binding scopes and operate on the altered and original content models."

So if the original content model is <A/><B/>, A+B should match B.
I feel pretty strongly that any content that's actually in the document needs to match or not match based on its siblings in the document, ignoring any XBL anonymous content.  CSS styles (e.g., bindings) should not be able to affect the results of CSS selector matching.

I don't have strong opinions about what happens to content that's in the binding... at least not yet.
Comment on attachment 360016 [details] [diff] [review]
make adjacent selector matching code XBL-aware

What comment 13 said.
Attachment #360016 - Flags: superreview?(bzbarsky)
Attachment #360016 - Flags: superreview-
Attachment #360016 - Flags: review?(bzbarsky)
Attachment #360016 - Flags: review-
Attached file testcase #2 (for comment 9) (obsolete) —
> Given <A/><anonymous node/><B/>, does this handle A + B correctly?

No, with the patch it doesn't match <B/>. I forgot about <children includes=.../>, so didn't think this situation was possible.

I didn't understand what the XBL2 spec was trying to say, either. I think it tries to say
1) that the selectors must continue to work for non-anonymous content, as if no anonymous content existed (i.e. <B/> should be matched in comment 9's example)
2) that the "+" selector must match an anonymous node if and only if the part before "+" matches the anonymous node right before the context node in the binding. This means that if the final flattened tree looks like this:
  <bound-element>
    <anonymous node 1/>
    <A/> (from <xbl:children/>)
    <anonymous node 2/>
    <anonymous node 3/>
  </bound-element>
.. you can't match any anonymous nodes 1&2 using the '+' selector, but you can match anonymous node #3.
Attachment #324694 - Attachment is obsolete: true
Attachment #324776 - Attachment is obsolete: true
Attachment #360004 - Attachment is obsolete: true
Comment 15 sounds correct to me in terms of understanding how this should work.

Note that I believe that doesn't address the use case the bug was filed on, right?
I don't understand for example why this selector actually works: .tabs-newtab-button + stack , which I use to skin the all-tabs stack on FF 3.2
The testcase from comment 15 seems wrong to me. For "Anonymous child #2 (should be red)" to be red, the selector would have to skip the included button that's between the anonymous nodes. That's neither a match in terms of "altered content" nor "original content".
> I don't understand for example why this selector actually works:

Because the <xul:toolbarbutton> with class="tabs-newtab-button" is a previous sibling of the <xul:stack> involved?  As in, they have the same parent.  This parent happens to be an anonymous node in an XBL binding, but they are just plain old DOM kids of their parent, so matching along the DOM tree matches as you see.
> Note that I believe that doesn't address the use case the bug was filed
> on, right?
Oops, yeah. Sorry about hijacking the bug. I'll have to think about the original use-case more.

> The testcase from comment 15 seems wrong to me.
Right.
Attachment #360004 - Attachment is obsolete: false
Attachment #360017 - Attachment is obsolete: true
Attached patch patch, try 2Splinter Review
Attachment #360016 - Attachment is obsolete: true
Attachment #361099 - Flags: superreview?(bzbarsky)
Attachment #361099 - Flags: review?(bzbarsky)
v2 implements what I said in comment 2.

I tried to describe how the matching should work with anonymous nodes in source code comments (basically the same as comment 15, plus one deviation from XBL2). The patch also contains the fixed testcase.

I don't see what can be done to address the original use case, so I'm going to clone this bug so that the original issue continues to be open.
> in comment 2.
"15". Sigh...
Blocks: 477423
That patch looks like it will assert on native anonymous content matching the rhs of the '+'.

It also looks like it'll do weird things with nested insertion points, since the only check is IsInAnonymousSubtree, without attempts to see whether the two nodes are in the _same_ anonymous subtree.

It also doesn't make :nth-* selectors work, of course.  Or :first-* or :last-*. Not sure whether we care, but it seems odd to fix specifically the combinators but not the selectors that do similar things...

(I haven't checked yet whether GetXBLChildNodesFor hands back what we want here, by the way; these are comments off the top of my head.)

It really might be better worth it to wait until sicking cleans up the xbl code and then implement this on top of said cleaner (and more xbl2-like) code.
Comment on attachment 361099 [details] [diff] [review]
patch, try 2

Per comment 24.
Attachment #361099 - Flags: superreview?(bzbarsky)
Attachment #361099 - Flags: superreview-
Attachment #361099 - Flags: review?(bzbarsky)
Attachment #361099 - Flags: review-
Assignee: asqueella → nobody
Status: ASSIGNED → NEW
Depends on: XBL2
Keywords: testcase
Whiteboard: testcase
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: