Closed
Bug 1394311
Opened 7 years ago
Closed 7 years ago
stylo: Inconsistent application of non-lowercase lang tags
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: kevin.hsieh, Assigned: emilio)
References
Details
(Keywords: regression)
Attachments
(2 files)
1.02 KB,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
manishearth
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•7 years ago
|
||
Ugh, we have absolutely no test coverage for this then, really :( Reverting that bit sounds ok to me.
Assignee | ||
Updated•7 years ago
|
Component: DOM: Core & HTML → CSS Parsing and Computation
Assignee | ||
Comment 2•7 years ago
|
||
(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...)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 6•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
Flags: needinfo?(kevin.hsieh)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(kevin.hsieh)
Updated•7 years ago
|
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: Inconsistent application of non-lowercase lang tags → stylo: Inconsistent application of non-lowercase lang tags
Assignee | ||
Comment 7•7 years ago
|
||
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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8895f095ee2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Reporter | ||
Comment 10•7 years ago
|
||
Confirmed that this affects 56.0b6, even with stylo disabled. Will nominate for beta uplift once we have a testcase in Bug 1394762.
Reporter | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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+
Comment 13•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/63632d8680a0
Flags: in-testsuite+
Comment 15•7 years ago
|
||
(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.
Description
•