1212680 - Devtools test "browser_ruleview_authored.js" fails if native webkit gradient prefixes are enabled

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Apply bug 1210575 "part 5" to current mozilla-inbound, and run devtools test browser_ruleview_authored.js

ACTUAL RESULTS:
{
105 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleinspector/test/browser_ruleview_authored.js | post-change check overridden for 0 - Got true, expected false
106 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleinspector/test/browser_ruleview_authored.js | post-change check overridden for 1 - Got false, expected true 
}
Sample test log from a Try run with that patch:
https://treeherder.mozilla.org/logviewer.html#?job_id=12380184&repo=try

EXPECTED RESULTS: No test failure.

This test has an "overrideTest" function which has a series of gradients, including a "-webkit-linear-gradient". I'm guessing that's what's involved here, and the test is getting confused because it's expecting us to reject that expression, but we now will accept it.


NOTE for any local live testing outside of the test harness: along with applying the patch mentioned in the STR, one also needs to toggle the pref "layout.css.prefixes.webkit" before these prefixed gradients will start working. That pref is disabled by default, but enabled during test runs.

tromey, do you know what we need to change in this test for it to keep working after we support "-webkit-linear-gradient" natively?

Comment 1

[Daniel Holbert [:dholbert]]

[ni=tromey for question at the end of comment 0.]

Comment 2

[Daniel Holbert [:dholbert]]

Perhaps we could replace "-webkit-linear-gradient" with "-ms-linear-gradient" in this test? As far as I know, we aren't planning on adding native support for that.

(-ms-linear-gradient is indeed a thing, per this MSDN blog: http://blogs.msdn.com/b/ie/archive/2012/06/25/unprefixed-css3-gradients-in-ie10.aspx )

Comment 3

[Daniel Holbert [:dholbert]]

Side notes: 
 (1) I verified that the "-ms-linear-gradient" is recognized in IE11, by testing http://jsfiddle.net/ef8bp9rj/3/

 (2) Incidentally, the gradient you're using here is a bit complex and also transparent, which means it's completely invisible on a page with a white background. (This caused me some confusion when I first started trying to play around with it to see if it was valid or not.) Is its transparency/complexity necessary?  If it's OK, we should be able to swap in e.g. "linear-gradient(orange, blue)" and rip out the complicated expression here.

 (3) The test currently assumes that -moz-linear-gradient is supported. That may not always be the case, per bug 1176496. (We tried to unsupport them once, and failed due to web-compat issues. We may try again at some point.)  If it's possible to just use two different "linear-gradient" expressions at position #0 and #2 (instead of using a moz-prefixed expression at #0), that may be more future-proof.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1: use -ms instead of -webkit, drop dependence on -moz, and simplify gradient expressions

Here's a strawman patch which addresses my thoughts in comment 2 and 3.

I verified that the test passes locally, with this applied. (on top of bug bug 1210575 part 5)

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8671121 [details] [diff] [review]
fix v1: use -ms instead of -webkit, drop dependence on -moz, and simplify gradient expressions

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

Thanks for doing this.

Comment 6

[Tom Tromey :tromey]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Side notes: 
>  (1) I verified that the "-ms-linear-gradient" is recognized in IE11, by
> testing http://jsfiddle.net/ef8bp9rj/3/
> 
>  (2) Incidentally, the gradient you're using here is a bit complex and also
> transparent, which means it's completely invisible on a page with a white
> background. (This caused me some confusion when I first started trying to
> play around with it to see if it was valid or not.) Is its
> transparency/complexity necessary?  If it's OK, we should be able to swap in
> e.g. "linear-gradient(orange, blue)" and rip out the complicated expression
> here.

Yeah, I don't think the content matters so much.
What the test is trying to test here is that if the user disables one
property, then the rule-view knows that an invalid property value won't
be used.

>  (3) The test currently assumes that -moz-linear-gradient is supported. That
> may not always be the case, per bug 1176496. (We tried to unsupport them
> once, and failed due to web-compat issues. We may try again at some point.) 
> If it's possible to just use two different "linear-gradient" expressions at
> position #0 and #2 (instead of using a moz-prefixed expression at #0), that
> may be more future-proof.

I think that is fine as well.

Comment 7

[Daniel Holbert [:dholbert]]

Thanks for the review!

(In reply to Tom Tromey :tromey from comment #6)
> >  (2) Incidentally, the gradient you're using here is a bit complex and also
> > transparent [...]
> 
> Yeah, I don't think the content matters so much.
> What the test is trying to test here is that if the user disables one
> property, then the rule-view knows that an invalid property value won't
> be used.

Cool. In that case, I'd say the one thing that *does* matter about the gradient-expression here is that it should be easy to play around with to diagnose what's going wrong, if & when the test fails.  And in that respect, the new gradient is hopefully easier, since it renders something instead of nothing. :)

I'll land this shortly.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f0e5491de5

Comment 9

[Carsten Book [:Tomcat]]

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