1099557 - spurious control characters within content should be rendered as hexboxes, not hidden (reverting the behavior change from bug 947588)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

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

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Make -moz-control-character-visibility default to 'visible' rather than 'hidden', so that spurious control characters are rendered as hexboxes

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.

Comment 2

[Jonathan Kew [:jfkthame]]

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 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Jonathan Kew [:jfkthame]]

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

Comment 5

[Jonathan Kew [:jfkthame]]

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

Comment 6

[Jonathan Kew [:jfkthame]]

(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.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Reftest to check that stray control characters are visible in HTML content

Let's also add a test to verify that stray control chars in regular HTML content are no longer invisible.

Comment 8

[Jonathan Kew [:jfkthame]]

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

[Phil Ringnalda (:philor)]

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

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: followup - Put the default setting for control-character visibility behind a runtime pref, and keep it off-by-default on release builds for now

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.

Comment 11

[Jonathan Kew [:jfkthame]]

Oh, if we do this we'll also need to set the pref appropriately on the reftest that was added above.

Comment 12

[Jonathan Kew [:jfkthame]]

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

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e23e76de2669b437c2f2576614c9936c713906f4
Backed out changeset b3937b455406 (bug 1099557) because an incomplete patch was pushed.

Comment 14

[Jonathan Kew [:jfkthame]]

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

Comment 15

[Jonathan Kew [:jfkthame]]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Jonathan Kew [:jfkthame]]

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

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 19

[Jonathan Kew [:jfkthame]]

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

Comment 20

[Mike Taylor [:miketaylr]]

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).

Comment 21

[Jonathan Kew [:jfkthame]]

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.

Comment 22

[Mike Taylor [:miketaylr]]

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

Comment 23

[Koji Ishii]

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