Last Comment Bug 306620 - option and optgroup should match enabled/disabled too
: option and optgroup should match enabled/disabled too
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Allan Beaufour
: Hixie (not reading bugmail)
Mentors:
Depends on: 308270
Blocks: 84400 306621
  Show dependency treegraph
 
Reported: 2005-08-31 14:16 PDT by Allan Beaufour
Modified: 2005-09-28 01:32 PDT (History)
2 users (show)
asa: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.34 KB, patch)
2005-09-01 12:01 PDT, Allan Beaufour
bzbarsky: review+
bzbarsky: superreview+
asa: approval1.8b5+
Details | Diff | Review

Description Allan Beaufour 2005-08-31 14:16:07 PDT
I missed that option and optgroup also have a disabled attribute.
Comment 1 Allan Beaufour 2005-09-01 12:01:12 PDT
Created attachment 194563 [details] [diff] [review]
Patch

Here's a patch.

It basically just adds the AfterSetAttr() functionality to OptGroup and Option.
It's exactly the same code... should I inject a common superclass, or just let
it rest?

We could possibly inject the Before/AfterSetAttr somewhere higher in the object
hierachy, as it is needed in quite a few places now.
Comment 2 Allan Beaufour 2005-09-01 12:02:00 PDT
Testcases here:
http://beaufour.dk/tmp/
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-08 21:48:38 PDT
> We could possibly inject the Before/AfterSetAttr somewhere higher in the object
> hierachy

Separate bug on that, please?
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-08 21:53:20 PDT
Comment on attachment 194563 [details] [diff] [review]
Patch

>Index: content/html/content/src/nsHTMLOptGroupElement.cpp
>+  virtual void AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

Why virtual?

>Index: content/html/content/src/nsHTMLOptionElement.cpp
>+  virtual void AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

Again.

Looks great otherwise.
Comment 5 Asa Dotzler [:asa] 2005-09-15 15:23:52 PDT
please add the fixed1.8 keyword when you land this on the branch
Comment 6 Allan Beaufour 2005-09-27 03:33:56 PDT
Checked in to 1.8 branch
Comment 7 Allan Beaufour 2005-09-27 03:40:56 PDT
Attachment 194563 [details] [diff] does not apply on trunk anymore. Maybe we should wait for bug
308270 before putting this on trunk?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-09-27 07:44:16 PDT
It might be a while before that happens.  Please land this on trunk ASAP so it
can get tested by trunk users too, not just branch users...
Comment 9 Allan Beaufour 2005-09-28 01:32:56 PDT
Checked in to trunk

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