1315368 - Navigation bar at the top of WordPress blog shows up as bulleted list

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

I noticed today that the navigation bar across the site of my wife's website shows up as a bulleted list. I was able to bisect it with mozregression down to bug 790997. Displays correctly in Chrome.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: expected rendering

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: actual rendering

Comment 3

[Boris Zbarsky [:bzbarsky]]

The CSS involved is presumably this bit:

  url('<?php bloginfo('template_directory');?>/images/sticky.gif')

yes, with the php thing right in there.  Simple testcase:

  data:text/html,<style>body { color: red; background: url('('')'); color: green}</style>Text

This ends up red in Gecko, green in Blink.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so per spec we start at https://www.w3.org/TR/css-syntax-3/#consume-a-url-token0 and in step 4 see an apostrophe.  So we go to https://www.w3.org/TR/css-syntax-3/#consume-a-string-token0 and keep going until the next apostrophe.  We unwind back to https://www.w3.org/TR/css-syntax-3/#consume-a-url-token0 step 4.3 and then see that the next input cod point is not ')' so we should https://www.w3.org/TR/css-syntax-3/#consume-the-remnants-of-a-bad-url0.  This consumes everything up to and including the first ')' character and returns.

We now unwind from "consumer a url token", and start consuming the next token.  The next character we see is  U+0027 APOSTROPHE so we start consuming a string... and it never terminates.  You can see this if you do:

  data:text/html,<style>body { color: red; background: url('('')')'; color: green}</style>Text

which is green in Firefox, but now red in Chrome.

As far as I can tell Chrome just isn't following the spec here, but does that mean the spec needs to get changed?

Comment 5

[Tab Atkins Jr.]

Whoops, this is a change since the last CR publication. (I just put in a request to repub the spec, since the TR version is 2 1/2 years old.)  In particular, we resolved a while ago to allow the "string url" syntax form to just be an ordinary function, so we can add arguments to it (in particular, CORS and SRI stuff).

If you follow the ED, you see that we never enter "consume a url token" at all; this case is caught by "consume an ident-like token" and just returns a <function-token> immediately after consuming "url(". You then continue parsing as normal for functions, getting two <string-token>s followed by a <(-token> in your first example, and three <string-token>s (the third of which contains the rest of the stylesheet) in your second example.

So, Chrome is correct in both of these examples.

(I also finally put a calender reminder in for myself to repub all specs with changes every 6 months. We keep forgetting to do this.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Argh.  The really shitty part is that we're about to ship this change on the web, due to the lack of CSSWG tests that actually test the spec.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

So what I think we should do here is probably back out bug 790997 on all branches.  Then we can think about what we want to do here.

Implementing the ED behavior of turning url('string' whatever) into a function token followed by some other tokens requires some reworking of the parser.  I _could_ try just faking it in the scanner by doing the equivalent of SkipUntil after consuming the string, or just moving SkipUntil into the scanner, and continuing to return a URL token.  But doing that would mean that our lexer would be lossy; we'd need to update our lexer tests to deal, I guess.  Maybe that's an OK short-term solution....

In any case, for 50 I propose we just go back to the 49 behavior.  Any objections?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, dbaron agreed with backing this out in email.  Still somewhat interested in his thoughts on the long-term fix.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 50

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Should we add the reduced testcase from this bug as a new reftest while we're at it, or are our existing tests sufficient?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 51 and 52

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 51 and 52

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8807814 [details] [diff] [review]
Patch for 51 and 52

This is just like the patch for 50, but doesn't have the css-lexer.js change, since that file is gone.  Will do some try runs to make sure it still passes and whatnot, of course.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Should we add the reduced testcase from this bug as a new reftest

That's probably not a bad idea.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8807810 [details] [diff] [review]
Patch for 50

Approval Request Comment
[Feature/regressing bug #]: Bug 790997
[User impact if declined]: Some sites will be broken that are not broken in 49.
[Describe test coverage new/current, TreeHerder]: I'm changing the tests, so... ;)
[Risks and why]: Some sites will go back to being broken as they were before bug
   790997.  But that's just restoring the 49 status quo.
[String/UUID change made/needed]: None.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 51 and 52

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8807810 [details] [diff] [review]
Patch for 50

It will take us sometime to land a proper fix (to be spec compliant). In the meantime we could either stay consistent with what has been shipping thus far (49 and older releases) or ship something new/unknown in 50 which we has to be fixed eventually. BZ's suggestion of reverting to a last good known state (aka 49 behavior) is better than what is in 50 atm. Let's uplift and stabilize in RC2 build.

Comment 18

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

Comment on attachment 8807818 [details] [diff] [review]
Patch for 51 and 52

Oops, I was thinking this was a more substantive fix rather than just a slightly more merged backout.  r=dbaron on this too

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1af90b862e1
Back out changeset bf190e326bfd (bug 790997) because what it implements doesn't actually follow the CSS syntax editor's draft and breaks some sites in the process.  r=dbaron

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8807818 [details] [diff] [review]
Patch for 51 and 52

Let's backout from 51 too to ensure we still stay consistent with 49 behavior in case the proper fix is not ready by the time 51 goes live. Aurora+

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69e17427242dc8063a7ad7d739247d4c47402da5
https://hg.mozilla.org/releases/mozilla-release/rev/dc617d65c9f0cdbbe4351cc1e5c288b05f25f8f7

beta is closed because all the 50 action is on release now, so I didn't push there.

I included a reftest on inbound and aurora.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a1af90b862e1

Not resolving because it's not clear whether this is meant to also be the bug for the eventual fix or not.

Comment 23

[Boris Zbarsky [:bzbarsky]]

This is fixed.  I'll reopen bug 790997.

Comment 24

[Gerry Chang [:gchang]]

Track 50-/51-/52- as this was fixed.

Comment 25

[Tom Tromey :tromey]

> This is just like the patch for 50, but doesn't have the css-lexer.js change, since that file is gone.  Will do some try runs to make sure it still passes and whatnot, of course.

It's devtools/shared/css/lexer.js now.  AFAIK the tests should still verify that it works similarly to
the platform lexer.  At first I was worried that this was going to need deeper devtool changes, but
I think just changing the lexer back should be sufficient.

Comment 26

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #25)

> It's devtools/shared/css/lexer.js now.

... and I see that one of the patches handled this correctly.
So I think all's well from devtools.  Thank you.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> It's devtools/shared/css/lexer.js now

Yeah, I found it eventually; the try runs that didn't modify that file failed nicely.  Yay tests.  ;)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Verified fixed across all channels.