75872 - [CSS]bad matching in [attr |= value] selectors if value contains a dash

Status: RESOLVED FIXED

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

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attribute dash-separated value selector has incorrect answer if the tested value
contains a dash. Test case attached.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case showing bug

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch for 75872

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Proposing patch for this bug. Test case attached to this bug becomes green.
Ian, can you please torture it a little bit ? I think it is ok because I have
other css3 selectors tests that go green but...

Pierre : since vietnamese motorbikes don't have internet access, can you
         r= before your sabbatical ?-) [just kidding, lucky man]
Marc : sr= ?

Thanks

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

bug ==> [e-mail]

Comment 5

[Marc Attinasi]

I approve! sr=attinasi
- Thanks Daniel

Comment 6

[Pierre Saslawsky]

Space characters are not handled with homogeneity:
   <p lang = "en-us">         matches        *[lang |= "en-us"]
   <p lang = " en-us">        matches        *[lang |= "en-us"]
   <p lang = "en-us ">        doesn't match  *[lang |= "en-us"]

Also, with:
    *[lang |= "en-us"]        matching works as described above
    *[lang |= " en-us"]       nothing matches
    *[lang |= "en-us "]       nothing matches

I don't know what the spec says about leading and trailing spaces.  I think both 
should be ignored.

Otherwise, r=pierre.

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Good catch, thanks Pierre. Investigating. If I find no answer, I will push this
issue to the CSS WG.
Btw, setting milestone for 0.9.1.

Comment 8

[Hixie (not reading bugmail)]

# User agents MAY ignore leading and trailing white space in CDATA attribute 
# values (e.g., "   myval   " MAY be interpreted as "myval"). Authors SHOULD 
# NOT declare attribute values with leading or trailing white space.
 -- http://www.w3.org/TR/html4/types.html#h-6.2

We SHOULD NOT ignore trailing spaces in the CSS attributes.

In other words... what Pierre describes is, technically, correct, although I 
would recommend trim()ing the value of the attribute before doing the comparison
(not in the DOM, of course).

Comment 9

[Hixie (not reading bugmail)]

DOH! I mean "CSS selectors" not "CSS attributes".

As in, [attr|="x     "] should match <el attr="x     -"/>.

You might want to do the test twice, once without the trim, and if it doesn't
match, then trim it, and try again (if there was anything to trim -- does trim()
tell you if it trimmed anything?).

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case ; all paragraph must be green with a correct implementation

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Ian: I think I have found a difficult case...
Given your comments above, the selector

  [attr|=" e"]

should match

  attr="      e "
             ^^
             you find here the selected value

right ?...

I think that the only way to solve this problem is Pierre's proposal : trim
the attribute's value *and* the selected value in selector.

Anyway, this bug needs more discussion between us, and perhaps an escalation to
the CSS WG, and we are friday afternoon, just a few hours before the freeze.
==> marking 0.9.2.

Comment 12

[Hixie (not reading bugmail)]

No, in my opinion

 [attr|=" e"]

should NOT match

  attr="      e "

...because either you completely trim the value, or you don't, you don't do 
bits. I see absolutely no advantage to matching in the above case.

(BTW, the freeze was for major landings, not any bug fix.)

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Ian, I think that this won't be clear from a CSS author's perspective and I
recommend bringing this issue to the CSS WG.

Marc, David, what do you think ?

Comment 14

[Hixie (not reading bugmail)]

Daniel: Why isn't it clear? Authors will very quickly learn to not include 
spaces in their dash match attribute selectors... I look forward to replying to
your post in w3c-css-wg. :-)

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Decision from CSS Working Group : you never trim white space on the CSS side.
White space in attribute values is trimmed during parsing of the XML.
The match is an exact match.

Back to implementation.

Comment 16

[Hixie (not reading bugmail)]

...which is basically what I said... :-)

Comment 17

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

This should be fixed with my checkin for bug 83616.

Comment 18

[rubydoo123]

Ian, can you test if this is indeed fixed with David's checkin for bug 83616?

Comment 19

[basic]

attachment 30736 [details] works for me in build 2001070108 win32
attachment 34063 [details] is invalid
marking resolve fixed, Hixie: can you confirm?

Comment 20

[Madhur Bhatia]

looking at testcase attachment 34063 [details], I still see the bug 

following is the style provided
----------------------------------------
<style type="text/css">
  p { color : red }
  p[lang|="a     "] { color : green }
  p[lang|="b"] { color : green }
  p[lang|="c"] { color : green }
  p[lang|="  d   "] { color : green }
  p[lang|="  en"] { color : green }
  p[lang|="fr"] { color : green }

  p.false { color : green }
  p[lang|="f"] { color : red }

  p.nl {color : green }
  p[lang|=" nl"] { color : red }

  p.se {color : green }
  p[lang|="se "] { color : red }
  </style>
----------------------------------------

the following praragraph text is displayed in 'red'
=========================================
<p lang=" b">This paragraph should be green.</p>
<p lang="c ">This paragraph should be green.</p>
<p lang="  fr-ca  ">Ce paragraphe doit être vert.
     (<span lang="en" style="font-style:italic">This paragraph should be
green</span>)</p>
=======================================

re-opening  bug 

Comment 21

[Boris Zbarsky [:bzbarsky]]

Madhur, see comment 19.

Resolving fixed again per comment 17.