Implement the hidden attribute

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

({dev-doc-complete, html5})

Trunk
mozilla2.0b3
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-webkit], URL)

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Created attachment 446977 [details] [diff] [review]
Patch v1
Attachment #446977 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #446977 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Whiteboard: [parity-webkit]
(Assignee)

Comment 2

7 years ago
Created attachment 450840 [details] [diff] [review]
Patch v1.1, merged to tip
Attachment #446977 - Attachment is obsolete: true
Attachment #450840 - Flags: review?(Olli.Pettay)
Attachment #446977 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 3

7 years ago
Created attachment 451887 [details] [diff] [review]
Patch v1.2, merged to tip
Attachment #450840 - Attachment is obsolete: true
Attachment #451887 - Flags: review?(Olli.Pettay)
Attachment #450840 - Flags: review?(Olli.Pettay)
Attachment #451887 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #451887 - Flags: review+

Comment 6

7 years ago
> 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

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

Comment 8

7 years ago
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

7 years ago
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

7 years ago
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

7 years ago
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

7 years ago
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

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

Comment 15

7 years ago
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.
Attachment #451887 - Flags: review-
... that's instead of the html.css changes.
(Assignee)

Comment 18

7 years ago
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?
Attachment #458096 - Flags: feedback?(dbaron)
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)
Attachment #458096 - Flags: feedback?(dbaron) → feedback+
(Assignee)

Comment 21

7 years ago
Created attachment 458169 [details] [diff] [review]
Patch v2

Thanks, that did make it work.
Attachment #451887 - Attachment is obsolete: true
Attachment #458096 - Attachment is obsolete: true
Attachment #458169 - Flags: review?(jonas)

Comment 22

7 years ago
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);
      }
    }
  }
(Assignee)

Updated

7 years ago
Attachment #458169 - Flags: review?(jonas)
(Assignee)

Comment 24

7 years ago
Created attachment 458397 [details] [diff] [review]
Patch v2.1

Thanks, it looks like that works.
Attachment #458169 - Attachment is obsolete: true
Attachment #458397 - Flags: review?(jonas)
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.
Attachment #458397 - Flags: review?(jonas) → review+
(Assignee)

Comment 27

7 years ago
Created attachment 458608 [details] [diff] [review]
Patch v2.2 for checkin

Done.
Attachment #458397 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed, dev-doc-needed
You need to request approval2.0 on your patch before it can land.
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Attachment #458608 - Flags: approval2.0?
Attachment #458608 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b1d534754b62

Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
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
(Assignee)

Comment 31

7 years ago
Thanks! You rock!
Keywords: dev-doc-needed → dev-doc-complete
Target Milestone: --- → mozilla2.0b3
Depends on: 585101
Depends on: 613722

Comment 32

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

Comment 33

7 years ago
(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

7 years ago
(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

7 years ago
> 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...
You need to log in before you can comment on or make changes to this bug.