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

VERIFIED FIXED in Firefox 50

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: RyanVM, Assigned: bzbarsky, NeedInfo)

Tracking

(Blocks 1 bug, {regression, site-compat})

50 Branch
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 unaffected, firefox50- verified, firefox51- verified, firefox52- verified)

Details

()

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
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.
Flags: needinfo?(bzbarsky)
Reporter

Comment 1

3 years ago
Posted image expected rendering
Reporter

Comment 2

3 years ago
Posted image actual rendering
Reporter

Updated

3 years ago
Has Regression Range: --- → yes
Has STR: --- → yes
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.
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

3 years ago
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.)
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.  :(
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?
Flags: needinfo?(ttromey)
Flags: needinfo?(dbaron)
Ah, dbaron agreed with backing this out in email.  Still somewhat interested in his thoughts on the long-term fix.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Reporter

Comment 10

3 years ago
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 hidden (offtopic)
Attachment #8807810 - Attachment description: Patch for 50 and 51 → Patch for 50
Attachment #8807810 - Flags: review?(cam)
Posted patch Patch for 51 and 52 (obsolete) — Splinter Review
Attachment #8807813 - Attachment is obsolete: true
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.
Attachment #8807814 - Flags: review?(cam)
> Should we add the reduced testcase from this bug as a new reftest

That's probably not a bad idea.
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.
Flags: needinfo?(bzbarsky)
Attachment #8807810 - Flags: approval-mozilla-release?
Attachment #8807810 - Flags: approval-mozilla-beta?
Attachment #8807818 - Flags: review?(cam)
Attachment #8807814 - Attachment is obsolete: true
Attachment #8807814 - Flags: review?(cam)
Attachment #8807810 - Flags: review?(cam) → review+
Attachment #8807818 - Flags: approval-mozilla-aurora?
Attachment #8807818 - Flags: review?(cam) → review?(dbaron)
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.
Attachment #8807810 - Flags: approval-mozilla-release?
Attachment #8807810 - Flags: approval-mozilla-release+
Attachment #8807810 - Flags: approval-mozilla-beta?
Attachment #8807810 - Flags: approval-mozilla-beta+
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
Attachment #8807818 - Flags: review?(dbaron) → review+

Comment 19

3 years ago
Pushed by bzbarsky@mozilla.com:
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 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+
Attachment #8807818 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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

3 years ago
backoutbugherder
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.
Flags: in-testsuite+
This is fixed.  I'll reopen bug 790997.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Track 50-/51-/52- as this was fixed.
> 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.
Flags: needinfo?(ttromey)
(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.
> 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.  ;)
Reporter

Comment 28

3 years ago
Verified fixed across all channels.
Reporter

Updated

3 years ago
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.