Last Comment Bug 875287 - Whitespace tokens in font-family parsing
: Whitespace tokens in font-family parsing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla24
Assigned To: Simon Sapin (:SimonSapin)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-23 06:16 PDT by Simon Sapin (:SimonSapin)
Modified: 2013-05-30 09:02 PDT (History)
1 user (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix, with a reftest (4.05 KB, patch)
2013-05-23 22:06 PDT, Simon Sapin (:SimonSapin)
dbaron: review+
Details | Diff | Splinter Review
Same patch, with filenames changed per dbaron’s comment (4.27 KB, patch)
2013-05-24 05:58 PDT, Simon Sapin (:SimonSapin)
no flags Details | Diff | Splinter Review
Same patch with a better commit message, including r=dbaron (4.51 KB, patch)
2013-05-28 23:17 PDT, Simon Sapin (:SimonSapin)
no flags Details | Diff | Splinter Review

Description Simon Sapin (:SimonSapin) 2013-05-23 06:16:25 PDT
From CSS 2.1, http://www.w3.org/TR/CSS21/fonts.html#font-family-prop

# Font family names must either be given quoted as strings,
# or unquoted as a sequence of one or more identifiers. […]
#
# If a sequence of identifiers is given as a font family name,
# the computed value is the name converted to a string by joining
# all the identifiers in the sequence by single spaces.

Identifiers tokens can be separated by comments, whitout whitespace.
For example, 'font-family: Fontin/**/Sans' should parse the same as 'font-family: Fontin Sans',
but doesn’t with a recent mozilla-central build.

I have a patch to fix this, but I’m adding some tests before submitting.
The patch only affects ParseOneFamily in nsCSSParser.cpp 
and should apply independently of bug 280443.
Comment 1 Simon Sapin (:SimonSapin) 2013-05-23 22:06:47 PDT
Created attachment 753618 [details] [diff] [review]
Proposed fix, with a reftest
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2013-05-24 03:02:04 PDT
Comment on attachment 753618 [details] [diff] [review]
Proposed fix, with a reftest

Seems like a good test to contribute to the CSS 2.1 or css3-fonts test suite.

Also, perhaps rename the test to font-family-whitespace-1 or something like that?  (I dislike bug numbers in test filenames.)  That's easy to fix by search-replacing the patch.

r=dbaron
Comment 3 Simon Sapin (:SimonSapin) 2013-05-24 05:58:13 PDT
Created attachment 753751 [details] [diff] [review]
Same patch, with filenames changed per dbaron’s comment
Comment 4 Simon Sapin (:SimonSapin) 2013-05-28 23:17:36 PDT
Created attachment 755206 [details] [diff] [review]
Same patch with a better commit message, including r=dbaron
Comment 5 Ryan VanderMeulen [:RyanVM] 2013-05-30 05:13:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2488e956bc6
Comment 6 Ryan VanderMeulen [:RyanVM] 2013-05-30 09:02:28 PDT
https://hg.mozilla.org/mozilla-central/rev/f2488e956bc6

Note You need to log in before you can comment on or make changes to this bug.