Closed Bug 1415946 Opened 2 years ago Closed 2 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)

defect

Tracking

()

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

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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 patchSplinter 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: 2 years ago
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.