Closed
Bug 1099557
Opened 10 years ago
Closed 9 years ago
spurious control characters within content should be rendered as hexboxes, not hidden (reverting the behavior change from bug 947588)
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
References
Details
Attachments
(3 files)
1.29 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
dbaron
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The CSS WG has agreed that stray control characters should be rendered visibly (as per Unicode recommendations) rather than hidden.[1] We should therefore revert the change made in bug 947588. Initially, we can do this by simply changing the initial value of -moz-control-character-visibility from 'hidden' to 'visible'. Eventually, we may wish to remove the property altogether, unless we see an ongoing use-case for it. [1] http://dev.w3.org/csswg/css-text-3/#white-space-processing
Assignee | ||
Comment 1•9 years ago
|
||
I believe this is the minimal change needed here; we should land this at the appropriate time once the CSS WG has agreed on a schedule for implementing the change.
Attachment #8659473 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
We can also remove the instances of -moz-control-character-visibility:visible in our stylesheets (e.g. for View Source), but let's leave that until we're confident the change to the default will stick.
Comment on attachment 8659473 [details] [diff] [review] Make -moz-control-character-visibility default to 'visible' rather than 'hidden', so that spurious control characters are rendered as hexboxes You need to also change the aInitialValue parameter in: // -moz-text-discard: enum, inherit, initial SetDiscrete(*aRuleData->ValueForControlCharacterVisibility(), text->mControlCharacterVisibility, conditions, SETDSC_ENUMERATED | SETDSC_UNSET_INHERIT, parentText->mControlCharacterVisibility, NS_STYLE_CONTROL_CHARACTER_VISIBILITY_HIDDEN, 0, 0, 0, 0); in nsRuleNode::ComputeTextData. You should also fix the comment (text-discard -> control-character-visibility) while you're there. Please keep an eye on Chrome's progress shipping this change so that we also revert the change if they have problems shipping it. r=dbaron with that
Attachment #8659473 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6487b399f9b2
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e842c0559ff5
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to David Baron [:dbaron] βUTC-7 from comment #3) > Comment on attachment 8659473 [details] [diff] [review] > Make -moz-control-character-visibility default to 'visible' rather than > 'hidden', so that spurious control characters are rendered as hexboxes > > You need to also change the aInitialValue parameter in: ... Argh, how did I overlook that? Thanks. > Please keep an eye on Chrome's progress shipping this change so that we also > revert the change if they have problems shipping it. Currently, https://code.google.com/p/chromium/issues/detail?id=530342 says they're targeting Chrome 47, which ships at the beginning of December IIUC. So landing this for Firefox 43 (shipping Dec 15) should be the right timeframe. I'll watch the chromium issue in case of any change in plans there.
Assignee | ||
Comment 7•9 years ago
|
||
Let's also add a test to verify that stray control chars in regular HTML content are no longer invisible.
Attachment #8659968 -
Flags: review?(dbaron)
Attachment #8659968 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce588ba0323d873829bbf3e5505619eaeb3f7236 Bug 1099557 - Make -moz-control-character-visibility default to 'visible' rather than 'hidden', so that spurious control characters are rendered as hexboxes. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/6227331e17ed5320b7b997d72704f8939bced596 Bug 1099557 - Reftest to check that stray control characters are visible in HTML content. r=dbaron
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce588ba0323d https://hg.mozilla.org/mozilla-central/rev/6227331e17ed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 10•9 years ago
|
||
I'm a bit concerned by recent Microsoft comments (from Greg W) on www-style, which make it sound like the 'flag day' for turning this on may be many months away yet. If that's the case, we don't want to be shipping it by default yet in FF43, and should probably have it behind a runtime pref for a few releases. Still waiting to see what the Blink position is going to be, but we need to be prepared to go for a slower deployment here. So here's a patch to add a runtime pref, defaulting to off for release builds; I'd like to be ready to land this and potentially uplift it to Aurora 43 if we get confirmation of the longer timeline for shipping on-by-default.
Attachment #8663750 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•9 years ago
|
||
Oh, if we do this we'll also need to set the pref appropriately on the reftest that was added above.
Attachment #8663750 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3937b4554065fd4c93d88d66d079168c4e7e476 Bug 1099557 followup - Put the default setting for control-character visibility behind a runtime pref, and keep it off-by-default on release builds for now. r=dbaron
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e23e76de2669b437c2f2576614c9936c713906f4 Backed out changeset b3937b455406 (bug 1099557) because an incomplete patch was pushed.
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/464952b30298b4ba94ce53915ccfefcbabff07e1 Bug 1099557 followup - Put the default setting for control-character visibility behind a runtime pref, and keep it off-by-default on release builds for now. r=dbaron
Assignee | ||
Comment 15•9 years ago
|
||
Latest word from Blink is that they plan to ship this preffed OFF in 47 (early Dec), and then pref it ON in 48 (mid-Jan).[1] On that basis, I think we want to target mozilla-44 for shipping this behavior on release builds, which will mean uplifting the followup here (adding the about:config pref) to aurora-43, with the default remaining false in release builds; and then flip that default on trunk for the mozilla-44 train. Accordingly, I plan to request uplift for the pref-patch once it has merged to central. [1] https://lists.w3.org/Archives/Public/www-style/2015Sep/0242.html
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8663750 [details] [diff] [review] followup - Put the default setting for control-character visibility behind a runtime pref, and keep it off-by-default on release builds for now Approval Request Comment [Feature/regressing bug #]: Bug 1099557 (this bug!) changed our behavior here (making stray control characters in content display as hexboxes, instead of hiding them), based on the CSS WG decision that all browsers will be making such a change. [User impact if declined]: We landed the behavior change for mozilla-43, but the latest word from Blink (who we're aiming to coordinate with) is that they'll keep it preffed off for one more cycle that we originally thought. So as things stand (if we don't uplift this followup), we'll ship the change of behavior here to the release channel a month earlier than Blink, instead of in the closest release as intended. [Describe test coverage new/current, TreeHerder]: Covered by reftests. [Risks and why]: Minimal, the patch just puts the change to the initial value of the CSS property behind a pref. [String/UUID change made/needed]: n/a
Attachment #8663750 -
Flags: approval-mozilla-aurora?
Comment 18•9 years ago
|
||
Comment on attachment 8663750 [details] [diff] [review] followup - Put the default setting for control-character visibility behind a runtime pref, and keep it off-by-default on release builds for now Useful explanation, thanks! Approved for uplift to aurora.
Attachment #8663750 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 19•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/7921dfde86c7
Comment 20•8 years ago
|
||
Jonathan, do we have a target version that Blink intends to ship this behavior on? If we're going to contact sites about cleaning up these stray control chars, it's useful to let them know that Chrome is going to appear broken as well (and approx. what version).
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 21•8 years ago
|
||
I don't know anything beyond what we can glean from https://bugs.chromium.org/p/chromium/issues/detail?id=530342, which suggests that it was supposed to have been added to Canary (behind the --enable-experimental-web-platform-features flag) by now, but apparently didn't get done yet.
Flags: needinfo?(jfkthame)
Comment 23•8 years ago
|
||
We're close to land the fix at https://codereview.chromium.org/1964773002 Still behind the runtime flag.
You need to log in
before you can comment on or make changes to this bug.
Description
•