Closed Bug 506554 Opened 15 years ago Closed 14 years ago

Implement the CSS3 pseudo-classes :required and :optional for HTML

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: deprecationmail, Assigned: mounir)

References

()

Details

(Keywords: css3, dev-doc-complete, html5, Whiteboard: parity-Opera)

Attachments

(2 files, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; sv-SE; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: 

Tracking the progress of implementing the CSS3 psuedo-classes :required and
:optional. This discussion at Webkit might be of help:
https://bugs.webkit.org/show_bug.cgi?id=25551

Reproducible: Always
Flags: wanted1.9.2?
Keywords: css3
Whiteboard: parity-Opera
We already implement them in the style system; the question is making particular HTML elements reflect these states when they're supposed to.
Ok, so does that mean that :required and :optional should be green on this page?

http://en.wikipedia.org/wiki/Comparison_of_layout_engines_%28Cascading_Style_Sheets%29
Are there test cases somewhere?  And do you have a pointer to the spec (e.g., HTML5) that says when certain form controls should be in these states?


Also, it generally doesn't work very well when you use the release-blocking nomination flags for features; they're largely used for end-of-the-release-cycle regression fixing (e.g., things that we broke and need to fix before we ship a release, or broken things about already-implemented features that need to be fixed before they cane be shipped), not beginning-of-the-release-cycle decisions about what features to work on.  So requests for new features generally get minused.
Here we go:

http://www.w3.org/TR/html5/interactive-elements.html#selector-required

Could not find a test case though.

To me it seems like they are optional in almost all conditions except for

" * input elements that are required
  * textarea elements that have a required attribute
"

For example, this <textarea> (that I am writing in) does not have a required attribute. Therefore it matches the selector :optional. If it would have the required attribute, it would match the :required selector.

At least, that's how it seems to me.
The correct behavior can currently be seen in Opera and Webkit nightlies.
I figured that the previous textcase would have shown a green background on both textareas even if only the :optional psuedo-class was implemented (as it would ignore the :required part). This still works as expected in Opera and the Webkit nightlies.
Attachment #394022 - Attachment is obsolete: true
Flags: wanted1.9.2?
As a bit of a treat/bonus implementing this would make Gecko the first engine to support all selectors (although some are still in experimental stage).
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Sounds like something we should tie to the required property of form controls.  (Which I'm working on.)
Depends on: 345822
See bug 345624 - that's probably where I'm going to write a patch for this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Implement the CSS3 psuedo-classes :required and :optional → Implement the CSS3 pseudo-classes :required and :optional
Assignee: nobody → mounir.lamouri
Blocks: 344614
Status: NEW → ASSIGNED
Keywords: html5
Attached patch Tests v1 (obsolete) — Splinter Review
Attachment #442692 - Flags: review?(jonas)
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #442693 - Flags: review?(jonas)
Btw, I'm not sure the :optional pseudo-class has a good specification. Button and select elements can't be required so they are always optional. I do not think having optional buttons is needed. It should even make page authors worse. For select element, they should be able to suffer from type mismatch.

I've open a bug here:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=9626

I will change the patch or open a follow-up depending of the answer I got.
Summary: Implement the CSS3 pseudo-classes :required and :optional → Implement the CSS3 pseudo-classes :required and :optional for HTML
Comment on attachment 442693 [details] [diff] [review]
Patch v1

I'd like to hear from bz if that is ok perf-wise.

I don't know how often we call IntrinsicState.

To speed things up we could cache the requiredness in a flag.
Attachment #442693 - Flags: review?(jonas)
Attachment #442693 - Flags: review?(bzbarsky)
Attachment #442693 - Flags: review+
> I don't know how often we call IntrinsicState.

A fair amount, and I plan to make us call it more, but the new code is no slower than existing form control intrinsic state code, right?  When I make us call it more, I'll probably transition the whole thing to a state word, fwiw, or move away from state bits where we get all of them at once or something.

One comment I do have is that I'd prefer "RequiredApplies" to "DoesRequiredApply".
Attachment #442693 - Flags: review?(bzbarsky) → review+
https://wiki.mozilla.org/User:Mounir.lamouri/HTML5_Forms mentions ":required and :selected". Is that a "mistype", or a for me, unknown pseudo-class?
(In reply to comment #16)
> https://wiki.mozilla.org/User:Mounir.lamouri/HTML5_Forms mentions ":required
> and :selected". Is that a "mistype", or a for me, unknown pseudo-class?

That's a mistake. Even in the summary line I wrote ":optional".
Thanks for reporting it :)
(In reply to comment #15)
> > I don't know how often we call IntrinsicState.
> 
> A fair amount, and I plan to make us call it more, but the new code is no
> slower than existing form control intrinsic state code, right?  When I make us
> call it more, I'll probably transition the whole thing to a state word, fwiw,
> or move away from state bits where we get all of them at once or something.
> 
> One comment I do have is that I'd prefer "RequiredApplies" to
> "DoesRequiredApply".

I used Does${Attribute}Apply everywhere so I prefer to keep that naming. Actually, I don't think it really matters as we are going to remove these functions to use something more flexible to know if a type accepts a specific attribute.
Attachment #442693 - Flags: superreview?(Olli.Pettay)
I'm wondering if a sr is really needed here... Olli, do not hesitate to cancel it if you think Jonas and Boris reviews are enough or the patch doesn't worth a super-review.
> I used Does${Attribute}Apply everywhere so I prefer to keep that naming.

Yes, I know you used it everywhere.  And it looks/sounds really weird everywhere to me...  If you prefer to change it throughout, that's fine.

> we are going to remove these

Bug number?

You can treat my review as sr if desired.
Attachment #442693 - Flags: superreview?(Olli.Pettay)
(In reply to comment #20)
> > I used Does${Attribute}Apply everywhere so I prefer to keep that naming.
> 
> Yes, I know you used it everywhere.  And it looks/sounds really weird
> everywhere to me...  If you prefer to change it throughout, that's fine.
> 
> > we are going to remove these
> 
> Bug number?

I've just open bug 563637.

> You can treat my review as sr if desired.

Thanks. Then, no need to bother Olli.
Blocks: 566348
(In reply to comment #2)
> We already implement them in the style system; the question is making
> particular HTML elements reflect these states when they're supposed to.

I know it's quite old now, but I'd like to know; When were these added to the style system, and do they currently "work" on any element at all?
They worked on some elements when using the XForms extension.
(In reply to comment #23)
> They worked on some elements when using the XForms extension.

I see, thanks. When were they added to the style system?
> When were they added to the style system?

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/style/nsCSSPseudoClassList.h&rev=HEAD has that information, no?  Not sure why you need it, but you don't have to ask for it; it's all on the web.
(In reply to comment #25)
> > When were they added to the style system?
> 
> http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/style/nsCSSPseudoClassList.h&rev=HEAD
> has that information, no?  Not sure why you need it, but you don't have to ask
> for it; it's all on the web.

Thanks. I'm not experienced enough to know where to look for it, I have no clue what Bonsai is (in this situation), for example, but it did provide me with the information I needed. For the use, it was about giving accurate information for the Wikipedia page: http://en.wikipedia.org/wiki/Comparison_of_layout_engines_%28Cascading_Style_Sheets%29 where support for these psuedo-classes were incorrectly listed as not supported in Gecko.
Ah, ok.  For HTML purposes they were in fact unsupported, though...

Bonsai's a web interface that lets one access historical CVS information for the mozilla CVS repository.

If you want, I can give you a brief rundown offline on the various webtools you can use to get data of that sort for the Mozilla tree; just let me know by e-mail?
Attached patch Patch v1.1 (obsolete) — Splinter Review
Updated patch to apply against current tip.
Merged with test patch.
Attachment #442692 - Attachment is obsolete: true
Attachment #442693 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
I've done a cleanup on this patch following a discussion with Boris on IRC which make me realize some code was completely useless.
Attachment #460887 - Attachment is obsolete: true
Attached patch Removed code (obsolete) — Splinter Review
Boris, could you confirm this code wasn't needed?

AFAIU, when changing @required IntrinsicState() is going to be called just after updating mAttrsAndChildren then HasAttr(required) will return the new value so nothing should be done in AfterSetAttr except for @type. Right?
That sounds correct, yes.  You should (if we don't have them already) add tests somewhat like this:

  <style>
    input { display: none; }
    input + span { color: red; }
    input[required]:required + span { color: green; }
  </style>
  <input id="x"><span>This should be green</span>
  <script>
    document.body.offsetWidth; 
    document.getElementById("x").setAttribute("required", "required");
  </script>

and a similar test with the colors reversed and removeAttribute.
(In reply to comment #31)
> That sounds correct, yes.  You should (if we don't have them already) add tests
> somewhat like this:

There are already some tests in the patch using setAttribute and removeAttribute for various element/types combination. This is not using the same technique to show the result (text + color) but that's testing the same thing.

Thank you for taking the time to check that change.
Attachment #461920 - Attachment is obsolete: true
> There are already some tests in the patch using setAttribute

Yes, but none of them are testing the situation where a selector is selecting on both the attribute _and_ the state, which is the situation where the exact way we send state change notifications starts to really matter.  For the tests in the patch, as long as we send them at all things will be fine.
Attached patch Patch v1.3Splinter Review
Adding the tests requested by Boris.
Attachment #461918 - Attachment is obsolete: true
This is now in the tree.
rev: http://hg.mozilla.org/mozilla-central/rev/55ef0e0529bc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.