Ticket Navigation

Ticket #4854 (closed enhancement: wontfix)

Opened 2 years ago

Last modified 2 years ago

[PATCH] Element.visible() returns correct value when display is set in a stylesheet

Reported by:	acc_ruby@riggers.me.uk	Assigned to:	sam
Priority:	low	Milestone:	1.2
Component:	Prototype	Version:
Severity:	major	Keywords:	prototype element visible
Cc:	mislav

Description

Element.visible() returns true, even though the element has display:none defined in css.

<html>
	<head>
		<script type="text/javascript" src="prototype.js"></script>
		<style type="text/css">
			#x { display:none; }
		</style>
	</head>
	<body>
		<div id='x'>
			hello
		</div>
		<script type="text/javascript">
			alert($('x').visible());
		</script>
	</body>
</html>

Alert box displays 'true' - should be 'false'.

Following patch fixes the problem, by using getStyle() in visible()

Index: prototype/src/dom.js
===================================================================
--- prototype/src/dom.js        (revision 4254)
+++ prototype/src/dom.js        (working copy)
@@ -50,7 +50,7 @@
 
 Element.Methods = {
   visible: function(element) {
-    return $(element).style.display != 'none';
+    return $(element).getStyle('display') != 'none';
   },
   
   toggle: function() {

Attachments

visible.diff (2.2 kB) - added by Tobie on 12/04/06 21:45:41.: js + test + benchmarks

Change History

12/04/06 21:43:49 changed by Tobie

status changed from closed to reopened.
type changed from defect to enhancement.
summary changed from [PATCH] Element.visible() returns true if display:none to [PATCH] Element.visible() returns true if display:none is set in a stylesheet.
resolution deleted.
milestone set to 1.2.

There are two problems with the solution above:

getComputedStyle() returns null on any style in Safari if the element is hidden... see Jonathan Snook's post on this issue.
it does not account for elements contained inside hidden elements.

I've attached a patch (and tests) that corrects both issues. However... the overhead added is HUGE ! The method is now about 20x slower in Firefox! (you can see the benchmarks for yourself in the added tests).

I'm wondering if this shouldn't be closed as won't fix... or proposed as another method.

Note: I've tried using a standard for loop to see if that reduced computation time, but it didn't.

(in reply to: ↑ 5 ) 12/05/06 09:19:29 changed by Tobie

Don't underestimate the severity of this. I've been bitten many times I wanted to use visible, hide, show etc. on elements hidden by CSS

I'm not. I just think that visible() has been implemented for use in methods like toggle() which apply inline style.

I don't like the name of it. It doesn't make any sense. This method should be called displayed() or hidden() or something similar.

That's why I suggested implementing a new method which would check if any CSS was applied to it or any of it's parents.

The speed problem is significant - we have to make sure getStyle gets called only once (the first time) after which the original style is saved in a hidden property.

I don't see how you want to do that. Changing the display of any of the element's parents would make your hidden property outdated.

As for Safari, maybe we can turn this "feature" against it - maybe we can actually use the null returned to detect whether an element is hidden.

That's what I did (see the patch). The method is thus much faster in Safari (no looping through ancestors needed).

(follow-up: ↓ 9 ) 12/05/06 13:29:39 changed by mislav

I don't think we should worry by default about parent(s) being hidden. I would wrap the ancestor iterating in a conditional block that happens only if the optional argument (say, checkParents) is set to true. That would partially solve the problem of your patch making this (very important!) method too expensive.

There is another perspective to the computed style problem: accessibility. We're trying to detect if an element has been hidden with CSS stylesheets so we can show/hide it with JavaScript, right? Well, we shouldn't encourage that. My rule is: if you're gonna show it with JavaScript, better hide it with JavaScript, too. If a user agent doesn't support JavaScript / has it disabled, the hidden content will never be made visible or read aloud by most screen readers. Knowing that, is it right to make this obtrusive practice possible in the framework by providing support for it?

(in reply to: ↑ 7 ) 12/05/06 21:56:25 changed by Tobie

Replying to mislav:

I don't think we should worry by default about parent(s) being hidden. I would wrap the ancestor iterating in a conditional block that happens only if the optional argument (say, checkParents) is set to true. That would partially solve the problem of your patch making this (very important!) method too expensive.

Good idea.

There is another perspective to the computed style problem: accessibility. We're trying to detect if an element has been hidden with CSS stylesheets so we can show/hide it with JavaScript, right? Well, we shouldn't encourage that. My rule is: if you're gonna show it with JavaScript, better hide it with JavaScript, too. If a user agent doesn't support JavaScript / has it disabled, the hidden content will never be made visible or read aloud by most screen readers. Knowing that, is it right to make this obtrusive practice possible in the framework by providing support for it?

I get your point. However, consider the following for example: drag and drop support has been added to a web page (for whatever reason). The developper, concerned with the website users not necessarily being web 2.0 literate, has added a notice on the page explaining this new behavior. He obviously wants to have it hidden in case the browser does not support JavaScript, the only thing he can use for that is CSS.

Now that I think about it: You are right with your comment saying that the two patches are related.

If we fixed Element.visible(), and did not fix showing and hidding, we'd be in a really ugly situation where one could hide an element using CSS in an external stylesheet, but showing it using Element.show() would not work.

Unfortnately, there seems to be no solution to this issue. As the original style we would need to get is not the inline style, as proposed by the patch you mention, but the default style the element has when no styling is applied to it (e.g.: 'block', 'inline', etc.). If we set the element's display to 'none' in the stylesheet, there is no way we can access that property. Except by having a constant mapping each tag to it's default display value (which is probably browser dependent). And finding out the orginal display by using the tagName property.

There is another issue here again: it is very possible that a div (for example) has two classes applied to it, the first one setting it's display to 'inline', and the second one to 'none'. In which case, mapping the element's tagName to the expected default display would break the layout.

Honestly, I really don't see how this can be solved. I suggest leaving the visible method as it is and closing both patches as won't fix.

Download in other formats:

Rails Trac

Ticket Navigation

Ticket #4854 (closed enhancement: wontfix)

[PATCH] Element.visible() returns correct value when display is set in a stylesheet

Description

Attachments

Change History

09/03/06 23:04:09 changed by madrobby

12/04/06 21:43:49 changed by Tobie

12/04/06 21:44:45 changed by Tobie

12/04/06 21:45:41 changed by Tobie

12/04/06 22:12:46 changed by Tobie

(follow-up: ↓ 6 ) 12/05/06 09:01:41 changed by mislav

(in reply to: ↑ 5 ) 12/05/06 09:19:29 changed by Tobie

(follow-up: ↓ 9 ) 12/05/06 13:29:39 changed by mislav

12/05/06 19:12:55 changed by mislav

(in reply to: ↑ 7 ) 12/05/06 21:56:25 changed by Tobie

12/05/06 21:57:57 changed by Tobie

12/10/06 00:32:02 changed by Tobie

12/31/06 20:22:43 changed by Tobie

Download in other formats: