Last Comment Bug 567663 - Implement the hidden attribute
: Implement the hidden attribute
Status: RESOLVED FIXED
[parity-webkit]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b3
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
http://www.whatwg.org/html/#the-hidde...
Depends on: 585101 613722
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-23 11:27 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-03-09 09:41 PST (History)
14 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (13.17 KB, patch)
2010-05-23 11:32 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v1.1, merged to tip (13.27 KB, patch)
2010-06-12 05:00 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v1.2, merged to tip (13.18 KB, patch)
2010-06-17 01:39 PDT, :Ms2ger (⌚ UTC+1/+2)
jonas: review-
Details | Diff | Splinter Review
Mapped attribute WIP (1.96 KB, patch)
2010-07-17 08:41 PDT, :Ms2ger (⌚ UTC+1/+2)
dbaron: feedback+
Details | Diff | Splinter Review
Patch v2 (13.83 KB, patch)
2010-07-18 06:50 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2.1 (14.84 KB, patch)
2010-07-19 11:40 PDT, :Ms2ger (⌚ UTC+1/+2)
jonas: review+
Details | Diff | Splinter Review
Patch v2.2 for checkin (15.86 KB, patch)
2010-07-20 03:28 PDT, :Ms2ger (⌚ UTC+1/+2)
jonas: approval2.0+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2010-05-23 11:27:03 PDT

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2010-05-23 11:32:33 PDT
Created attachment 446977 [details] [diff] [review]
Patch v1
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2010-06-12 05:00:01 PDT
Created attachment 450840 [details] [diff] [review]
Patch v1.1, merged to tip
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2010-06-17 01:39:35 PDT
Created attachment 451887 [details] [diff] [review]
Patch v1.2, merged to tip
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2010-06-21 07:05:47 PDT
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] (reviewing overload) 2010-06-21 07:25:10 PDT
Uh, right. Need to check how this affects to performance.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2010-06-21 09:50:13 PDT
> 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 2010-06-23 12:24:07 PDT
What do we do when we check for matches to the :lang() pseudo-class, for example?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2010-06-23 12:36:20 PDT
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 2010-06-23 13:38:48 PDT
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 [:bz] (still a bit busy) 2010-06-23 13:41:16 PDT
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 [:bz] (still a bit busy) 2010-06-23 13:42:01 PDT
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 13 Boris Zbarsky [:bz] (still a bit busy) 2010-06-23 16:22:21 PDT
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-7 (busy September 14-25) 2010-06-23 16:34:31 PDT
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 [:bz] (still a bit busy) 2010-06-23 16:52:53 PDT
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 2010-07-13 14:32:44 PDT
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-7 (busy September 14-25) 2010-07-13 14:46:14 PDT
... that's instead of the html.css changes.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2010-07-17 08:41:48 PDT
Created attachment 458096 [details] [diff] [review]
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-7 (busy September 14-25) 2010-07-17 09:07:17 PDT
You don't want mDisplay.SetNoneValue(); instead you want mDisplay.SetIntValue(NS_STYLE_DISPLAY_NONE, eCSSUnit_Enumerated).
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-07-17 09:08:16 PDT
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 (⌚ UTC+1/+2) 2010-07-18 06:50:41 PDT
Created attachment 458169 [details] [diff] [review]
Patch v2

Thanks, that did make it work.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2010-07-18 22:36:02 PDT
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-7 (busy September 14-25) 2010-07-18 22:50:57 PDT
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 (⌚ UTC+1/+2) 2010-07-19 11:40:21 PDT
Created attachment 458397 [details] [diff] [review]
Patch v2.1

Thanks, it looks like that works.
Comment 25 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-19 11:47:57 PDT
Presumably comment 22 should be added as a reftest too...
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-19 16:28:57 PDT
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 (⌚ UTC+1/+2) 2010-07-20 03:28:17 PDT
Created attachment 458608 [details] [diff] [review]
Patch v2.2 for checkin

Done.
Comment 28 Dão Gottwald [:dao] 2010-07-21 00:26:12 PDT
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) 2010-08-01 23:34:24 PDT
http://hg.mozilla.org/mozilla-central/rev/b1d534754b62

Thanks!
Comment 30 Jean-Yves Perrier [:teoli] 2010-08-02 04:40:21 PDT
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 (⌚ UTC+1/+2) 2010-08-02 08:18:57 PDT
Thanks! You rock!
Comment 32 Alex Stansfield 2011-03-09 01:55:13 PST
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 (⌚ UTC+1/+2) 2011-03-09 02:26:46 PST
(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 2011-03-09 05:18:35 PST
(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 [:bz] (still a bit busy) 2011-03-09 09:41:41 PST
> 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...

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