Closed Bug 1511570 Opened 6 years ago Closed 6 years ago

content-language processing doesn't invalidate style data

Categories

(Core :: CSS Parsing and Computation, defect)

60 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- verified

People

(Reporter: Fanolian+BMO, Assigned: emilio)

References

(Blocks 1 open bug, )

Details

Attachments

(6 files)

Attached video Screencast for Apple Daily font change (deleted) —
(Fonts mentioned below apply to Windows only) Prerequisite: To reproduce this bug, I think it needs a few CJK fonts installed in the system. In this case Microsoft JhengHei, Meiryo, and MingLiU_HKSCS (or SimSun if you test it on other zh-CN sites) are used. I have all Microsoft provided fonts installed. Steps to reproduce: (Please refer to the attached screencast.) 1. Visit https://hk.entertainment.appledaily.com/entertainment/daily/article/20181201/20558431. 2. Change the main article font by adding a font-family:serif entry in Inspector. ** DO NOT assign a specific fontname. USE ONLY serif (or sans-serif). ** 3. Observe the font change. Acutal result: Chinese characters change from JhengHei to Meiryo instead of MingLiU_HKSCS, the default font for zh-HK sites. It may not happen 100% of the time. If so, reload the page and retry a few times. "Workaround": The font will display/be chosen correctly after I go to Options > Fonts & Colors > Advanced, select another language, and press OK without making any changes. This workaround fails when the page is reloaded, even if I permanently modifty the CSS with extensions like Stylus (which is how I first notice the bug). Expected result: Either the page follows the value of http-equiv="content-language" (just like pre-bug1438911), or assign at least a serif font defined in whatever language Nightly defaults to. I understand that http-equiv="content-language" is not recommended. If the current display behavior is intended however, the "workaround" should not be working as it is confusing (though I assume no one will do this in daily browsing). Note: If I specify a font rather than serif or sans-serif, this bug is not exhibited. According to Mozregression, the first bad Nightly is 2018-02-20 and regressed by bug 1438911.
Attached file Reduced testcase (deleted) —
This is a reduced testcase but it exhibits a slightly different behavior. Upon opening the testcase the first time the font is wrong (Meiryo) most of the time. Reloading the page will have a chance to display correctly (MingLiU_HKSCS). The workaround in comment 0 still applies. Before bug 1438911 is fixed, reloading this testcase will not randomly display correctly but only after opening devtool, or applying the workaround.
Attached video Screencast of reduced testcase (deleted) —
Attached file Modified reduced testcase (deleted) —
As a reference, this is a modified reduced testcase which declares language properly. It always displays with preferred proportional font for zh-HK.
Blocks: 1438911
Keywords: regression
If bug 1438911 changed the behavior here that means that there's a pre-existing bug / race somewhere else that was being hidden by the fact we deferred the charset change. I'll try to take a look, I'll ask if I have trouble to repro.
Flags: needinfo?(emilio)
(In reply to Fanolian from comment #1) > Before bug 1438911 is fixed, reloading this testcase will not randomly > display correctly but only after opening devtool, or applying the workaround. Ah, so if I'm reading this correctly that bug _improved_ the behavior, right? That probably reinforces the hypothesis that there's something on our code that looks at the current charset but doesn't react to it changing properly. That bug makes the change happen sooner, which increases the chances of whatever the bogus code is of reading the right thing.
No longer blocks: 1438911
Depends on: 1438911
Keywords: regression
(In reply to Emilio Cobos Álvarez (:emilio) from comment #5) > (In reply to Fanolian from comment #1) > > Before bug 1438911 is fixed, reloading this testcase will not randomly > > display correctly but only after opening devtool, or applying the workaround. > > Ah, so if I'm reading this correctly that bug _improved_ the behavior, right? I would not say this is an improvement :). (consistently wrong -> randomly wrong) Perhaps it is a side effect, after bug 1438911 I cannot apply font-family:serif/sans-serif to this site anymore with Stylus. It would not apply the expected font, as defined in Options, for me.
(In reply to Fanolian from comment #6) > (In reply to Emilio Cobos Álvarez (:emilio) from comment #5) > > (In reply to Fanolian from comment #1) > > > Before bug 1438911 is fixed, reloading this testcase will not randomly > > > display correctly but only after opening devtool, or applying the workaround. > > > > Ah, so if I'm reading this correctly that bug _improved_ the behavior, right? > > I would not say this is an improvement :). (consistently wrong -> randomly > wrong) Well, it only is consistently wrong because there's no dynamic change to the page, so it's not really consistent either.
Attached file Test-case showing the underlying bug. (deleted) —
In my machine zooming into the page causes the second line to switch from the generic serif font on my system to noto serif. I know why, though I think ideally the two lines should render the same? We don't seem to be using content-language to determine the default font-family, is that expected Jonathan?
Flags: needinfo?(jfkthame)
I'll write a patch, this is pretty broken.
Assignee: nobody → emilio
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: CJK font change via modifying CSS may not apply correctly for sites using http-equiv="content-language" → content-language processing doesn't invalidate style data
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8) > Created attachment 9029177 [details] > Test-case showing the underlying bug. > > In my machine zooming into the page causes the second line to switch from > the generic serif font on my system to noto serif. > > I know why, though I think ideally the two lines should render the same? We > don't seem to be using content-language to determine the default > font-family, is that expected Jonathan? I can reproduce the issue since Firefox58(en-US) windows10 Home 64bit Japanese edition. Clear cache Load the test case(attachment 9029177 [details]) Times New Roman -> SimSun after zoom in/out -> Times New Roman after Reload(F5) Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5 Regressed by: 7b3215e18443 J. Ryan Stinnett — Bug 1330412 - Enable Stylo by default
This is needed because content-language can affect the default computed values for a given document.
Blocks: 1511675
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8) > Created attachment 9029177 [details] > Test-case showing the underlying bug. > > In my machine zooming into the page causes the second line to switch from > the generic serif font on my system to noto serif. > > I know why, though I think ideally the two lines should render the same? We > don't seem to be using content-language to determine the default > font-family, is that expected Jonathan? I'm ~sure now that this is not expected, I think... I took a quick look and filed bug 1511675. All this font stuff is a huge mess.
Flags: needinfo?(emilio)
Is bug 1349852 related to this bug?
(In reply to Fanolian from comment #13) > Is bug 1349852 related to this bug? Yeah, to some extent.
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ddd8bdd2f05e Make content-language invalidate style data. r=jfkthame
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify+

I was able to reproduce this bug on an affected Nightly build 65.0a1 (2019-01-21), and using the STR from comment 10.

The issue is no longer repro on 65.0 (20190121133710) running Windows 10 x64.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: