567663 - Implement the hidden attribute

Status: RESOLVED FIXED

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

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.1, merged to tip

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.2, merged to tip

Comment 4

[Jeff Walden [:Waldo]]

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?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[d]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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....

Comment 9

[d]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[d]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Mapped attribute WIP

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Thanks, that did make it work.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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);
      }
    }
  }

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2.1

Thanks, it looks like that works.

Comment 25

[Jeff Walden [:Waldo]]

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

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 458397 [details] [diff] [review]
Patch v2.1

r=me if you also add the testcase in comment 22 as a reftest.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2.2 for checkin

Done.

Comment 28

[Dão Gottwald [:dao]]

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

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Thanks!

Comment 30

[Jean-Yves Perrier [:teoli]]

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

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks! You rock!

Comment 32

[Alex Stansfield]

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

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 34

[Alex Stansfield]

(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.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> 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...