stylo: Inconsistent application of non-lowercase lang tags

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kevin.hsieh, Assigned: emilio)

Tracking

({regression})

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

Attachments

(2 attachments)

Posted file testcase.htm
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Nightly version: 57.0a1 (2017-08-27) (64-bit)

Steps to reproduce:

Open the attachment in Nightly.

Actual results:

The font used depends on the capitalization of the lang tag.

lower-lower lang tags: always work
lower-UPPER lang tags: work for zh-CN, zh-Hans, ja-JP, ko-KR but not for zh-TW, zh-Hant
UPPER-UPPER lang tags: work for ZH-CN, JA-JP, but not for ZH-HANS, ZH-TW, ZH-HANT, KO-KR

Expected results:

lower-UPPER results should match lower-lower results, at least. More generally, lang tags should probably not be case sensitive.

Regressed by:

Bug 1370802 "There's actually no need for lang to be lowercased."
Confirmed that reversing this commit fixes the issue
Ugh, we have absolutely no test coverage for this then, really :(

Reverting that bit sounds ok to me.
Component: DOM: Core & HTML → CSS Parsing and Computation
(On another note, seems like the place to lowercase the string shouldn't be the DOM / CSS engine, but its consumers, which look like they're doing that inconsistently...)
Hi Kevin, could you verify the above patch fixes the problem? I think it's the right solution instead of lower-casing the lang string in the style system.

I can't repro the bug, presumably because I don't have the correct fonts installed, or the correct prefs. Also, if you could guide me to add an automatic test for this, or provide me with one, I'd appreciate it.
Flags: needinfo?(kevin.hsieh)
Comment on attachment 8901704 [details]
Bug 1394311: Use lowercase consistently for looking up the language group.

https://reviewboard.mozilla.org/r/173138/#review178450
Attachment #8901704 - Flags: review?(manishearth) → review+
Hi Emilio, I can confirm that the patch fixes the inconsistencies, but I'm not sure how to test without the proper fonts...will look into it.
Flags: needinfo?(kevin.hsieh)
Flags: needinfo?(kevin.hsieh)
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: Inconsistent application of non-lowercase lang tags → stylo: Inconsistent application of non-lowercase lang tags
I'll land this for now, so we don't forget about this. I'll file a bug about landing a test-case for this, and ni? you Kevin.

Thanks a lot for testing btw!
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c8895f095ee2
Use lowercase consistently for looking up the language group. r=manishearth
Blocks: 1394762
https://hg.mozilla.org/mozilla-central/rev/c8895f095ee2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Confirmed that this affects 56.0b6, even with stylo disabled. Will nominate for beta uplift once we have a testcase in Bug 1394762.
Comment on attachment 8901704 [details]
Bug 1394311: Use lowercase consistently for looking up the language group.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1370802 "There's actually no need for lang to be lowercased."
[User impact if declined]: Breakage and/or inconsistent rendering of pages with East Asian text
[Is this code covered by automated tests?]: Yes (see Bug 1394762)
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1394762 (automated test)
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a one-line change that fixes a regression.
[String changes made/needed]: None
Flags: needinfo?(kevin.hsieh)
Attachment #8901704 - Flags: approval-mozilla-beta?
Comment on attachment 8901704 [details]
Bug 1394311: Use lowercase consistently for looking up the language group.

Fix a crash. Beta56+.
Attachment #8901704 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1395232
(In reply to Kevin Hsieh from comment #11)
> [Is this code covered by automated tests?]: Yes (see Bug 1394762)
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Kevin's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.