Closed Bug 637019 Opened 9 years ago Closed 9 years ago

regression at www.staples.com - Sorting feature doesn't work (Sort by A-Z; Z-A; etc)

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: cab26715, Assigned: mounir)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [hardblocker])

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110226 Firefox/4.0b13pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110226 Firefox/4.0b13pre

When searching for items on Staples.com, I cannot sort the results by price (low to high) or name (A to Z).

The example URL is a search for "Microsoft Office".

Reproducible: Always
Regression Range:

Works = http://hg.mozilla.org/mozilla-central/rev/881f4ab63c31
BROKEN (2/25 Nightly) = http://hg.mozilla.org/mozilla-central/rev/d7ef42d7782c
Keywords: regression
Summary: www.staples.com - Sorting feature doesn't work (Sort by A-Z; Z-A; etc) → *** REGRESSION *** www.staples.com - Sorting feature doesn't work (Sort by A-Z; Z-A; etc)
Also broken (2/26/11 Nightly) = http://hg.mozilla.org/mozilla-central/rev/d708c2fa7fea
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/30b87b9ea0cc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre ID:20110224202009
Fails:
http://hg.mozilla.org/mozilla-central/rev/c2c9c3eb5c2c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre ID:20110224203828
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30b87b9ea0cc&tochange=c2c9c3eb5c2c
Blocks: 633133
blocking2.0: --- → ?
Component: General → DOM: Core & HTML
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #3)
> Regression window:
> Works:
> http://hg.mozilla.org/mozilla-central/rev/30b87b9ea0cc
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110224
> Firefox/4.0b13pre ID:20110224202009
> Fails:
> http://hg.mozilla.org/mozilla-central/rev/c2c9c3eb5c2c
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110224
> Firefox/4.0b13pre ID:20110224203828
> Pushlog:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=30b87b9ea0cc&tochange=c2c9c3eb5c2c

Thanks, Alice!  You're better at narrowing down the range than I am.
cc'ing blame-able folks for hopefully quick eyes on nominated blocker.
Attached file Testcase #1
SELECT.onchange is undefined, should be a function object.
Keywords: testcase
OS: Windows 7 → All
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
This is a regression from:
https://hg.mozilla.org/mozilla-central/rev/c2c9c3eb5c2c

Blake forgot to call nsElementSH::NewResolve if nsHTMLSelectElementSH::NewResolve find nothing.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #515456 - Flags: review?(jst)
I guess this should hardblock...
Whiteboard: [has patch][needs review]
Why not make the onchange return some value and then actually calling it
and test the result?
(In reply to comment #10)
> Why not make the onchange return some value and then actually calling it
> and test the result?

Because we want to check if onchange is correctly resolved and the simplest and straightforward way to do that is to check if it's undefined or not. Checking the returned value would be less clear (even if, indeed, it would fail if it's not resolved).
Ok, but resolved to what?  I think we want a test that fails if onchange
is not callable.  That's what the site was actually doing, traversing
the DOM replacing all the onchange, onfocus, onblur etc with new functions
to track the activity and then call the original function.
Attached patch Patch v1Splinter Review
Add a test checking that select.onchange type is 'function'.
Attachment #515456 - Attachment is obsolete: true
Attachment #515456 - Flags: review?(jst)
Attachment #515467 - Flags: review?(jst)
Whiteboard: [has patch][needs review] → [has patch][needs review][passed-try]
Summary: *** REGRESSION *** www.staples.com - Sorting feature doesn't work (Sort by A-Z; Z-A; etc) → regression at www.staples.com - Sorting feature doesn't work (Sort by A-Z; Z-A; etc)
blocking2.0: ? → final+
Whiteboard: [has patch][needs review][passed-try] → [has patch][needs review][passed-try][hardblocker]
Comment on attachment 515467 [details] [diff] [review]
Patch v1

r=me if you add a |return NS_OK| in the block where you call JS_DefineElement; without that the patch is wrong.

Also, this test will start failing once we implement what the HTML5 spec says for onchange.  At that point we'll need to replace it with a test that actually checks whether the handler ran, right?
Attachment #515467 - Flags: review?(jst) → review+
Whiteboard: [has patch][needs review][passed-try][hardblocker] → [passed-try][hardblocker][can land]
Whiteboard: [passed-try][hardblocker][can land] → [passed-try][hardblocker][can land][has patch]
Fix landed (with missing return NS_OK added):

http://hg.mozilla.org/mozilla-central/rev/cd1aa5bb0d1a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Whiteboard: [passed-try][hardblocker][can land][has patch] → [hardblocker]
Target Milestone: --- → mozilla2.0
(In reply to comment #14)
> Also, this test will start failing once we implement what the HTML5 spec says
> for onchange.  At that point we'll need to replace it with a test that actually
> checks whether the handler ran, right?

It's not really clear to me how different things are going to be when we will implement event handlers the way HTML5 specs it. I mean, I don't know what |attribute Function| exactly change.
Anyway, I guess |"onchange" in select| will still work and we should have had already some tests checking that the handlers work. Actually, we could extend content/html/content/test/test_bug618948.html to <select> or simply create a real test suite for all event handlers while doing the change requested by HTML5 specs.
Oh, it says |Function| now.  Didn't use to...  In that case the test will probably be fine.

|"onchange" in select| will always test true after that change, but the typeof check will catch thing.
You need to log in before you can comment on or make changes to this bug.