I recall #webkit scrollback to the effect that their implementation wasn't as simple as the rule addition made here, for reasons of perf considerations of checking for an attribute on every element during style resolution. (I think that's the term I want, somebody tell me if I'm mistaken.) Do we not have to worry about this method of implementation having performance impact?

Uh, right. Need to check how this affects to performance.

> Do we not have to worry about this method of implementation having performance > impact? We do, in general. Not sure how much it would show up in practice so far, but as we optimize more and more it could become noticeable.... It's worth looking at how the webkit folks implemented theirs.

What do we do when we check for matches to the :lang() pseudo-class, for example?

I'm not sure I follow the question in comment 7.... Also note that we already have [dir] rules in html.css. :( This would be no worse....

Since the :lang pseudo-class requires a check of the "lang" attribute on every element, I wondered how we handled that, since it might help us in this bug.

We check the lang attribute. The :lang pseudo-class is rarely used. This patch, however, would have the [hidden] check for all HTML elements.

And again, it may be that the concern is not important. Can we get some numbers here (using a stylesheet with such a rule in it, say)? What did webkit end up doing?

https://bugs.webkit.org/show_bug.cgi?id=40511 https://bug-40511-attachments.webkit.org/attachment.cgi?id=58533 http://trac.webkit.org/changeset/61052

Thanks. We could do that sort of thing, though our mapped attr code is more similar to our style resolution code (involves attr gets during resolution time). But it would only happen for elements with mapped attributes... If we want to do that, you'd want to add it to nsGenericHTMLElement::sCommonAttributeMap and change nsGenericHTMLElement::MapCommonAttributesInto. It's worth getting numbers on the performance impact before resorting to the complicated way, seems like.

Using the attribute mapping code isn't all that complicated, and I think it's also more consistent with other code. And I suspect there would be a big perf impact from changing everything from the attribute mapping code to being written in CSS. There's also the question of whether this belongs at the preshint or UA level of the cascade.

That had crossed my mind, but I think putting it in preshint for the time being is fine.

Comment on attachment 451887 [details] [diff] [review] Patch v1.2, merged to tip dbaron says that it would be more performant to implement this using nsGenericHTMLElement::MapCommonAttributesInto/nsGenericHTMLElement::sCommonAttributeMap. Check it to that and ask me for review. Would be great to have this in Firefox 4.

... that's instead of the html.css changes.

This doesn't work. I can't find any documentation that explains what I should do instead. David, perhaps you could tell me?

You don't want mDisplay.SetNoneValue(); instead you want mDisplay.SetIntValue(NS_STYLE_DISPLAY_NONE, eCSSUnit_Enumerated).

Comment on attachment 458096 [details] [diff] [review] Mapped attribute WIP (and otherwise this looks fine, assuming that makes it work... except for the fact that the html.css change doesn't need to be added in the other patch and removed in this one)

Thanks, that did make it work.

Doesn't that patch also need to check that the display is not already set? Seems like this: <style> div { display: block !important; } </style> <div hidden>This text should show up</div> would not work with the patch as written...

Er, right, it does. I forgot about that because I was looking at the lang case above, which is the exception. It should probably look like: if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Display)) { nsRuleDataDisplay *disp = aData->mDisplayData; if (disp->mDisplay.GetUnit() == eCSSUnit_Null) { if (aAttributes->IndexOfAttr(nsGkAtoms::hidden, kNameSpaceID_None) >= 0) { disp->mDisplay.SetIntValue(NS_STYLE_DISPLAY_NONE, eCSSUnit_Enumerated); } } }

Thanks, it looks like that works.

Presumably comment 22 should be added as a reftest too...

Comment on attachment 458397 [details] [diff] [review] Patch v2.1 r=me if you also add the testcase in comment 22 as a reftest.

Done.

You need to request approval2.0 on your patch before it can land.

http://hg.mozilla.org/mozilla-central/rev/b1d534754b62 Thanks!

I've updated the documentation: https://developer.mozilla.org/en/HTML/Global_attributes#attr-hidden and https://developer.mozilla.org/en/Firefox_4_for_developers

Thanks! You rock!

Running Firefox 4 RC, the hidden attribute hides elements on a page with XHTML transitional doctype (i.e not HTML5)? Is this correct?

(In reply to comment #32) > Running Firefox 4 RC, the hidden attribute hides elements on a page with XHTML > transitional doctype (i.e not HTML5)? Is this correct? Yes. We're not IE, we implement everything in all modes, unless there's a good reason not to. This is what the HTML specification requires. If you want to use non-standard attributes, please use the data-* attributes; in that case you're sure we'll never do something with them.

(In reply to comment #33) > Yes. We're not IE, we implement everything in all modes, unless there's a good > reason not to. This is what the HTML specification requires. I didn't realise that changes like this impacted on previous html versions. I realised after making my post that if I don't fix this now I'd have to do it if we changed to HTML5 anyway, so have addressed the issue. Thanks for the prompt reply.

> I didn't realise that changes like this impacted on previous html versions. The point is we only implement one version of HTML. We ignore the doctype completely, except for deciding on standards or quirks mode...