1384463 - non-breaking space C2 A0 (U+00A0) causing problems with CSS Rules Inspector (which shows bogus character  , or shows decl as accepted when it's really rejected)

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[lewiscowles]

Attachment: Screenshot from 2017-07-26 07-53-07.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

open https://codepen.io/joao-silva/pen/qjGYpo in browser - CSS backed up to https://pastebin.com/uWSWZitt/raw in case of change


Actual results:

vertical align for inline-block elements seemed strange. (I've tested this works for any css property with (hex) 20 C2 A0.) It's likely that C2 A0 is not being trimmed, or needs to be replaced with 20 (space). Apologies, I've looked at the FF source code and couldn't make heads or tails of it.


Expected results:

vertical-align property should have been read in as-is without C2 A0 character (which is not displayed in inspector so can be trimmed). I've also attached an image of chrome, which has the same bug, but because the inspector shows a space, I know it's a character that has not been trimmed. Firefox doesn't seem to show this (indicating it knows to trim for display in inspector, but either chose not to trim in CSS, or has two divergent implementations for white-space trimming)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (ignore this version)

Here's a testcase I made, from copying CSS from your pastebin into a HTML file's <style> block.

For me, this actually triggers a devtools bug, where we display the property name as " vertical-align" (note the bogus  character), in addition to rejecting the declaration in the CSS parser.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

(Sorry, I think that last one was the wrong version...)

Comment 3

[Daniel Holbert [:dholbert]]

Odd, the  thing only happens when I view a local copy of the file -- not when I view it hosted on Bugzilla.

Anyway, the bugzilla-hosted version is still busted, but in a different way: devtools "rules" view shows the style as being accepted for the .p-card element (it shows "vertical-align: top" just fine, not struck out) -- but if you inspect the computed style for that element, you'll see that it's still stuck at the default value ("baseline").  So devtools and our CSS parser are disagreeing.

Which one is correct is a different matter; but they should agree.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of "Â" in devtools, when I view testcase 1 locally

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: screencast showing bugzilla-hosted "testcase 1" triggering disagreement between "rules" view & computed-style

Comment 6

[Daniel Holbert [:dholbert]]

So there's definitely a devtools bug here, but regarding whether the space should be accepted as valid CSS -- the CSS spec's section on tokenization says:

  # Only the characters "space" (U+0020), "tab" (U+0009),
  # "line feed" (U+000A), "carriage return" (U+000D),
  # and "form feed" (U+000C) can occur in white space.
  # Other space-like characters [...] are never part of
  # white space. 
https://www.w3.org/TR/CSS2/syndata.html

It looks like this "C2 A0" character is also known as U+00A0 , per https://en.wikipedia.org/wiki/Non-breaking_space -- and that's not one of the listed U+ characters there.  So I think Firefox and Chrome are *correctly* (per spec) parsing it as part of the property-name, and it ends up producing an unrecognized property name (i.e. there is no such property with name "{U+00A0}vertical-align"), and that's why it's rejected and doesn't end up influencing computed style.

So the actual rendering behavior is correct. What remains here are two devtools bugs:
 (1) When the testcase is viewed as a local file, "Â" shows up in rules view (as shown in attachment 8914979 [details])
 (2) When the testcase is viewed hosted on Bugzilla, the "vertical-align" declaration is shown as if it's accepted (but really it's not, as can be seen in compute style, and it should be struck out)

Migrating this bug to devtools component to cover those areas.

Comment 7

[Tom Tromey :tromey]

(In reply to Daniel Holbert [:dholbert] from comment #6)

>  (1) When the testcase is viewed as a local file, "Â" shows up in rules view
> (as shown in attachment 8914979 [details])

In this case I see a warning in the console:

> The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol.

So while this still isn't great, it's also somewhat expected.

Comment 8

[Tom Tromey :tromey]

The bug is here:

https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/devtools/shared/css/parsing-utils.js#313

At this spot, the non-breaking space is stripped from the property name.

Comment 9

[Tom Tromey :tromey]

There are actually two such spots; and removing the ".trim()" does the right thing.

However I wonder if the non-breaking space here should somehow be shown more clearly.
It's not totally obvious why " vertical-align" is crossed out.

Comment 10

[Tom Tromey :tromey]

I think it's better to have it crossed out than to wait for some better UI.
So I'm going to fix this and perhaps file a follow-up to make the styling better.

Comment 11

[Tom Tromey :tromey]

Attachment: Bug 1384463 - only trim CSS-allowed whitespace in declaration parser;

Review commit: https://reviewboard.mozilla.org/r/191296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191296/

Comment 12

[lewiscowles]

Hello,

I raised this bug because someone learning to code for the web showed me something, and it wasn't apparently obvious what had gone wrong.

While I accept that it defies standards and the spec prevents the definition of the character bytes as whitespace, it is a weird bug (first time in over 15 years I've noticed it). 

While I may know enough to visit a hex-editor to describe, I'm guessing others may simply give up and be lost to the web, or unnecessarily struggle. 

Crossing out a property really should come with a warning that on hover says something like "invalid property name", or finds some alternative representation for displaying the character to signify to the user that this is not a usual space character (which it looks like in most text-editors).

I really appreciate everyone time to try to approach this.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8920271 [details]
Bug 1384463 - only trim CSS-allowed whitespace in declaration parser;

https://reviewboard.mozilla.org/r/191296/#review196494

::: devtools/shared/css/parsing-utils.js:253
(Diff revision 1)
>            offsets: [undefined, undefined],
>            colonOffsets: false};
>  }
>  
>  /**
> + * Like trim, but only trims CSS whitespace.

s/CSS/CSS-allowed

for better clarity

::: devtools/shared/css/parsing-utils.js:264
(Diff revision 1)
> +  }
> +  return str;
> +}
> +
> +/**
> + * Like trimRight, but only trims CSS whitespace.

Same as above.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8920271 [details]
Bug 1384463 - only trim CSS-allowed whitespace in declaration parser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191296/diff/1-2/

Comment 15

[Tom Tromey :tromey]

(In reply to lewiscowles from comment #12)

> Crossing out a property really should come with a warning that on hover says
> something like "invalid property name", or finds some alternative
> representation for displaying the character to signify to the user that this
> is not a usual space character (which it looks like in most text-editors).

I filed a follow-up bug to deal with the message and perhaps calling out
the non-breaking space.  It is bug 1410423.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96703e5d9e78
only trim CSS-allowed whitespace in declaration parser; r=gl

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/96703e5d9e78