Closed Bug 1415946 Opened 7 years ago Closed 7 years ago

stylo: font-variant-alternates doesn't check for historical-forms in a case-insensitive way.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1415946: Test that font-variant-alternates: historical-forms is parsed case-insensitively.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Beta patch
1.80 KB, patch
jfkthame
: review+
Sylvestre
: approval-mozilla-release+
Details | Diff | Splinter Review
The following test-case: <!doctype html> <div style="font-variant-alternates: Historical-Forms"></div> <script> alert(getComputedStyle(document.querySelector('div')).fontVariantAlternates)); </script> Should alert "historical-forms", but instead alerts "normal" on stylo. I discovered this while reviewing https://github.com/servo/servo/pull/19167, and will land a test-case when that lands.
This is an uncommon property, but this is is definitely a regression, and is about to ship with 57. I'm not familiar with this property, but the fix is a one-liner. Jonathan, do you think it's worth uplifting this? (Also, ni? me to not forget about the test-case).
Flags: needinfo?(jfkthame)
Flags: needinfo?(emilio)
Keywords: regression
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1) > This is an uncommon property, but this is is definitely a regression, and is > about to ship with 57. > > I'm not familiar with this property, but the fix is a one-liner. Jonathan, > do you think it's worth uplifting this? Assuming the fix truly is a simple, safe one-liner, I'd be in favor of uplifting if at all possible, to avoid shipping the regression. Just changing a case-sensitive to insensitive comparison for this value doesn't sound like it would carry any wider risk. OTOH, I don't think this is important enough to risk the release schedule, if we've already triggered "final" builds or anything like that. If it doesn't make the 57.0 release, I hope we'd consider taking it as a ride-along fix in any dot-release that may happen. (Again, I wouldn't suggest that this issue itself justifies a dot-release; but if something comes up that causes us to do one, this fix should be safe enough to ride with it.)
Flags: needinfo?(jfkthame)
ritu, sylvestre fyi. We can track this for a dot release ride-along, but we're days away from release and have built and tested the second release candidate, so I don't think this can make 57.0.
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox57: ? → +
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Agreed, this is too late. Should we add that in the release notes for known issues ? (or at least on mdn?)
Flags: needinfo?(sledru)
Assignee: nobody → emilio
Priority: -- → P2
I've added this one to the 57.x ride-along candidate list.
Flags: needinfo?(rkothari)
Attached patch Beta patch — — Splinter Review
I know you don't do much rust Jonathan, but I guess you can review this :). The allow(missing_imports) is because rust-nightly has moved that method to str itself, which means that we wouldn't be using that trait. See https://github.com/servo/servo/pull/19162. Approval Request Comment [Feature/Bug causing the regression]: stylo [User impact if declined]: well, potentially missing styling [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not risky [Why is the change risky/not risky?]: Just changes how we compare a string to be ascii-case-insensitive. [String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8927521 - Flags: review?(jfkthame)
Attachment #8927521 - Flags: approval-mozilla-beta?
Comment on attachment 8927520 [details] Bug 1415946: Test that font-variant-alternates: historical-forms is parsed case-insensitively. https://reviewboard.mozilla.org/r/198832/#review203908 ::: testing/web-platform/tests/css/css-fonts-3/font-variant-alternates-parsing.html:5 (Diff revision 1) > +<script src=/resources/testharness.js></script> > +<script src=/resources/testharnessreport.js></script> Please add quotes to the paths here, for consistency with the overwhelming majority of wpt-css tests, and just because I think it's better style. :) Bonus points for doing the charset name as well, though that one bothers me less for some reason.
Attachment #8927520 - Flags: review?(jfkthame) → review+
Comment on attachment 8927521 [details] [diff] [review] Beta patch Review of attachment 8927521 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!
Attachment #8927521 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #8) > Please add quotes to the paths here, for consistency with the overwhelming > majority of wpt-css tests, and just because I think it's better style. :) > > Bonus points for doing the charset name as well, though that one bothers me > less for some reason. FWIW I also like this style better, the no-quotes were just the default template when you do |./mach wpt-create|, and I didn't bother changing them. (Maybe the default template should be updated?)
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7d77ff1bbdc4 Test that font-variant-alternates: historical-forms is parsed case-insensitively. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/7d77ff1bbdc4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8927521 [details] [diff] [review] Beta patch 57 is on release now.
Attachment #8927521 - Flags: approval-mozilla-beta? → approval-mozilla-release?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: no > [Needs manual test from QE? If yes, steps to reproduce]: no Does not need manual testing and has automated coverage, per Emilio.
Flags: qe-verify-
Comment on attachment 8927521 [details] [diff] [review] Beta patch We are probably going to do a dot release for 57. Taking it as ride along (recent regression, didn't cause any in nightly and beta 58)
Attachment #8927521 - Flags: approval-mozilla-release? → approval-mozilla-release+
And the actual fix since only the test changes got uplifted (should make those permafailing tests a lot happier too)... https://hg.mozilla.org/releases/mozilla-release/rev/6eb5c8f570e9222947577319f810a85d074890af
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: