786105 - Setting .style.foo to null no longer removes the property

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Regression changesets

I have attached a log of the changesets in the regression range.  I'm having difficulty narrowing it down further because I'm hitting compiler errors and skipping changesets isn't helping.

Comment 1

[David Zbarsky (:dzbarsky)]

Ok, |hg bisect --skip| was getting confused.  I'm back on track now.

Comment 2

[David Zbarsky (:dzbarsky)]

I'm still bisecting this, but here's the preliminary result:

central  hg log -r e89ead1bbe38:103356
changeset:   103352:e89ead1bbe38
user:        David Zbarsky <[e-mail]>
date:        Fri Aug 24 16:50:57 2012 -0400
summary:     Bug 785486 - DomCameraManager redefines DOM_CAMERA_LOG_LEVEL r=mikeh

changeset:   103353:50430d101d34
user:        Chris Jones <[e-mail]>
date:        Thu Aug 23 17:32:00 2012 -0700
summary:     Bug 785166: Protect against already-canceled vibrations. r=jlebar

changeset:   103354:ee747ea1354f
user:        Ehsan Akhgari <[e-mail]>
date:        Fri Aug 24 17:14:18 2012 -0400
summary:     Bug 785500 - Remove the unused CrossScriptSSA::cx member; r=luke

changeset:   103355:7058cad952ca
user:        Matt Brubeck <[e-mail]>
date:        Fri Aug 24 15:34:50 2012 -0700
summary:     Back out b3c861bd1e2f (bug 785190) because it depends on bug 776208 on a CLOSED TREE

changeset:   103356:f077de66e52d
user:        Benjamin Smedberg <[e-mail]>
date:        Fri Aug 24 13:08:15 2012 -0400
summary:     Revert bug 776208 for semi-consistent failures:

Comment 3

[David Zbarsky (:dzbarsky)]

And none of these patches could have possibly caused this.

Comment 4

[David Zbarsky (:dzbarsky)]

Nevermind, that was the good half of the segment.  Here is the range:
central  hg log -r 103356:103362
changeset:   103356:f077de66e52d
user:        Benjamin Smedberg <[e-mail]>
date:        Fri Aug 24 13:08:15 2012 -0400
summary:     Revert bug 776208 for semi-consistent failures:

changeset:   103357:ead893fc780c
user:        ffxbld
date:        Sat Aug 25 03:21:44 2012 -0700
summary:     Automated blocklist update from host mv-moz2-linux-ix-slave09

changeset:   103358:770b96704cde
parent:      103356:f077de66e52d
user:        Gavin Sharp <[e-mail]>
date:        Tue Jul 24 17:30:32 2012 -0700
summary:     Bug 766616. Part 0. Strings for the error UI. r=mixedpuppy ui-r=Boriss

changeset:   103359:0e8f4d4d62fe
user:        Felipe Gomes <[e-mail]>
date:        Fri Aug 24 17:24:52 2012 -0700
summary:     Bug 782468. Basic telemetry for SocialAPI. r=froydnj,mixedpuppy

changeset:   103360:67ff83142ba5
user:        Dave Herman <[e-mail]>
date:        Fri Aug 24 16:54:40 2012 -0700
summary:     Bug 632027 - comma expressions in array literals are discarded. r=jorendorff

changeset:   103361:75f3cd90e743
user:        Boris Zbarsky <[e-mail]>
date:        Thu Aug 23 21:08:08 2012 -0700
summary:     Bug 753517 part 3.   Expose the API needed for Paris bindings on nsDOMCSSDeclaration and nsICSSDeclaration.

changeset:   103362:c526d9dfb684
user:        Boris Zbarsky <[e-mail]>
date:        Thu Aug 23 21:08:09 2012 -0700
summary:     Bug 753517 part 4.  Set up auto-generation of CSS2Properties.webidl from nsCSSPropList.h and enable Paris b


I'm guessing its bz's patches.

Comment 5

[David Zbarsky (:dzbarsky)]

The first bad revision is:
changeset:   103362:c526d9dfb684
user:        Boris Zbarsky <[e-mail]>
date:        Thu Aug 23 21:08:09 2012 -0700
summary:     Bug 753517 part 4.  Set up auto-generation of CSS2Properties.webidl from nsCSSPropList.h and enable Paris bindings for CSSStyleDeclaration and CSS2Properties.  r=khuey,peterv,dbaron

Comment 6

[Boris Zbarsky [:bzbarsky]]

Just to check, does flipping the new-bindings pref (or perhaps just commenting out the SetIsDOMBinding bits added in that revision) fix the problem?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

@bz: I am told by @dzbrasky that you need a test case. I've fixed Gaia system app for that, so the test case would be simply cloning Gaia and open up |apps/system/index.html|.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So I opened that up.  I get a black screen with a little batter indicator near the top right, and a bunch of errors of various sorts in the error console (some invalid CSS property values, a few things like navigator.mozL10n and navigator.mozKeyboard being undefined, etc).

How do I tell whether I'm seeing this bug?  As in, what are the steps to reproduce after cloning Gaia?  Assuming the right thing to clone is https://github.com/mozilla-b2g/gaia.git at least; if it's not please let me know what I _should_ clone.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

You can just use this: 
http://people.mozilla.org/~tchien/bug786105-testcase/

STR can be seem here: https://github.com/mozilla-b2g/gaia/issues/3871

1. Touch the unlock handle and move it right until the unlock icon turns blue
2. release the icon, let the blue glow move to the unlock icon

Expected:

1. After the glow move to the unlock icon, the lock screen unlocks

Actual:

2. the glow move to the unlock icon and nothing happens.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Perfect, thanks.  With that testcase and those steps I see the problem.

Comment 11

[Boris Zbarsky [:bzbarsky]]

And I can confirm that flipping the bindings off makes the problem go away, ok.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, so what's going on here is that handleMove does this:

  this.areaHandle.style.MozTransition = 'none';

(presumably to shadow some less-specific transition rule?) and then handleGesture does this:

  this.areaHandle.style.MozTransition = null;

This used to pass an empty (and voided) string down to setProperty, which made it act like removeProperty.  But in the new bindings null becomes the string "null", which is then passed to setProperty, and that's a perfectly ok value for 'transition', so we continue to shadow the presumed less-specific rule.

Need to check what other UAs do with null here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Looks like WebKit and Presto do what we used to do.

Trident does what the new bindings do.

I suppose the old thing is probably more web-compatible, for us.  But which one do we want the web to have long-term?

Comment 14

[Boris Zbarsky [:bzbarsky]]

In any case, using "" instead of null in the Gaia code there would make it work no matter what.  Or just removeProperty if that's what you're really trying to do.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> In any case, using "" instead of null in the Gaia code there would make it
> work no matter what.  Or just removeProperty if that's what you're really
> trying to do.

I'll take that and push some fixes to Gaia. Thank you very much.

Comment 16

[Boris Zbarsky [:bzbarsky]]

You're welcome.  Thank you for the standalone testcase!  It made this much simpler to deal with.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Setting inline style properties to null should remove them, just like setting them to empty string does.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.

This adds support for [TreatNullAs] and [TreatUndefinedAs] on attributes.  Maybe I should have done that in a separate changeset...

In any case, Kyle, if you get a chance to glance at this that would rock.

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.

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

::: dom/bindings/Codegen.py
@@ +3296,5 @@
>          self.optional = False
>          self.variadic = False
>          self.defaultValue = None
> +        self.treatNullAs = interfaceMember.treatNullAs
> +        self.treatUndefinedAs = interfaceMember.treatUndefinedAs

Does this really make sense? For a setter it doesn't and the one other use doesn't need it (it's a boolean).

::: dom/bindings/GenerateCSS2PropertiesWebIDL.py
@@ +7,5 @@
>  
>  propList = eval(sys.stdin.read())
>  props = ""
>  for [prop, pref] in propList:
> +    pref = ', Pref=%s' % pref if pref is not "" else ""

Make an extended attributes array:

  extendedAtts = ["TreatNullAs=EmptyString"]
  if pref is not "":
    extendedAtts.append('Pref=%s' % pref)

And then ','.join() it below.

::: dom/bindings/parser/WebIDL.py
@@ +333,5 @@
> +            (attr, value) = attrAndValue
> +            if attr == "TreatNullAs":
> +                if not self.type.isString() or self.type.nullable():
> +                    raise WebIDLError("[TreatNullAs] is only allowed on "
> +                                      "arguments whose type is DOMString",

"arguments or attributes"? Here and below.

@@ +336,5 @@
> +                    raise WebIDLError("[TreatNullAs] is only allowed on "
> +                                      "arguments whose type is DOMString",
> +                                      [self.location])
> +                if self.dictionaryMember:
> +                    raise WebIDLError("[TreatNullAs] is not allowed for "

I wonder if it wouldn't make more sense to keep this and the 'Missing' stuff in IDLArgument? Or pass dictionaryMember and optional in as arguments and always pass |False, False| from IDLAttribute.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Does this really make sense? For a setter it doesn't

Er.. How does it not make sense for a setter?  The whole point of that part is that I want to toss a [TreatNullAs=EmptyString] on a setter!

> Make an extended attributes array:

Nice.  Will do.

> "arguments or attributes"? Here and below.

Will do.

> I wonder if it wouldn't make more sense to keep this and the 'Missing' stuff in
> IDLArgument?

I considered that, but it complicates the logic a good bit.  I could switch to using arguments in the python instead of relying on self.dictionaryMember and self.optional always being false for attributes.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.

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

::: layout/reftests/cssom/inline-style-null.html
@@ +3,5 @@
> +  div { color: green; }
> +  div#reverse { color: red; }
> +</style>
> +<div id="null" style="color: red">This text should be green</div>
> +<div id="emptystring"style="color: red">This text should be green</div>

Space between attributes

Comment 22

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #20)
> Er.. How does it not make sense for a setter?  The whole point of that part
> is that I want to toss a [TreatNullAs=EmptyString] on a setter!

Yeah, nevermind, should have deleted that bit (got confused about what we use FakeArgument for). Still feels awkward for the special operations one.

> I considered that, but it complicates the logic a good bit.  I could switch
> to using arguments in the python instead of relying on self.dictionaryMember
> and self.optional always being false for attributes.

Yeah, I'd prefer arguments.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Still feels awkward for the special operations one.

Well, it's just setting it to the default values always in that case...

Do you have any suggestions for how to make this less awkward?

Comment 24

[Peter Van der Beken [:peterv]]

You can leave it as is. The only way I could think of is again to pass it as args with a default value of "Default".

Comment 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/078f3af11d13 with the above fixes.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 753517
User impact if declined: Possible web compat change (switching from the
   Opera/WebKit/Firefox behavior to the IE behavior).  And a change the other way
   the next cycle.
Testing completed (on m-c, etc.): Passes automated tests.
Risk to taking this patch (and alternatives if risky): Risk should be low.
String or UUID changes made by this patch:

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/078f3af11d13

Comment 28

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ef2cd6879fa

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.

I think the window to give useful feedback has passed.

Comment 30

[u279076]

No QA verification since this has in-testsuite coverage. Please add verifyme keyword to request manual verification.