background shorthand doesn't parse anything after "position / size"

[upsuper]

The issue is that, the following code parses:

background: left top / auto;

but the following doesn't:

background: left top / auto none;

As far as I can see, anything in the same background item after the "position / size" syntax is rejected.

I tried to look into the code, but I failed to find anything wrong there.

[atheed]

I might be wrong, but I think this is because left top / auto none is syntactically malformed. The problem is the none: it is a keyword that cannot be applied to background-size, nor to any of the CSS rules that can follow background-size in the background shorthand (that is, background-repeat, background-attachment, background-origin, background-clip, and background-color).
I think the other major browsers also don't parse background: left top / auto none; for this reason.

[upsuper]

none is a valid value for background-image which is part of background shorthand, and all major browsers certainly parse left top / auto none. It is not the problem of none, but it seems to me nothing would get parsed after left top / auto including no-repeat and others.

(I think that was what I saw when I last checked. I don't have a working servo build at the moment...)

[atheed]

I think the background-image rule must strictly precede background-position and background-size when using the background shorthand, no? So a none applied to background-image must come before the "position / size"?

I did a quick check, and it appears Servo successfully parses something like this <div style="background: url(\'http://servo/test.png\') left top / auto auto repeat-x fixed padding-box content-box red;"> (a slightly-manipulated version of https://github.com/servo/servo/blob/master/tests/unit/style/parsing/background.rs#L19-L20) but not <div style="background: url(\'http://servo/test.png\') left top / auto none repeat-x fixed padding-box content-box red;"> (with none being the only change).

[upsuper]

Because in your last case, background-image has already been occupied by the url value, the additional none is surely invalid.

[atheed]

Oh, I see, I wasn't aware of that; apologies for the misinformation. In that case, I'm not entirely sure - I too had a look through some of the background shorthand parsing code too, and wasn't able to pinpoint anything there immediately.

[upsuper]

I think I know why. It is because the parser of background-size tries to parse two values, and it returns error when it doesn't accept the second one. However it is perfectly valid to have only one value in the background shorthand for background-size.

So something like left top / auto auto none can be parsed, but left top / auto none cannot, despite that the latter is also valid.

[atheed]

Hmm, I think you're right. Running a quick check, left top / auto auto none parses correctly, while left top / auto none does not (while both parse correctly on the major browsers; I must've made a typo the last time I tested the latter).
Looking at

servo/components/style/properties/longhand/background.mako.rs

Lines 397 to 407 in 5a624ff

	let height;
	if let Ok(value) = input.try(|input| {
	match input.next() {
	Err(_) => Ok(specified::LengthOrPercentageOrAuto::Auto),
	Ok(_) => Err(()),
	}
	}) {
	height = value
	} else {
	height = try!(specified::LengthOrPercentageOrAuto::parse_non_negative(input));
	}

suggests that it is requiring two values to be parsed, though one should be enough/valid too.

[upsuper]

It doesn't require two... but it requires two if there is anything after the first accepted value, which is wrong.

[atheed]

Yup, exactly.

[upsuper]

The same issue also affects mask shorthand (which currently only available with stylo, though).

[jdm]

@tmccrmck Does this look interesting? It requires creating a unit test in tests/unit/style/parsing/background.rs verifying that parsing the values from the original description works correctly. It then requires modifying the parsing code from this comment to make the test pass. The tests can be run with ./mach test-unit -p style.

[tmccrmck]

@jdm Yep! I'll reach out on IRC if I have any questions.

[jdm]

@tmccrmck Did you make any progress?

[tmccrmck]

@jdm Ah sorry, no. I haven't been able to compile Servo with my machine. The issue here. You should go ahead and unassign me.

[KiChjang]

I don't ever recall Servo builds requiring bindgen...?