964255 - [rule view] Cannot add background images containing a colon in the CSS value. Extra space added in URLs.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

STR:

- Open any page
- Open the devtools' inspector
- In the css rule-view panel, enter a new property in any of rules
- Property name should be |background|
- Property value should be |url(http://test.com)|

==> Notice that after submitting the value, an extra space is added after the column, making the URL invalid and therefore, the image is not applied to the background.

Comment 1

[Patrick Brosset <:pbro>]

I could be wrong, but I think this might come from the recent changes made to the rule-view in bug 913630.
Indeed, there's now a parsing done by splitting on : and ; when a new value is added.

Brian: Can you check if this could indeed come from that bug?

Comment 2

[Brian Grinstead [:bgrins]]

Interestingly, if you paste the full rule it isn't a problem.  So when testing on this URL: http://jsfiddle.net/bgrins/VqCrR/, it works if I paste in:

background: url('https://mozorg.cdn.mozilla.net/media/img/home/firefox-sm-high-res.png') repeat scroll 0% 0% transparent;

But if I start by adding "background", then copy in just the value in the value editor, the space becomes an issue.

Comment 3

[Brian Grinstead [:bgrins]]

This appears to be an error with the _parseCSSText function, which is parsing the following string: url('https://mozorg.cdn.mozilla.net/media/img/home/firefox-sm-high-res.png')

As a property with the name of:
url('https: 
And with a value of:
//mozorg.cdn.mozilla.net/media/img/home/firefox-sm-high-res.png

Then it is hitting the case where you paste name: value into a value editor editor and printing it as such (just as if you've entered "width:10px" into a value editor.

This regex is not very reliable for this, I can think of all kinds of edge cases that would fail involving content, like "content: 'background:red;'", and I'm sure there are plenty of others..  Though the most common thing to hit this seems it would be referencing a URL.

This particular issue could be fixed by handling this edge case without a space, though that does seem a bit fragile to rely on that.

Patrick, didn't you have a little editor that did a pretty good job parsing arbitrary CSS text when we were looking at the authored styles?  I wonder if part of that could be used for this case.

Comment 4

[Patrick Brosset <:pbro>]

Funny I was thinking exactly about css parsing today (I added some of my thoughts in here https://etherpad.mozilla.org/devtools-content-2014).
I realized we've got a number of places where we do some css parsing, depending on our needs (could be for pasting in new properties, or parsing image urls, ...).
The problem is that these various places do not share the parser, and they often use string splitting and simple regexs.
I think we need to get started with a real css parser to avoid having more and more of these edge cases bugs.
And also to make sure we have only one parser that is used for all our style inspector and style editor needs.

Can I suggest that we simply "fix" this regexp for now. The scenario in this bug is pretty common and is annoying and, in parallel, start working on a more full-blown parser.

In fact, the recently added style editor code completion tool landed with a css tokenizer which could very well be what we need (see browser/devtools/sourceeditor/css-tokenizer.js, and doc here https://github.com/tabatkins/css-parser).
I'd like to start experiment with it and see how it could fit in.

As for the style editor demo you're referring to, I don't think it would be easy to integrate it here. But I'll take a look again.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Funny I was thinking exactly about css parsing today (I added some of my
> thoughts in here https://etherpad.mozilla.org/devtools-content-2014).
> I realized we've got a number of places where we do some css parsing,
> depending on our needs (could be for pasting in new properties, or parsing
> image urls, ...).
> The problem is that these various places do not share the parser, and they
> often use string splitting and simple regexs.
> I think we need to get started with a real css parser to avoid having more
> and more of these edge cases bugs.
> And also to make sure we have only one parser that is used for all our style
> inspector and style editor needs.
> 
> Can I suggest that we simply "fix" this regexp for now. The scenario in this
> bug is pretty common and is annoying and, in parallel, start working on a
> more full-blown parser.
> 
> In fact, the recently added style editor code completion tool landed with a
> css tokenizer which could very well be what we need (see
> browser/devtools/sourceeditor/css-tokenizer.js, and doc here
> https://github.com/tabatkins/css-parser).
> I'd like to start experiment with it and see how it could fit in.
> 
> As for the style editor demo you're referring to, I don't think it would be
> easy to integrate it here. But I'll take a look again.

I agree - I will work on fixing this issue as it is and we should work towards a better solution long term.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: ruleview-colon.patch

As discussed, this works around the common issue with http:// in a URL by concatenating the 'name: value' edge case entered into a value field.  There may be other ways we could work around this if you think we should not change this behavior.  This is an edge case though, like if you entered "width: height: 10px" the end result will now be |width|: |height:10px| instead of |width|: |height: 10px|

Ongoing try push: https://tbpl.mozilla.org/?tree=Try&rev=6ac21d8969f0

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url.patch

Brian, here's another take on this bug using the new css-tokenizer that has landed last week.

The changes are only in parseCSSText, and the function keeps the exact same signature.
It also returns the same data in the edge cases where you have trailing property name, etc...
All tests seem to be passing fine.

The function body is noticeably longer than in the previous version, but by using a tokenizer, we're sure to avoid other weird issues in the future.

I noticed with the first fix that indeed the http:// problem was solved, but other problems like ": " being transformed into ":" appeared.

I have a feeling that we could perhaps simplify a bit this version parseCSSText if we changed how multiple properties were handled, but I don't know enough about it to be sure. For instance, parseCSSText only returns a value without property if it's at the end of the csstext string. If you're passing only "margin" for instance, it doesn't return anything. I think we should make that function as simple as possible so it always parses in the same way, and let consumers deal with these cases.

Having said that, in any case, I'd like to get started into css parsing for supporting authored styles in the rule-view, so anyway, we'll need to rework that part.

So, it's up to you Brian, how we proceed for this bug.

Comment 8

[Patrick Brosset <:pbro>]

Oh, and I forgot to remove the first fix. See this comment:
> // Note that we remove the space between colon and value to workaround
> // Bug 964255, until parseCSSText returns better results with URLs.
This isn't needed anymore with my patch.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8370010 [details] [diff] [review]
bug964255-rule-view-breaks-image-url.patch

Review of attachment 8370010 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> Created attachment 8370010 [details] [diff] [review]
> bug964255-rule-view-breaks-image-url.patch
> 
> Brian, here's another take on this bug using the new css-tokenizer that has
> landed last week.
> 
> The changes are only in parseCSSText, and the function keeps the exact same
> signature.
> It also returns the same data in the edge cases where you have trailing
> property name, etc...
> All tests seem to be passing fine.
> 

Sounds great, especially since all the tests are still passing! I think this is a cleaner way to handle the input parsing.

> I have a feeling that we could perhaps simplify a bit this version
> parseCSSText if we changed how multiple properties were handled, but I don't
> know enough about it to be sure. For instance, parseCSSText only returns a
> value without property if it's at the end of the csstext string. If you're
> passing only "margin" for instance, it doesn't return anything. I think we
> should make that function as simple as possible so it always parses in the
> same way, and let consumers deal with these cases.

Yes, this makes sense.  There are a couple of cases in the ruleview where we check to see if the parsed CSS length is 0 (like https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#2332), but we should be able to just as easily check length === 1 and properties[0].value == null or whatever.

> Having said that, in any case, I'd like to get started into css parsing for
> supporting authored styles in the rule-view, so anyway, we'll need to rework
> that part.
> 
> So, it's up to you Brian, how we proceed for this bug.

Let's go with what you have here.  The other fix was more temporary, and this will help with other use cases in the future.  Since we may be using this functionality for other parts of the style inspector, we may want to add a test that covers just this function (not the UI) and all the edge cases we can experience with it.

::: browser/devtools/styleinspector/rule-view.js
@@ +2457,3 @@
>        firstValue = enteredValueFirst ?
>          firstProp.value + "!" + firstProp.priority :
> +        firstProp.name + ":" + firstProp.value + "!" + firstProp.priority;

As you say, this workaround can be removed

::: browser/devtools/styleinspector/test/browser_ruleview_multiple_properties.js
@@ +243,5 @@
>      EventUtils.synthesizeKey("VK_ESCAPE", {}, ruleWindow);
>      is(elementRuleEditor.propertyList.children.length, 2, "Should have removed the value editor.");
>  
>      is(elementRuleEditor.rule.textProps[0].name, "width", "Should have correct property name");
> +    is(elementRuleEditor.rule.textProps[0].value, "height:10px", "Should have correct property value");

Don't need this change anymore either

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> > I have a feeling that we could perhaps simplify a bit this version
> > parseCSSText if we changed how multiple properties were handled, but I don't
> > know enough about it to be sure. For instance, parseCSSText only returns a
> > value without property if it's at the end of the csstext string. If you're
> > passing only "margin" for instance, it doesn't return anything. I think we
> > should make that function as simple as possible so it always parses in the
> > same way, and let consumers deal with these cases.
> 
> Yes, this makes sense.  There are a couple of cases in the ruleview where we
> check to see if the parsed CSS length is 0 (like
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> styleinspector/rule-view.js#2332), but we should be able to just as easily
> check length === 1 and properties[0].value == null or whatever.
Ok, I'll take a look at that.
What I'm doing now is creating a new xpcshell unit test for parseCSSText (I've extracted it to a separate module) to test all sorts of edge cases, just to make sure I've got them all in mind. Once done, I'll make its output more consistent and then adapt the rule-view.

Comment 11

[Patrick Brosset <:pbro>]

While working on the test, I realized that the other parsing utility function we have: parseValue, is prone to failure as well. It's used to parse a value and !important parts of a string in an object, but it doesn't even check if the word important follows the ! characters, nor does it checks if those are within q quoted string.
So I'm gonna make this function just use the parseCSSText and only keep its signature. I'm adding test for it too.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url v2.patch

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=917f85e45c28

In this patch:
- split the parseCSStext and parseValue functions into their own module, so they can be imported in other modules and unit tested separately
- renamed both functions
- rewrote parseCSSText using a css tokenizer to correctly handle cases like url or content strings
- removed the edge cases from that function and instead changed rule-view.css to handle these
- rewrote parseValue to use parseCSSText since it was error-prone
- added 2 new xpcshell unit test for these functions

All style-inspector mochitest-browser tests pass locally.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8372237 [details] [diff] [review]
bug964255-rule-view-breaks-image-url v2.patch

Review of attachment 8372237 [details] [diff] [review]:
-----------------------------------------------------------------

I've listed a couple of issues I see with the parsing - overall it looks like this will be a great fix to drop in that will resolve a bunch of edge cases

::: browser/devtools/styleinspector/css-parsing-utils.js
@@ +35,5 @@
> +      if (!lastProp.name) {
> +        lastProp.name = current.trim();
> +        current = "";
> +        hasBang = false;
> +      } else {

Please add a comment for this case
// If there are multiple colons in a single value

@@ +39,5 @@
> +      } else {
> +        current += ":";
> +      }
> +    } else if (token.tokenType === ";") {
> +      lastProp.value = current.trim();

We should add tests for the cases: 

* background: red;;;;; - Should return a single [name: background, value: red].  This currently fails badly on the frontend (wiping out the rule view)
* background:; - Should return a single [name: background, value: '']
* ;;;;; - Should return an empty array
* :;:; - Should return an empty array

I believe all we may need to do is check `if (lastProp.name)` to prevent this adding a bunch of empty declarations.  And/or we could filter the entire list at the bottom of the function removing all empty items from declarations

@@ +84,5 @@
> +            current += token.value;
> +          }
> +          break;
> +        case "STRING":
> +          current += '"' + token.value + '"';

For both URL and string, do we need to check if the output contains a " and wrap in single quotes?  Something similar is being done with markup view attributes: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1807.  There is a section on this in the spec: http://www.w3.org/TR/CSS21/syndata.html#strings.  We probably want to add tests for the listed examples:


"this is a 'string'"
"this is a \"string\""
'this is a "string"'
'this is a \'string\''

and

"a not s\
o very long title"

::: browser/devtools/styleinspector/rule-view.js
@@ +2357,3 @@
>      let propertiesToAdd = [];
> +    let properties = parseDeclarations(aValue);
> +    // Check that the value could be parsed

(we are confusing terms here - it should be declarations, but we use properties).  Probably not worth trying to rename everything in this file though. Either way, this could read:

// Check to see if the input string can be parsed as multiple properties

@@ +2357,5 @@
>      let propertiesToAdd = [];
> +    let properties = parseDeclarations(aValue);
> +    // Check that the value could be parsed
> +    if (properties.length) {
> +      // If indeed the value was a vakue (potentially followed by other properties)

Could be "Get the first property value (if any), and any remaining properties (if any)".

@@ +2595,5 @@
>   */
>  function blurOnMultipleProperties(e) {
>    setTimeout(() => {
> +    let props = parseDeclarations(e.target.value);
> +    if (props.length && props.length > 1) {

Can just check props.length > 1

::: browser/devtools/styleinspector/test/browser_ruleview_editor_changedvalues.js
@@ +173,5 @@
> +
> +        waitForEditorBlur(aEditor, function() {
> +          promiseDone(expectRuleChange(idRuleEditor.rule).then(() => {
> +            let value = idRuleEditor.rule.domRule._rawStyle().getPropertyValue("background-image");
> +            is(value, TEST_URL, "border-color should have been set.");

"background-image should have been set"

@@ +174,5 @@
> +        waitForEditorBlur(aEditor, function() {
> +          promiseDone(expectRuleChange(idRuleEditor.rule).then(() => {
> +            let value = idRuleEditor.rule.domRule._rawStyle().getPropertyValue("background-image");
> +            is(value, TEST_URL, "border-color should have been set.");
> +            is(propEditor.isValid(), true, "red should be a valid entry");

"The test URL should be a valid entry"

::: browser/devtools/styleinspector/test/browser_ruleview_multiple_properties.js
@@ +28,5 @@
>    let newElement = doc.createElement("div");
>    newElement.textContent = "Test Element";
>    doc.body.appendChild(newElement);
> +
> +  inspector.selection.setNode(newElement, "test");

Is this change intentional?  I guess this is passing a reason for the selection

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url v3.patch

Thanks for the review Brian!

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8372237 [details] [diff] [review]
> bug964255-rule-view-breaks-image-url v2.patch
> 
> Review of attachment 8372237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've listed a couple of issues I see with the parsing - overall it looks
> like this will be a great fix to drop in that will resolve a bunch of edge
> cases
> 
> ::: browser/devtools/styleinspector/css-parsing-utils.js
> @@ +35,5 @@
> > +      if (!lastProp.name) {
> > +        lastProp.name = current.trim();
> > +        current = "";
> > +        hasBang = false;
> > +      } else {
> 
> Please add a comment for this case
> // If there are multiple colons in a single value
Done

> @@ +39,5 @@
> > +      } else {
> > +        current += ":";
> > +      }
> > +    } else if (token.tokenType === ";") {
> > +      lastProp.value = current.trim();
> 
> We should add tests for the cases: 
> 
> * background: red;;;;; - Should return a single [name: background, value:
> red].  This currently fails badly on the frontend (wiping out the rule view)
> * background:; - Should return a single [name: background, value: '']
> * ;;;;; - Should return an empty array
> * :;:; - Should return an empty array
> 
> I believe all we may need to do is check `if (lastProp.name)` to prevent
> this adding a bunch of empty declarations.  And/or we could filter the
> entire list at the bottom of the function removing all empty items from
> declarations
Good catch. I've changed the code to remove all empty items at the end and added several test cases.

> @@ +84,5 @@
> > +            current += token.value;
> > +          }
> > +          break;
> > +        case "STRING":
> > +          current += '"' + token.value + '"';
> 
> For both URL and string, do we need to check if the output contains a " and
> wrap in single quotes?  Something similar is being done with markup view
> attributes:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.js#1807.  There is a section on this in the spec:
> http://www.w3.org/TR/CSS21/syndata.html#strings.  We probably want to add
> tests for the listed examples:
> 
> 
> "this is a 'string'"
> "this is a \"string\""
> 'this is a "string"'
> 'this is a \'string\''
> 
> and
> 
> "a not s\
> o very long title"
Good catch too! I didn't think of these cases.
I've extracted the string quoting into a function and added several test cases for this.
The logic is that it will always enclose into double-quotes (and escape accordingly) except if the string contains only double-quotes in which case it will use single quotes.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +2357,3 @@
> >      let propertiesToAdd = [];
> > +    let properties = parseDeclarations(aValue);
> > +    // Check that the value could be parsed
> 
> (we are confusing terms here - it should be declarations, but we use
> properties).  Probably not worth trying to rename everything in this file
> though. Either way, this could read:
> 
> // Check to see if the input string can be parsed as multiple properties
I've changed the comment as advised. I agree changing all occurences of properties to declarations is not a good idea, going to make the patch bigger than necessary. I could rename the function parseProperties though, but I feel like parseDeclarations is more correct.

> @@ +2357,5 @@
> >      let propertiesToAdd = [];
> > +    let properties = parseDeclarations(aValue);
> > +    // Check that the value could be parsed
> > +    if (properties.length) {
> > +      // If indeed the value was a vakue (potentially followed by other properties)
> 
> Could be "Get the first property value (if any), and any remaining
> properties (if any)".
Done.

> @@ +2595,5 @@
> >   */
> >  function blurOnMultipleProperties(e) {
> >    setTimeout(() => {
> > +    let props = parseDeclarations(e.target.value);
> > +    if (props.length && props.length > 1) {
> 
> Can just check props.length > 1
Yes you're correct, parseDeclarations always return an array (unless it throws).

> :::
> browser/devtools/styleinspector/test/browser_ruleview_editor_changedvalues.js
> @@ +173,5 @@
> > +
> > +        waitForEditorBlur(aEditor, function() {
> > +          promiseDone(expectRuleChange(idRuleEditor.rule).then(() => {
> > +            let value = idRuleEditor.rule.domRule._rawStyle().getPropertyValue("background-image");
> > +            is(value, TEST_URL, "border-color should have been set.");
> 
> "background-image should have been set"
Done.

> @@ +174,5 @@
> > +        waitForEditorBlur(aEditor, function() {
> > +          promiseDone(expectRuleChange(idRuleEditor.rule).then(() => {
> > +            let value = idRuleEditor.rule.domRule._rawStyle().getPropertyValue("background-image");
> > +            is(value, TEST_URL, "border-color should have been set.");
> > +            is(propEditor.isValid(), true, "red should be a valid entry");
> 
> "The test URL should be a valid entry"
Done.

> :::
> browser/devtools/styleinspector/test/browser_ruleview_multiple_properties.js
> @@ +28,5 @@
> >    let newElement = doc.createElement("div");
> >    newElement.textContent = "Test Element";
> >    doc.body.appendChild(newElement);
> > +
> > +  inspector.selection.setNode(newElement, "test");
> 
> Is this change intentional?  I guess this is passing a reason for the
> selection
Yes it was an intentional change. I've recently introduced this new selection reason, specifically for tests to avoid calling brieflyHighlightElement which ends up sending an extra async request to the server which is oftentimes unwanted during tests (unless we're specifically testing the highlighter, which we're not here).

Comment 15

[Patrick Brosset <:pbro>]

New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=501b205a406e

Comment 16

[Patrick Brosset <:pbro>]

It's safe to say that the try build is green!

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8373247 [details] [diff] [review]
bug964255-rule-view-breaks-image-url v3.patch

Review of attachment 8373247 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/css-parsing-utils.js
@@ +129,5 @@
> +    }
> +  }
> +
> +  // Remove declarations that have neither a name nor a value
> +  declarations = declarations.filter(prop => prop.name || prop.value);

Two more edge cases I've found:

1) 0: 0; does not show up if pasted in.  I'm assuming it fails this filter, so we may want to check on prop.name.length and prop.value.length instead
2) More importantly, the string `: red;` fails and puts the rule view into a broken state until leaving the node and coming back.  My suggestion would be that instead of checking name || value here, we simply check for prop.name, since I'm not sure how much sense a declaration with an empty name makes.  Alternatively, we could handle this case more gracefully in the markup view, filtering out empty names before whatever happens that is causing this bad error case.  Either way, we should define the interface and test for it.

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> 1) 0: 0; does not show up if pasted in.  I'm assuming it fails this filter,
> so we may want to check on prop.name.length and prop.value.length instead
Right, I'll do that.

> 2) More importantly, the string `: red;` fails and puts the rule view into a
> broken state until leaving the node and coming back.  My suggestion would be
> that instead of checking name || value here, we simply check for prop.name,
> since I'm not sure how much sense a declaration with an empty name makes. 
> Alternatively, we could handle this case more gracefully in the markup view,
> filtering out empty names before whatever happens that is causing this bad
> error case.  Either way, we should define the interface and test for it.
I can't remove the name check part in the condition because the parseDeclarations parser allows empty names because it's also used when pasting text in the value editors. We even have a specific test case that checks that empty names do work.
So I'll take a look at the rule-view code instead and see where we can prevent this error.

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> (In reply to Brian Grinstead [:bgrins] from comment #17)
> > 1) 0: 0; does not show up if pasted in.  I'm assuming it fails this filter,
> > so we may want to check on prop.name.length and prop.value.length instead
> Right, I'll do that.
Well, in fact this is the current behavior in Aurora too. Try adding a new declaration with a number as its name, as soon as you type `:` the name disappears.
And so the parser actually works for `0:0;` because "0" is truthy.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url v4.patch

Brian, here's a quick change.

As explained above, I haven't made any change about the `0: 0` input since that's the current behavior even without my patch. If we decide to support this, we should file a new bug, but I'm not sure if we should do it.

As for the `:red;` input, I've made a change to the rule-view's _onNewProperty method so it now filters out name-less properties since those don't make sense when creating a new property name.

Comment 21

[Patrick Brosset <:pbro>]

New ongoing try build : https://tbpl.mozilla.org/?tree=Try&rev=10437a149dbb

Comment 22

[Patrick Brosset <:pbro>]

As discussed with Brian, the `0:` input only fails in the element {} rule if that rule already contains declarations.
It fails in a way that the whole rule-view becomes empty.
The JS error thrown in the console points to a bug in the output-parser which takes the declaration name and applies it to a dummy element via `element.style[name] = value`.
This throws when name isn't settable. Using `element.setProperty(name, value)` instead works better, in all cases.
Here's a new ongoing try build with this change: https://tbpl.mozilla.org/?tree=Try&rev=346af591a632
Uploading a patch right now.

Comment 23

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url v5.patch

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8374154 [details] [diff] [review]
bug964255-rule-view-breaks-image-url v5.patch

Review of attachment 8374154 [details] [diff] [review]:
-----------------------------------------------------------------

This try build seems to be failing b-c tests.  I'm fine with breaking this odd case of 0:0; out into another bug since it is an existing issue that appears to be unrelated to this.  The patch without this fix looks good to go

Comment 25

[Patrick Brosset <:pbro>]

Yeah I think you're right, the seemingly minor change is having major consequences on some of the tests. So I'll revert the last change made in the v5 patch and file a new bug.

Comment 26

[Patrick Brosset <:pbro>]

Attachment: bug964255-rule-view-breaks-image-url v6.patch

- This is a simple revert of what was introduced in the v5 patch
- Exact same patch as v4
- Try build was already run earlier: https://tbpl.mozilla.org/?tree=Try&rev=10437a149dbb
- Filed a separate bug for the `0: 0;` usecase: bug 971628

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8374690 [details] [diff] [review]
bug964255-rule-view-breaks-image-url v6.patch

Review of attachment 8374690 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 28

[Patrick Brosset <:pbro>]

Thanks Brian.
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/c0aac2afec10

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

[Andrei Vaida [:avaida]]

Reproduced the bug on 29.0.1 with the STR from Comment 0. This issue is verified fixed on Firefox 30 Beta 4 (Build ID: 20140512231802), using Windows 7 64-bit [1].
[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0