Closed
Bug 786105
Opened 12 years ago
Closed 12 years ago
Setting .style.foo to null no longer removes the property
Categories
(Core :: DOM: CSS Object Model, defect)
Core
DOM: CSS Object Model
Tracking
()
People
(Reporter: dzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: regression, Whiteboard: [qa-])
Attachments
(2 files)
16.74 KB,
text/plain
|
Details | |
13.85 KB,
patch
|
peterv
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
Ok, |hg bisect --skip| was getting confused. I'm back on track now.
Reporter | ||
Comment 2•12 years ago
|
||
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:
Reporter | ||
Comment 3•12 years ago
|
||
And none of these patches could have possibly caused this.
Reporter | ||
Comment 4•12 years ago
|
||
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.
Reporter | ||
Comment 5•12 years ago
|
||
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
Updated•12 years ago
|
Assignee: nobody → dzbarsky
blocking-basecamp: --- → +
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
@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|.
Updated•12 years ago
|
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
Perfect, thanks. With that testcase and those steps I see the problem.
Assignee | ||
Comment 11•12 years ago
|
||
And I can confirm that flipping the bindings off makes the problem go away, ok.
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
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?
Assignee: dzbarsky → nobody
Component: General → DOM: CSS Object Model
Summary: B2G unlocking broken (possibly transitionend no longer firing?) → Setting .style.foo to null no longer removes the property
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
You're welcome. Thank you for the standalone testcase! It made this much simpler to deal with.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #656327 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 18•12 years ago
|
||
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.
Attachment #656327 -
Flags: feedback?(khuey)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [need review]
Comment 19•12 years ago
|
||
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.
Attachment #656327 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 20•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 23•12 years ago
|
||
> 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•12 years ago
|
||
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".
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/078f3af11d13 with the above fixes.
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 26•12 years ago
|
||
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:
Attachment #656327 -
Flags: approval-mozilla-aurora?
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/078f3af11d13
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #656327 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 28•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ef2cd6879fa
status-firefox17:
--- → fixed
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.
Attachment #656327 -
Flags: feedback?(khuey)
Comment 30•12 years ago
|
||
No QA verification since this has in-testsuite coverage. Please add verifyme keyword to request manual verification.
Keywords: verifyme
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•