302e028f broke specs in firefox. #2456

Closed
arian opened this Issue Dec 21, 2012 · 6 comments

Projects

None yet

4 participants

@arian
MooTools member

Commit 302e028

Error message "Expected '20px auto' to equal '20px'."

@kentaromiura kentaromiura was assigned Dec 21, 2012
@kentaromiura
MooTools member

http://www.css3.info/preview/background-size/
'20px auto' and '20px' is the same thing, since the latter implicit set the height to auto.
while this test can be fixed by using indexOf I guess it will be better to change
Element.Styles.backgroundSize='@px @px';
so we can set it like
foo.setStyle('background-size', [20,30])
and expect 20px 30px,
[20, 20] returns 20px in chrome while 20px 20px in ff.

@SergioCrisostomo
MooTools member

@kentaromiura @arian can this be added directly after this line

        if (property == 'backgroundSize' && result){
            if(result.split(' ').length == 1) return result + ' auto';
        }

That would make it consistent when the value returned is just one parameter, it would add auto as referred here

If only one value is given the second is assumed to be ‘auto’.

Here is a fiddle with my PR code for fixing background-position issue, plus this code suggestion I posted here for the background-size. (In this case the tests would need to be changed to expect "20px auto" in case either "20px auto" is given or just "20px")

Edit: Tested the fiddle on VM (IE6/7/8/), FF, Chrome ad Safari and it worked.

@kentaromiura
MooTools member

@SergioCrisostomo
I think as I suggested in my previous comment that
Element.Styles.backgroundSize='@px @px';
should fix that without changing anything else and it's more accurate spec-wise.
Not sure if it will break anything else though since I didn't test (well actually I might have done that when I commented but it was an year ago so I don't really remember)

@arian suggestions?

@SergioCrisostomo
MooTools member

@kentaromiura I'm curious about your suggestion, and to learn from it. How could changing to '@px @px' correct this? I don't see where getStyle checks if Element.Styles.backgroundSize has 2 parameters?

Here is a fiddle with the Element.Styles.backgroundSize='@px @px';, and no extra if() for background-size.
I get different results still in FF & Chrome.

@kentaromiura
MooTools member

Assuming the link I posted nearly a year ago has the correct specification for background-size:

background-size: <bg-size> [ , <bg-size> ]*
<bg-size> = [ <length> | <percentage> | auto ]{1,2} | cover | contain

that means that background-size can be either 1 or more values and value can be any length, percentage, auto or cover or contain.

the code you proposed:

if (property == 'backgroundSize' && result){
if(result.split(' ').length == 1) return result + ' auto';
}

will not be ok in case of cover and contain, in that case background-size shouldn't be amended.

The fact FF and chrome returns different results is a non influential one as both results have the same meaning,
another discussion should be if MooTools should have a consistent result between browser, iirc for oldIE this is and has never been true, but for now I believe that setting Element.Styles.backgroundSize='@px @px'; (right now it's '@px') and change the test so it'll load two different values and will check against them will be the best solution.
but that's just my opinion.

@ibolmo ibolmo modified the milestone: 1.5.1, 1.5 Mar 3, 2014
@ibolmo ibolmo added the bug label Mar 3, 2014
@SergioCrisostomo
MooTools member

@arian @kentaromiura Can we close this?

@ibolmo ibolmo closed this Jul 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment