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

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Depends on: 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 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
(Assignee)

Comment 1

3 years ago
Created 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

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

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 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 6

3 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

3 years ago
Created attachment 8659968 [details] [diff] [review]
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.
Attachment #8659968 - Flags: review?(dbaron)
(Assignee)

Comment 8

3 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
https://hg.mozilla.org/mozilla-central/rev/ce588ba0323d
https://hg.mozilla.org/mozilla-central/rev/6227331e17ed
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Updated

3 years ago
Blocks: 757521
(Assignee)

Comment 10

3 years ago
Created 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

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

3 years ago
Oh, if we do this we'll also need to set the pref appropriately on the reftest that was added above.
(Assignee)

Comment 12

3 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

3 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

3 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

3 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

3 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 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+
Depends on: 1220462
See Also: → bug 1282690
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)
Depends on: 1282690
Depends on: 1282815
(Assignee)

Comment 21

3 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)
OK, thanks. I'll try to keep an eye on blink-dev.

Comment 23

3 years ago
We're close to land the fix at https://codereview.chromium.org/1964773002
Still behind the runtime flag.
Depends on: 1299739
You need to log in before you can comment on or make changes to this bug.