The CSS WG has agreed that stray control characters should be rendered visibly (as per Unicode recommendations) rather than hidden. 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.  http://dev.w3.org/csswg/css-text-3/#white-space-processing
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: nobody → jfkthame
Status: NEW → ASSIGNED
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+
(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.
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)
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
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)
Oh, if we do this we'll also need to set the pref appropriately on the reftest that was added above.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/e23e76de2669b437c2f2576614c9936c713906f4 Backed out changeset b3937b455406 (bug 1099557) because an incomplete patch was pushed.
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
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). 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.  https://lists.w3.org/Archives/Public/www-style/2015Sep/0242.html
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 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+
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).
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.
OK, thanks. I'll try to keep an eye on blink-dev.
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.