Jonathan Kew [:jfkthame] Assignee
	Description • 10 years ago

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

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 3 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6487b399f9b2

	Jonathan Kew [:jfkthame] Assignee
	Comment 5 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e842c0559ff5

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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

	Phil Ringnalda (:philor)
	Comment 9 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/ce588ba0323d
https://hg.mozilla.org/mozilla-central/rev/6227331e17ed

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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

	Jonathan Kew [:jfkthame] 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.

	Jonathan Kew [:jfkthame] 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

	Jonathan Kew [:jfkthame] 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

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/464952b30298

	Jonathan Kew [:jfkthame] 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

	Liz Henry (:lizzard) (relman/hg->git project)
	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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 19 • 9 years ago

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7921dfde86c7

	Mike Taylor [:miketaylr]
	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).

	Jonathan Kew [: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.

	Mike Taylor [:miketaylr]
	Comment 22 • 8 years ago

OK, thanks. I'll try to keep an eye on blink-dev.

	Koji Ishii
	Comment 23 • 8 years ago

We're close to land the fix at https://codereview.chromium.org/1964773002
Still behind the runtime flag.