Closed
Bug 952365
Opened 11 years ago
Closed 11 years ago
Bug 932322 breaks dell.com
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | + | verified |
firefox29 | + | verified |
People
(Reporter: glandium, Assigned: bzbarsky)
References
Details
(Keywords: regression, site-compat, Whiteboard: [bugday-20140115])
Attachments
(4 files, 3 obsolete files)
4.65 KB,
patch
|
Details | Diff | Splinter Review | |
23.54 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
23.39 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
431.36 KB,
image/png
|
Details |
On the Dell site, when going to product pages, there is a menu on the left with "Configurations", "Features & Design", "Accessories & Services", etc. and information matching the selected item on the right side.
See http://www.dell.com/us/p/xps-13-9333/pd?oc=dncwp1238h&model_id=xps-13-9333 for example.
This works fine on release and beta, and not in aurora and nightly, where both the menu and the content are just gone (which, incidentally, makes it impossible to buy anything)
Thanks to mozregression, I could pinpoint bug 932322 as the root cause of the breakage. I don't have more time to investigate whether bug 932322 actually broke something or if Dell is doing something wrong.
I see an error in the web console on that page that might be related?
TypeError: $switchCTA.offset(...) is null
jsmin.ashx:152
That line in the file is this:
if($(".switchCTA").length>0){var $switchCTA=$(".switchCTA:visible");zombieStart=parseInt($switchCTA.offset().top)+parseInt($switchCTA.height());}
This error does not show up on my release version of Firefox.
Assignee | ||
Updated•11 years ago
|
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
Assignee | ||
Updated•11 years ago
|
Keywords: qawanted,
testcase-wanted
Assignee | ||
Comment 2•11 years ago
|
||
This seems to do with putting "onload" directly on the global.
The exception in comment 1 seems to be in http://www1-cdn.dell.com/JS/default/jsmin.ashx?set=magPd&theme=default&cmp=1&version-13.11.11.1 which has a bunch of places that set bareword "onload" without declaring it as a var, but the behavior of that should not have changed (should set to null both before and after).
It also has some "var onload", but they're all in functions...
Assignee | ||
Comment 3•11 years ago
|
||
This is moderately insane, actually.
The page includes http://www1-cdn.dell.com/JS/default/jsmin.ashx?set=core&theme=default&cmp=1&version=13.11.11.1 which has at toplevel:
var onload;
then the script linked in comment 2 comes later and has a bunch of non-var-prefixed bareword things like this in functions:
onload = { foo: bar };
doSomething(onload);
So before bug 932322 this shadowed the "onload" on Window.prototype and it all worked. But now we see that the passed-in thing is not callable and per spec set window.onload to null on that bareword set. Then the doSomething() call doesn't work right.
So I tried experimenting a bit as to what other UAs do. Here are the results:
Chrome: Assigning non-callables to window.onload normally fails. However, "var onload"
somehow manages to shadow the normal onload property, because Chrome actually
puts that property on Window.prototype! In fact, it does that for all the on*
properties I've tested so far, even though other properties (like
window.performance) go directly on the Window object.
Safari: Puts "onload" directly on the Window, and ignores "var onload" as expected, but
generally allows assigning _any_ object, not just a callable, to any event
handler DOM attribute. If the object is not callable, the event handler just
seems to silently do nothing?
Opera: Seems to have the same behavior as Gecko used to have: the on* properties are on
the prototype and only allow callables. Note that this is Presto-based Opera,
not Chromium-based.
IE11: Seems to put "onload" on Window.prototype but allows assigning any object like
Safari, so is OK for both reasons.
IE9: Same as IE11.
I'm going to post to public-script-coord with a note about this stuff, but how do we actually want to make this work for now? Seems like we should probably back out bug 932322 for the moment and then figure out what we actually want to implement and propose spec changes. :(
Flags: needinfo?(peterv)
Flags: needinfo?(cam)
Assignee | ||
Comment 4•11 years ago
|
||
Or I guess we could hope that Dell is the only site doing this sort of thing, but I'm not sure I want to rely on that.
Assignee | ||
Updated•11 years ago
|
Keywords: qawanted,
testcase-wanted
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
Of all the different behaviours, I like Safari's the most. It seems less special-casey than treating any non-callable thing as null. I think I also prefer having all the Window members on the instance, rather than splitting them up between the instance and Window.prototype.
Updated•11 years ago
|
Flags: needinfo?(cam)
Assignee | ||
Comment 7•11 years ago
|
||
Safari's behavior requires adding a new kind of IDL type (possibly replacing [TreatNonCallableAsNull] callbacks?), right? A type just like a callback function, but that takes any object and is a no-op if the object is not callable?
Assignee | ||
Comment 8•11 years ago
|
||
Also, note that Safari still treats non-objects as null...
Comment 9•11 years ago
|
||
Sigh. So I wonder if treating non-objects as null is really required, or whether that was just the easiest thing to do to handle random values being assigned to onload while keeping the internal type being EventHandler (or whatever equivalent they would have had at the time).
Assignee | ||
Comment 10•11 years ago
|
||
I have no idea, but I'm not all that willing to experiment with changing something that every single UA does.
One other note: Treating non-callables differently from callables is not hard to do in JS proper: just check that typeof(value) == "function". Treating all objects differently from all non-objects actually requires a lot more going out of one's way (though obviously not in Gecko's C++, where that's an easy test).
Comment 11•11 years ago
|
||
Is the backout of bug 932322 the only (and less risky) option here? If so we should definitely make this happen in Aurora so please nominate for landing.
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
Assignee | ||
Comment 12•11 years ago
|
||
It's not clear which is riskier. Backing out would get us back to a behavior we used to have, but it's not clear how much stuff has been added since that depends on the new behavior...
Assignee | ||
Comment 13•11 years ago
|
||
Talked to Peter. We're going to try the non-object thing on both m-c and aurora.
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8358095 -
Flags: review?(peterv)
Assignee | ||
Comment 15•11 years ago
|
||
Just fixed up merge conflicts; no substantive changes, so I don't think this needs extra review
Assignee | ||
Comment 16•11 years ago
|
||
So there is one test failure: dom/imptests/html/html/webappapis/scripting/events/test_event-handler-spec-example.html
This test comes down more or less to this:
var div = document.createElement("div");
div.onclick = {};
var z = 0;
div.addEventListener("click", function(e) { z = 1; });
div.onclick = function(e) { z = 2;}
div.dispatchEvent(new Event("click"));
assert(z == 2);
We used to pass because the first onclick assignment didn't actually register an event listener (since it got set to null), so the z = 2 listener ran after the z = 1 listener. But now we add the listener at the first assignment, then update it in place, so z = 2 runs before z = 1.
I checked other browsers, and WebKit/Blink seem to add a new listener on every onclick assignment, while IE11 has the behavior we would have with this patch. So I have no problem with the behavior change. The only question is how the test should be fixed. I can change the expected values, or I can replace that "{}" with "5" or something to make it continue to have the "set to null" behavior. Ms2ger, do you know what the intent of this test is?
I guess ideally we'd fork the test and test it both ways....
Flags: needinfo?(Ms2ger)
Comment 17•11 years ago
|
||
I expanded the test: https://github.com/w3c/web-platform-tests/pull/506
Flags: needinfo?(Ms2ger)
Assignee | ||
Comment 18•11 years ago
|
||
Awesome, thanks! I'll just need to land the import of that with this patch...
Comment 19•11 years ago
|
||
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8358468 -
Flags: review?(peterv)
Assignee | ||
Updated•11 years ago
|
Attachment #8358095 -
Attachment is obsolete: true
Attachment #8358095 -
Flags: review?(peterv)
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8358136 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: regression
Whiteboard: regression → [need review]
Comment 22•11 years ago
|
||
Comment on attachment 8358468 [details] [diff] [review]
With the test update rolled in
Review of attachment 8358468 [details] [diff] [review]:
-----------------------------------------------------------------
I suppose you're removing TreatNonCallableAsNull in a followup to minimize risk?
::: dom/bindings/Codegen.py
@@ +11214,1 @@
> " ${argc}, ${argv}, rval.address())) {\n"
Wrapping and whitespace end up a bit weird in the generated code here. Not sure how to make it better.
::: dom/imptests/html/html/webappapis/scripting/events/test_event-handler-spec-example.html
@@ +7,5 @@
> +var objects = [{}, function() {}, new Number(42), new String()];
> +var primitives = [42, null, undefined, ""];
> +objects.forEach(function(object) {
> + var t = async_test("Event handler listeners should be registered when they " +
> + "are first set to a object value (" +
an object value
Attachment #8358468 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 23•11 years ago
|
||
> I suppose you're removing TreatNonCallableAsNull in a followup to minimize risk?
No, I'm not removing it at all because we actually need it for Promise.... That's not a webidl spec issue, because Promise is not even being specced in WebIDL.
> Wrapping and whitespace end up a bit weird
Yeah. I could make it better by creating a separate if for the callGuard and making it "true" when we just want to always make the call, if you prefer...
> an object value
Good catch. ;)
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7770d2e633b1 with the imptest fix left to ms2ger.
Assignee | ||
Comment 25•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8358469 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 8359379 [details] [diff] [review]
Aurora patch with fixed commit message
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 932322
User impact if declined: Some sites are broken (e.g. dell.com)
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): Risk is moderate,
especially for web compat. It helps a bit that we're switching to an existing
(IE11) behavior... The other option it trying to back out bug 932322, which
seems riskier.
String or IDL/UUID changes made by this patch: None.
Attachment #8359379 -
Flags: approval-mozilla-aurora?
Comment 27•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 29•11 years ago
|
||
fix works for me on firefox 29, linux X86_64.
Whiteboard: [need review] → [need review] [bugday-20140115]
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need review] [bugday-20140115] → [bugday-20140115]
Comment 30•11 years ago
|
||
Comment on attachment 8359379 [details] [diff] [review]
Aurora patch with fixed commit message
That this only needs to uplift to Aurora buys some time and it's the less risky option between landing this and doing a backout - approving.
Attachment #8359379 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 31•11 years ago
|
||
Comment 32•11 years ago
|
||
Mike, can you please verify this is fixed for you in today's Nightly and tomorrow's Aurora?
Flags: needinfo?(mh+mozilla)
Comment 34•11 years ago
|
||
Fix is not working for me on Aurora, Linux 32 Bit.
Comment 35•11 years ago
|
||
Assignee | ||
Comment 36•11 years ago
|
||
AShickur Rahman, your build doesn't have this fix. Note the "tomorrow" in comment 32.
Comment 37•11 years ago
|
||
ohh, yes, I mixed up with time zone.
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Keywords: site-compat
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•