Fix for #2325 and #2364 (should also fix -more #1125) #2401

Merged
merged 2 commits into from Aug 9, 2012

Conversation

Projects
None yet
3 participants
Contributor

gonchuki commented Aug 9, 2012

remove Browser.ie check as Opera also returns non-numeric value when border.*Width is queried and that property is unset.

The fact that Opera returns "medium" when the border is unset must not be interpreted as 3px as suggested in #2364 since that string is actually a quirk from the browser (the border is actually 0px, Dragonfly properly shows this property as being unset in the layout pane).

@gonchuki gonchuki remove Browser.ie check as Opera also returns non-numeric value when …
…border.*Width is queried and that property is unset
69bd1a3
Owner

ibolmo commented Aug 9, 2012

Was there a failing spec? If not, we should have one that failed prior to the fix.

Contributor

gonchuki commented Aug 9, 2012

No failing spec but I can make one. Just not sure if this should go in 1.4client folder or create a 1.5 one.

Owner

arian commented Aug 9, 2012

1.4client would be fine. Now we have this fix, we can pull it in 1.4.6. (both #2325 and #2364 have the 1.4.6 milestone.. :))

Owner

arian commented Aug 9, 2012

Basically copy those two jsfiddles in the specs.. update it a bit.. check if it fails without the fix.. and verify it passes with the fix.

Owner

ibolmo commented Aug 9, 2012

Right, and just try to run the specs against most browsers. I'm sure the rest of the team can cover any missing browsers before release.

Thanks @gonchuki .

Contributor

gonchuki commented Aug 9, 2012

I ended up creating my own specs as I wanted to test for all three failing properties (border.*Width was the culprit but it bubbled up to width/height to produce the NaNpx error)

for reference, this is the result of the failing specs:

should inherit the height from the child
Expected 'NaNpx' to equal '100px'.

should get a pixel based width
Expected 'NaNpx' to match /\d+px/.

should have a 0px border left
Expected 'medium' to equal '0px'.

my IE is not working right with the test suite so I couldn't run it, but unless these specs uncover new bugs the changed code was already IE-only so there's nothing new there.

Owner

arian commented Aug 9, 2012

Looks awesome

...

merging...

arian merged commit d8a3967 into mootools:master Aug 9, 2012

Contributor

gonchuki commented Aug 10, 2012

If you don't mind to merge again/cherry-pick (or manually edit), I acidentally left a trailing comma on the spec
gonchuki/mootools-core@7f59171

Owner

arian commented Aug 10, 2012

pulled

On Fri, Aug 10, 2012 at 4:43 PM, gonchuki notifications@github.com wrote:

If you don't mind to merge again/cherry-pick (or manually edit), I
acidentally left a trailing comma on the spec

gonchuki/mootools-core@7f59171


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2401#issuecomment-7646494.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment