Last Comment Bug 786105 - Setting .style.foo to null no longer removes the property
: Setting .style.foo to null no longer removes the property
Status: RESOLVED FIXED
[qa-]
: regression
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
https://github.com/mozilla-b2g/gaia/i...
Depends on:
Blocks: 753517
  Show dependency treegraph
 
Reported: 2012-08-27 16:49 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-10-17 16:54 PDT (History)
10 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+
fixed


Attachments
Regression changesets (16.74 KB, text/plain)
2012-08-27 16:49 PDT, David Zbarsky (:dzbarsky)
no flags Details
Setting inline style properties to null should remove them, just like setting them to empty string does. (13.85 KB, patch)
2012-08-28 22:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-08-27 16:49:54 PDT
Created attachment 655821 [details]
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) 2012-08-27 16:59:39 PDT
Ok, |hg bisect --skip| was getting confused.  I'm back on track now.
Comment 2 David Zbarsky (:dzbarsky) 2012-08-27 17:48:33 PDT
I'm still bisecting this, but here's the preliminary result:

central  hg log -r e89ead1bbe38:103356
changeset:   103352:e89ead1bbe38
user:        David Zbarsky <dzbarsky@gmail.com>
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 <jones.chris.g@gmail.com>
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 <ehsan@mozilla.com>
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 <mbrubeck@mozilla.com>
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 <benjamin@smedbergs.us>
date:        Fri Aug 24 13:08:15 2012 -0400
summary:     Revert bug 776208 for semi-consistent failures:
Comment 3 David Zbarsky (:dzbarsky) 2012-08-27 17:50:17 PDT
And none of these patches could have possibly caused this.
Comment 4 David Zbarsky (:dzbarsky) 2012-08-27 18:07:22 PDT
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 <benjamin@smedbergs.us>
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 <gavin@gavinsharp.com>
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 <felipc@gmail.com>
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 <dherman@mozilla.com>
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 <bzbarsky@mit.edu>
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 <bzbarsky@mit.edu>
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) 2012-08-27 18:34:28 PDT
The first bad revision is:
changeset:   103362:c526d9dfb684
user:        Boris Zbarsky <bzbarsky@mit.edu>
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 [:bz] (still a bit busy) 2012-08-27 21:52:33 PDT
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) 2012-08-28 07:23:12 PDT
@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 [:bz] (still a bit busy) 2012-08-28 08:56:16 PDT
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) 2012-08-28 11:01:15 PDT
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 [:bz] (still a bit busy) 2012-08-28 11:12:54 PDT
Perfect, thanks.  With that testcase and those steps I see the problem.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-08-28 11:14:20 PDT
And I can confirm that flipping the bindings off makes the problem go away, ok.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-08-28 13:40:14 PDT
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 [:bz] (still a bit busy) 2012-08-28 13:52:08 PDT
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 [:bz] (still a bit busy) 2012-08-28 13:53:55 PDT
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) 2012-08-28 13:56:41 PDT
(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 [:bz] (still a bit busy) 2012-08-28 14:09:47 PDT
You're welcome.  Thank you for the standalone testcase!  It made this much simpler to deal with.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2012-08-28 22:03:38 PDT
Created attachment 656327 [details] [diff] [review]
Setting inline style properties to null should remove them, just like setting them to empty string does.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-08-28 22:05:10 PDT
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] 2012-08-31 07:45:13 PDT
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 [:bz] (still a bit busy) 2012-08-31 08:31:57 PDT
> 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 (⌚ UTC+1/+2) 2012-08-31 08:37:41 PDT
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] 2012-08-31 09:13:41 PDT
(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 [:bz] (still a bit busy) 2012-08-31 09:18:49 PDT
> 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] 2012-08-31 09:27:38 PDT
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 [:bz] (still a bit busy) 2012-08-31 18:00:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/078f3af11d13 with the above fixes.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2012-08-31 18:01:49 PDT
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] 2012-09-01 05:49:44 PDT
https://hg.mozilla.org/mozilla-central/rev/078f3af11d13
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2012-09-04 19:57:50 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ef2cd6879fa
Comment 29 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-22 12:31:09 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 16:54:26 PDT
No QA verification since this has in-testsuite coverage. Please add verifyme keyword to request manual verification.

Note You need to log in before you can comment on or make changes to this bug.