Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: double quotes in table style attributes #269

Merged
merged 2 commits into from
Aug 5, 2017

Conversation

codeclown
Copy link
Contributor

@codeclown codeclown commented Jul 10, 2017

Example:

<style>
  td { background-image: url("test.png"); }
</style>
<td>Foobar</td>

Without this fix results in broken HTML:

<style>
  td { background-image: url("test.png"); }
</style>
<td style="background-image: url('test.png');" background="url(" test.png")"="">Foobar</td>

After fix:

<style>
  td { background-image: url("test.png"); }
</style>
<td style="background-image: url('test.png');" background="url('test.png')">Foobar</td>

@codeclown
Copy link
Contributor Author

codeclown commented Jul 10, 2017

I have no idea why the Travis-test is failing in one environment.

  1)  juice-content/relative-url:
     Uncaught TypeError: Unknown encoding: 
      at stringSlice (buffer.js:557:9)
      at Buffer.toString (buffer.js:593:10)
      at Request.<anonymous> (node_modules/request/request.js:1145:39)
      at Gunzip.<anonymous> (node_modules/request/request.js:1091:12)
      at endReadableNT (_stream_readable.js:1047:12)
      at _combinedTickCallback (internal/process/next_tick.js:102:11)
      at process._tickCallback (internal/process/next_tick.js:161:9)
  2)  remote_url:
     Uncaught TypeError: Unknown encoding: 
      at stringSlice (buffer.js:557:9)
      at Buffer.toString (buffer.js:593:10)
      at Request.<anonymous> (node_modules/request/request.js:1145:39)
      at Gunzip.<anonymous> (node_modules/request/request.js:1091:12)
      at endReadableNT (_stream_readable.js:1047:12)
      at _combinedTickCallback (internal/process/next_tick.js:102:11)
      at process._tickCallback (internal/process/next_tick.js:161:9)

I am happy to help merge this PR if I get a hint from the maintainers as to what is the cause of error here.

@jrit
Copy link
Collaborator

jrit commented Jul 11, 2017

The failure isn't related to your change, so that is fine. The change is clearly an improvement as it improves the syntax, though its a little weird that the background value is not what would be wanted for the background property on td, which should be a plain url, like background="test.png". Would you be interested in updating this to take the path out of the enclosing url('...') when the property is background? That would make this more valid.

@codeclown
Copy link
Contributor Author

Yes, I can update this as you reflected (in a day or two). I wonder, though, if there are other cases like this where the attribute value should be different from the CSS property.

@codeclown
Copy link
Contributor Author

@jrit Please see updated code.

@jrit jrit merged commit 1847d88 into Automattic:master Aug 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants