221981 - implement content pseudo-class :contains()

Status: NEW

(Core :: CSS Parsing and Computation, enhancement)

Description

[Anne (:annevk)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031011
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031011

Implement the CSS3 selector pseudo-class :contains() . I will add a simple test
case.

(should there be filled more bugs for other CSS3 selectors?)

Reproducible: Always

Steps to Reproduce:

Comment 1

[Anne (:annevk)]

Attachment: test case

Simple test case based on the specification:
http://www.w3.org/TR/css3-selectors/#content-selectors

Comment 2

[Hixie (not reading bugmail)]

> (should there be filled more bugs for other CSS3 selectors?)

No, please don't file bugs for missing features, just errors in already-
implemented features.

Bugs for missing features should only be filed if someone is actually intending 
to implement it right away, so that there is somewhere to discuss the code.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So this would be "easy" to implement by just looking at the textContent of the
node during style resolution and searching for the arg to contains() in that
string.  That's not likely to be anything like fast, though.  Then again, I see
no real way this selector could ever be fast.

Comment 4

[Sander]

Attachment: first stab at a patch

This works (tested with the :contains() tests (84, 84b, 85 and 163) from the
CSS3 test-suite, as well as with numerous contraptions of my own), but I have
copy/pasted a few bits and pieces of this code from other places and am not yet
certain that some of that code is actually necessary (specifically I don't know
exactly why NS_RELEASE is used, nor if the t2b/t1b stuff is necessary here),
let alone that it's the most efficient way to do things; unless someone who
*knows* can tell me this patch is ready for review, I'm going to take a wee bit
more time for reading documentation and crawling through the code (and probably
bothering people in #mozilla) before trying to get it accepted. I think I
should at least be reasonably confident that I'm doing the right thing before
bothering reviewers with it. :)
(But ye gods, :contains() is useful! head:contains("title of annoying page") +
body { rules to hide annoyances; } in a user stylesheet is almost as good as a
per-url user stylesheet.)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

+  PRInt32 searchStringLength = nsDependentString(aSearchString).Length();

nsCRT::strlen avoid constructing this temporary...

Comment 6

[Boris Zbarsky [:bzbarsky]]

> specifically I don't know exactly why NS_RELEASE is used

QueryInterface() addrefs its out param.  you may just want to use an nsCOMPtr.

> nor if the t2b/t1b stuff is necessary here

Probably not.  It's just an optimization, really... (to only store one-byte
chars when all the data is ascii).  If you're willing to make a copy of the
data, you could just get the UTF-16 from the textnode and be done with it.

Comment 7

[Sander]

Attachment: updated patch

Okay, so I figure it's still quite possible I'm doing stupid or inefficient
things, but I now have at least a reasonable idea of what I'm doing and believe
this is what the patch should look like.
I realize this is another CSS3 Selector while the dynamic changes bug is still
present (reading up now to figure out what the actual problem is, yet figuring
this will be affected by it as well), but this is what I set out at first to
create, with bug 237568 being only a waystation while trying to get a grip on
the code. I promise not to write any additional patches for the other selectors
until the bugginess is gone. :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

I won't be able to review this for at least a week and a half... possibly longer.

Comment 9

[fantasai]

The WG has been seriously thinking of dropping :contains from CSS3 Selectors. I've
emailed them about this bug, though -- about how :contains(foo) should be handled
and whether, if Mozilla chooses to implement it and it gets dropped, it should be
done with a vendor extension.

Comment 10

[Hixie (not reading bugmail)]

It should get a vendor prefix anyway if we check it in as is since we already
know it has a major bug (it's not dynamic).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145532 [details] [diff] [review]
updated patch

r- per our discussion on irc.

Comment 12

[Jesse Ruderman]

This feature could reintroduce a security hole in mail if remote images are
enabled and :contains can be used to make a background image load.  (Combining
ideas from bug 84545 and bug 147777.)

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This appears to have been dropped from the working draft?

http://www.w3.org/TR/css3-selectors/#content-selectors

Comment 14

[AndrewM]

Yes, this has been dropped from the latest working draft (15 December 2005). Notice that section 6.6.6 is now intentionally left blank. So this bug should be closed presumably?

Comment 15

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

Not necessarily.  It's still in call-for-implementations from the previous CR; it's just been dropped due to lack of implementations so that the spec can advance past that stage.

Comment 16

[Michael Ventnor]

Attachment: Patch

This is actually surprisingly easy to implement now. roc, you can review layout, right? I'm not sure if bz is available.

Comment 17

[fantasai]

This isn't layout, it's style system. You probably want dbaron to review.

Comment 18

[Michael Ventnor]

The style system is part of layout though, isn't it?

dbaron's going away for just over a week and I would really love to avoid unnecessary waiting.

Comment 19

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

This needs review from me or Boris, sorry.

Comment 20

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

And you definitely need to address comment 12.

And I think it would be nice if this were implemented without having to do a copy of the contents of every text node inside the element, although that is a bit harder.

But given how slow this can be, I'm really not sure if we want to implement this at all.  (In other words, I'm not sure whether the bug should be WONTFIX.)

Comment 21

[Michael Ventnor]

I don't understand how an exploit like that could work.

Comment 22

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

Comment on attachment 325864 [details] [diff] [review]
Patch

This also doesn't handle dynamic changes at all, which means we'll have incorrect results as pages are loading if the text you're looking for comes in in a different packet.

Until you have a plan for efficiently handling dynamic changes, I think we definitely don't want to do this.  And I'm not sure if we'd want to even if dynamic changes were free, just given the up-front cost.

Comment 23

[Sebastian Zartner [:sebo]]

> But given how slow this can be, I'm really not sure if we want to implement this 
> at all.  (In other words, I'm not sure whether the bug should be WONTFIX.)
I'd say performance optimization mainly lies at the web developer in this case. What can be done is to give hints how to write performant :contains() selectors as it's done for other selectors:

https://developer.mozilla.org/en/Writing_Efficient_CSS

So the only other concern seems to be the security issue mentioned in comment 12.
Seems I am not creative enough to come up with an exploit example, but I guess it would help the progress of this feature to create a test case for that.

Sebastian

Comment 24

[Sebastian Zartner [:sebo]]

> The WG has been seriously thinking of dropping :contains from CSS3 Selectors.

> it's just been dropped due to lack of implementations so that the spec can 
> advance past that stage.
Is there a public discussion, which resulted in this decision?

Sebastian

Comment 25

[Boris Zbarsky [:bzbarsky]]

> as it's done for other selectors:

Web developers routinely ignore the advice you link to.

Web developer _tools_ (LESS and SASS) routinely ignore the advice you link to.

History shows that if you give web developers a way to slow down their web pages, they use it indiscriminately and blame the browsers for being slow.

Comment 26

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Boris Zbarsky (:bz) from comment #25)
> History shows that if you give web developers a way to slow down their web
> pages, they use it indiscriminately and blame the browsers for being slow.

This sentence, when rephrased as "We don't want Web Developers to use these as that makes the whole Web slower." makes perfect sense to me, as a user. (The way you laid out this sentence seems to suggest that you care much about market share, which is quite irrelevant in this case, I believe.) 
   
Can I ask for examples? (It's not that I have doubts about this statement, but I want more examples to convince other people.)

Comment 27

[Sebastian Zartner [:sebo]]

In regard of comment 25:
As a Firebug team member I know very well that users very often shout before reading.
By pointing them to the docs or other existing information they become silent very fast.

JS Frameworks like jQuery already implement this and they even have JavaScript parsing as overhead. So performance can't be worse than that.

And I am pretty sure it will be less people complaining about :contains()'s performance than about :contains() not existing.

As David mentioned in comment 15 this has just been dropped from the spec due to the lack of implementations. So one browser has to do the first step. Then the others will surely follow soon.

Sebastian

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Can I ask for examples?

Sure.  Overuse of descendant combinators is a good example.  Overuse of shadows (which are very slow to render).  Pages moving news tickers on a 1ms timeout instead of a 16ms one.  

And I was in fact talking about market share.  Asking a browser to implement a feature which will slow down web pages in that browser and put it at a competitive disadvantage with regard to other browsers has an obvious problem, from my point of view.  It's happened when the arguments for the feature are strong enough, but I don't see such arguments in this bug.

> JS Frameworks like jQuery already implement this 

No, they do not.  In particular, they don't implement the hard and slow part, which is the proper handling of dynamic updates, last I checked.

> And I am pretty sure it will be less people complaining about :contains()'s performance

Sure.  They'll just bitch about performance, not realizing that their use of :contains() is to blame.

> this has just been dropped from the spec due to the lack of implementations

Yes, because no one knows how to implement this without either doing it wrong (a la WEbKit's handling of '+' and '~' combinators) or having it slow things down too much.

Comment 29

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Boris Zbarsky (:bz) from comment #28)
> > Can I ask for examples?
> 
> Sure.  Overuse of descendant combinators is a good example.  Overuse of
> shadows (which are very slow to render).  Pages moving news tickers on a 1ms
> timeout instead of a 16ms one.  

Thanks for the examples! (Descendant combinators are great examples to explain this, but I'll have to think a bit more about shadows.)
 
> And I was in fact talking about market share.  Asking a browser to implement
> a feature which will slow down web pages in that browser and put it at a
> competitive disadvantage with regard to other browsers has an obvious
> problem, from my point of view.  It's happened when the arguments for the
> feature are strong enough, but I don't see such arguments in this bug.

Provided that "a feature" is replaced with "a change of existing behavior", I understand the market share argument, but I still don't get how this argument works *in this case*. In general, there seems to be three reasons why the browsers would get slower when implementing a new feature like this:

A. The code size, hash tabble, etc. get bigger.
B. That feature, when used on a page, would be less performant than a JS workaround. 
C. The visual improvement that can be made with this feature is too little as compared to slowdown of the overall site. (Or in other word, making it easy to use is harming the users.)

B and C. are probably reasons with which CSSWG can reject this feature, but I doubt they affect market share, since a new selector, unlike a new property, can't be easily used before all other major browsers support and unprefixed this selector. I am happy to be proved wrong with examples of -moz-any() and -webkit-any() used on the Web now though.

Or are you mainly concerned about the effect of A. in this case?
 

(In reply to Sebastian Zartner from comment #27)
> And I am pretty sure it will be less people complaining about :contains()'s
> performance than about :contains() not existing.

Users complain about browser performance everyday. Even if they don't, we should think for them (and for us because we are users too). As a user, I am extremely unhappy if the whole purpose of this feature is the save time for Web Developers without any visible gain from the point of a user, so that Web Developers can spend more time cracking the popup blocker, for example.

Having said that, collecting use cases (or examples created by JS workaround on the Web, etc.) is certainly not a bad idea. Just for example, if the only only use case is what's mentioned in Comment 4 then we can limit this selector on user style sheets, or if the main use case is for document.querySelector like jQuery, we can limit it on document.querySelector. (Just examples)
 
> As David mentioned in comment 15 this has just been dropped from the spec
> due to the lack of implementations. So one browser has to do the first step.
> Then the others will surely follow soon.

For your information, you are free to send a mail to www-style[1] to ask this to be put back in Selectors 4. I would suggest you send along some example use cases too, as "it was in Selectors 3" is not a convincing argument why we need this at all. (There is as far as can tell no information about the use cases of this selector on www-style yet.)

[1] http://lists.w3.org/Archives/Public/www-style/

Comment 30

[Sebastian Zartner [:sebo]]

> For your information, you are free to send a mail to www-style[1] to ask this 
> to be put back in Selectors 4.
I subscribed to the mailing list on weekend for bug 740910. Though it seems the mail I sent to it did never arrive. Any help on this is appreciated.
Also there's still a lack of implementations, so I don't believe they will put it back in.
And if they decided to put this back into the spec, I assume Boris won't change his mind on this just because of that.

> Having said that, collecting use cases (or examples created by JS workaround on 
> the Web, etc.) is certainly not a bad idea.
Actually I just picked up a request of my workmate, which is working on websites for accounting. The reason why he was asking for that is to mark negative numbers in red.

> Overuse of descendant combinators is a good example.  Overuse of shadows (which 
> are very slow to render).  Pages moving news tickers on a 1ms timeout instead of 
> a 16ms one.

> I am happy to be proved wrong with examples of -moz-any() and -webkit-any() used 
> on the Web now though.
Why were shadows, -moz-any(), 3D features etc. implemented when they slow down the website performance?

Sebastian

Comment 31

[Boris Zbarsky [:bzbarsky]]

> since a new selector, unlike a new property, can't be easily used 

Unless you only use it for "progressive enhancement", which is the most likely use case here.

> The reason why he was asking for that is to mark negative numbers in red.

If this is static content, then it might make more sense to semantically mark up the numbers or to use a JS approach...  Yes, I realize these are workarounds.

> Why were shadows, -moz-any(), 3D features etc. implemented when they slow down

-moz-any, when used, speeds things up.

Shadows were implemented for two reasons: 1) People didn't anticipate how much they would be abused and 2) There was no viable alternative for pages.  #2 is somewhat not true for :contains, especially for static-content cases.

Comment 32

[Sebastian Zartner [:sebo]]

> If this is static content, then it might make more sense to semantically mark 
> up the numbers or to use a JS approach...
Of course he already does. Though as you say this is just a workaround. Styling is simply something that should better be done in CSS.
Btw. his idea went even beyond this by allowing :contains() to hold a regular expression. Though of course this could indeed kill page speed and raise further security issues.

>> Why were shadows, -moz-any(), 3D features etc. implemented when they slow down
> Shadows were implemented for two reasons: 1) People didn't anticipate how much 
> they would be abused and 2) There was no viable alternative for pages.
When they are causing that much problems, why don't you just remove them again?

The intention behind mentioning all this was to show, that speed decrease on abuse isn't an argument against a feature.
Otherwise you could say, let's just throw away CSS and JavaScript because they slow down page speed.

Sebastian

Comment 33

[Boris Zbarsky [:bzbarsky]]

> why don't you just remove them again?

See item (2) in my answer.  The point being that speed decrease on _use_ (not abuse!) is an argument against a feature.  There may be other, stronger, arguments _for_ the feature.  Or there might not.  Life's full of tradeoffs.  But nice try at reductio ad absurdum!

Comment 34

[Sebastian Zartner [:sebo]]

> The point being that speed decrease on _use_ (not abuse!) is an argument against 
> a feature.
Yes, but every such feature decreases speed - of course some faster than others. And the "use" is depending on the case. The example I named would be much faster than styling thousands of divs containing a specific sentence.

> There may be other, stronger, arguments _for_ the feature.
The arguments for and against this feature are clear enough now I think.
The difference is just that you can't prove how much this feature really slows down the performance until you implemented it while the benefits for web developers are obvious.

For users it probably wouldn't make much difference, if formatting was achieved through CSS, JavaScript or a semantical mark up. Every approach would more or less slow down the page's speed.
In regard of comment 28:
The question is what is more of an competitive disadvantage. Having this feature and accepting it to be slower in some cases or not to have this feature and miss the visual benefit.

So you're worried about market share but you can't predict how users will react to this until you implemented it.
So how much would you lose if you implemented this with a -moz- prefix?

Sebastian

Comment 35

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to Sebastian Zartner from comment #30)
> > For your information, you are free to send a mail to www-style[1] to ask this 
> > to be put back in Selectors 4.
> I subscribed to the mailing list on weekend for bug 740910. Though it seems
> the mail I sent to it did never arrive. Any help on this is appreciated.

You mail did arrive. See [1][2]. There's no response yet, but no worry, your feature request is reasonable. (I'd like to say the editors would eventually get to you, but in this case, the spec is [css4-ui], and the editor of [css3-ui] isn't very active on the list, so ...*shrug*)

> Also there's still a lack of implementations, so I don't believe they will
> put it back in.

Selectors 4[3] already has a bunch of things that still lack implementations, but of course some of those might be removed at the later stage of the spec. I think that particular spec might take a year or so to reach that stage.

> And if they decided to put this back into the spec, I assume Boris won't
> change his mind on this just because of that.

Well, you need to convince the CSS Working Group anyway, or someone here might eventually say "it's not in the spec so we don't want to implement this". Most importantly, if your don't raise discussions there, the use cases of this selector ( 1) Comment 4 2) styling negative numbers) would be forever lost in this Bugzilla instance. It's also not a bad idea if people can brainstorm more use cases there.

By the way, Boris is a regular participant on that list too.

[1] http://lists.w3.org/Archives/Public/www-style/2012Apr/thread#msg337
[2] http://lists.w3.org/Archives/Public/www-style/2012Apr/thread#msg336
[3] http://dev.w3.org/csswg/selectors4/

Comment 36

[Sebastian Zartner [:sebo]]

> You mail did arrive.
Yes, I saw it. Just took some days until it was there.

> I'd like to say the editors would eventually get to you, but in this case, the 
> spec is [css4-ui], and the editor of [css3-ui] isn't very active on the list, so
Well, we'll see.

> Well, you need to convince the CSS Working Group anyway, or someone here might 
> eventually say "it's not in the spec so we don't want to implement this".
That would be a doom loop. The working group doesn't want to put it into the spec because no one implements it. And no one implements it because it's not in the spec. :-)

> Most importantly, if your don't raise discussions there, the use cases of this 
> selector ( 1) Comment 4 2) styling negative numbers) would be forever lost in 
> this Bugzilla instance. It's also not a bad idea if people can brainstorm more 
> use cases there.
Sure. It'd be good to hear some more opinions on this.

> By the way, Boris is a regular participant on that list too.
Well, I think we all know his and my opinion about this now. Nothing against you, Boris! :-)
I'll try to keep the mail to the list objective and name the pros and cons mentioned here.

Sebastian

Comment 37

[Sebastian Zartner [:sebo]]

Here it is:
http://lists.w3.org/Archives/Public/www-style/2012Apr/0377.html
(This time it appeared really fast.)

So feel free to comment on it.

Sebastian

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Every approach would more or less slow down the page's speed.

That's just not the case.  Semantic markup would be much faster than :contains() on any page that actually does dynamic mutations of any kind.  It might well be faster even on static pages, since semantic markup happens to be particularly easy to optimize for and is already highly optimized.

> So how much would you lose if you implemented this with a -moz- prefix?

This is actually the worst of all possible worlds: it would lead to pages that are _only_ slow in Gecko, even if other UAs implement the feature.

Comment 39

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

The reason we don't want to implement it, and the reason the WG doesn't want it in the spec, is that handling dynamic changes would be very slow.

Comment 40

[Sebastian Zartner [:sebo]]

> Semantic markup would be much faster than :contains() on any page that actually 
> does dynamic mutations of any kind.
Though it still slows down the page, hence "more or less".

> it would lead to pages that are _only_ slow in Gecko, even if other UAs implement 
> the feature.
Yes, if Gecko implemented it wrong in comparison to other UAs it would be just slow in there.

> The reason we don't want to implement it, and the reason the WG doesn't want it in 
> the spec, is that handling dynamic changes would be very slow.
A static approach wouldn't be the perfect solution but already a great advantage to what we have now.
This seems also to be what Anne's patch already covered and what Tab Atkins Jr. just wrote in http://lists.w3.org/Archives/Public/www-style/2012Apr/0380.html.
So maybe this could at least be implemented in querySelector() (in a separate issue) while this one covers the CSS approach?

Sebastian

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.