Closed Bug 1355352 Opened 3 years ago Closed 3 years ago

Parser doesn't completely reject unicode-range value when part of it is invalid

Categories

(Core :: CSS Parsing and Computation, enhancement)

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Whiteboard: [stylo])

Attachments

(1 file)

Example code:
<!DOCTYPE HTML>
<style>
  @font-face { unicode-range: U+100-17F,U+200-17F; }
</style>
<script>
  alert(document.styleSheets[0].cssRules[0].style.getPropertyValue('unicode-range'));
</script>

In this case, the unicode-range descriptor should be completely rejected, but we currently keeping the valid part (U+100-17F).

Blink and Stylo do the right thing that the whole value is rejected.

(NOTE: This is not a Stylo bug, but a Gecko bug which has test on this.)
(Probably better asking :jtd for review but I guess he's not active here nowadays...)

So the spec says the following [1]:
> Unicode codepoint values must be between 0 and 10FFFF inclusive.
> For interval ranges, ... the end codepoint must be greater than or equal to the start codepoint.
which indicates that those value are invalid. And per Simon Sapin, invalid value generally invalidates the whole declaration (descriptor here) in CSS, rather than being partially ignored.


[1] https://drafts.csswg.org/css-fonts-3/#unicode-range-desc
Assignee: nobody → xidorn+moz
The spec even says explicitly that "Ranges that do not fit one of the these forms are invalid and cause the declaration to be ignored." 

From skimming discussion back in bug 443976, e.g. comment 18 there, it looks like at one time the spec had text that matched our current behavior, but it has since been tightened up and we have not changed to match. But if Blink behaves this way already, there shouldn't be much compat risk in fixing our implementation to follow the current spec.
Comment on attachment 8867949 [details]
Bug 1355352 - Reject unicode-range descriptor when its value contains invalid part.

https://reviewboard.mozilla.org/r/139478/#review142960
Attachment #8867949 - Flags: review?(jfkthame) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e30cc47c3023 -d 9fa32a431bc8: rebasing 395967:e30cc47c3023 "Bug 1355352 - Reject unicode-range descriptor when its value contains invalid part. r=jfkthame" (tip)
merging layout/style/nsCSSParser.cpp
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/538468358017
Reject unicode-range descriptor when its value contains invalid part. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/538468358017
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1355356
You need to log in before you can comment on or make changes to this bug.