526402 - background: -moz-linear-gradient(0 0, yellow, blue); renders horizontal gradient instead of diagonal

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[aja+bugzilla]

background: -moz-linear-gradient(0 0, yellow, blue); renders horizontal gradient instead of diagonal

See 2nd column of "Example 2 - basic corner-to-corner diagonal gradient" at http://a-ja.net/newgrad.html

Tests at URL are derived from examples in spec proposal at http://www.xanthir.com/:4bhipd

FYI: 0px 0px or 0% 0% both result in diagonal, so perhaps it's unknown/undefined unit type case that's failing

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's just a parsing bug.

Comment 2

[aja+bugzilla]

(In reply to comment #1)
> I think it's just a parsing bug.

Perhaps so.   No message in error console for it, though, like the other couple of <bg-position> syntax "errors" in this test page.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: patch

Yeah, it's a parsing bug.  The first bare 0 was being interpreted as an angle, rather than as part of the box position.  Two-line patch attached.

aja, I'd like to make your example page into reftests.  Would that be okay?  I'm not gonna be able to do it until later today, though.

Comment 4

[aja+bugzilla]

(In reply to comment #3)
> aja, I'd like to make your example page into reftests.  Would that be okay? 

Sure, please do...make use of its content however you wish.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Hmmm.  But according to the spec linear-gradient(0 top left, yellow, blue) should also be valid, and this switches to rejecting that.

Perhaps we should be changing the spec here, although I'm not sure how.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Actually, the problem is that our code accepts '0' as an angle.  The spec says that angles always require a unit identifier.

We should fix that bug (and probably add a test to test_units_angle.html .)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

And the same for times and frequencies, actually.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 410267 [details] [diff] [review]
patch

based on dbaron's comments

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I have a patch to change things so that we don't accept '0' for those, except for arguments to transform functions, which I'll keep at least for now.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

Here's a patch that I just pushed to try.  (It passes the layout/style mochitests.)

Still needs the reftests added, though.  Any chance you could do that?

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #10)
> Here's a patch that I just pushed to try.  (It passes the layout/style
> mochitests.)

Passes full tests on try server on Linux and Mac (still waiting for Windows).

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 410300 [details] [diff] [review]
patch

Patch looks good to me.  Working on reftests now.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: tests

Here's a patch which adds all of the linear gradient tests from http://a-ja.net/newgrad.html to the reftests.  The ones that don't work because of bug 522607 are marked 'fails', as are two tests where we don't generate the exact same gradient for (bottom, blue, yellow) as (top, yellow, blue).

I haven't done the radials because they look less interesting and I'm not sure how to map from -moz-radial-gradient to canvas createRadialGradient.  I might come back to those later.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 410399 [details] [diff] [review]
tests

Looks good, although the tests that almost pass seem odd.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(I'm not in a position to land this today given W3C meetings, but you're welcome to.)

Comment 16

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/eb2a556deb11
http://hg.mozilla.org/mozilla-central/rev/0d248c37d1ae

The almost-passing tests are a gradient alignment issue.  I filed bug 526694.

Comment 17

[Daniel Holbert [:dholbert]]

Looks like "aja-linear-6a.html" is another case of an "almost-passing" test that should be marked as "todo" for now, at least on mac -- it's failed consistently in all 4 mac everythingelse cycles since it landed:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257396811.1257398357.10097.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257395407.1257397803.3960.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257392889.1257394826.3726.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257396489.1257398461.11102.gz

Comment 18

[Daniel Holbert [:dholbert]]

I just marked aja-linear-6a.html as "fails-on-mac", to make it stop oranging the mac everythingelse boxes: http://hg.mozilla.org/mozilla-central/rev/826336c03eb6

Comment 19

[Zack Weinberg (:zwol)]

Filed bug 526708 for the failure dholbert caught.

Comment 20

[Zack Weinberg (:zwol)]

Attachment: tests with bustage fixes

Incorporated the bustage fixes that dholbert and I made to trunk.

I'd like to land both the patches in this bug, together with bug 513395, on 1.9.2 tomorrow assuming no other problems are found.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 410442 [details] [diff] [review]
tests with bustage fixes

a192=beltzner

Comment 22

[Zack Weinberg (:zwol)]

Pushed to releases/1.9.2 together with bug 513395:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a811ab0025c1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/80b0cba026ed