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

Description :Ms2ger 2010-05-23 11:27:03 PDT

    
Comment 1 :Ms2ger 2010-05-23 11:32:33 PDT
Created attachment 446977 [details] [diff] [review]
Patch v1
Comment 2 :Ms2ger 2010-06-12 05:00:01 PDT
Created attachment 450840 [details] [diff] [review]
Patch v1.1, merged to tip
Comment 3 :Ms2ger 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] (high review load, please consider other reviewers) 2010-06-21 07:25:10 PDT
Uh, right. Need to check how this affects to performance.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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] (Out June 25-July 6) 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 (review requests must explain patch) 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] (Out June 25-July 6) 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) PTO Until July 5th 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 (review requests must explain patch) 2010-07-13 14:46:14 PDT
... that's instead of the html.css changes.
Comment 18 :Ms2ger 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 (review requests must explain patch) 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 (review requests must explain patch) 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 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] (Out June 25-July 6) 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 (review requests must explain patch) 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 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) PTO Until July 5th 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 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) (Exited; 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 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 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] (Out June 25-July 6) 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.