Closed Bug 319381 Opened 14 years ago Closed 13 years ago

[FIX]getComputedStyle/getPropertyValue returns wrong overflow values

Categories

(Core :: CSS Parsing and Computation, defect, P3, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: aclark, Assigned: bzbarsky)

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

When an element's overflow style property is set to "-moz-scrollbars-vertical" or "-moz-scrollbars-horizontal", the computed overflow value returns "0px".

For example:

  var div = document.createElement("DIV");
  div.style.overflow = "-moz-scrollbars-vertical";

  var css = window.getComputedStyle(div,"");
  alert(css.getPropertyValue("overflow"));

To work around this problem, application code must identify the browser as Firefox 1.5+ and if the overflow value is returned as "0px", then query the "overflow-x" value instead. If the overflow-x value is "hidden", then the overflow value should be "-moz-scrollbars-vertical", else it should be "-moz-scrollbars-horizontal".

Reproducible: Always

Steps to Reproduce:
1. Document contains element with overflow style property set to "-moz-scrollbars-vertical" or "-moz-scrollbars-horizontal" either using style attribute or CSS selector.
2. Call window.getComputedStyle on element.
3. Call getPropertyValue("overflow") on cssDecl object.

Actual Results:  
Calling getPropertyValue("overflow") returns "0px".

Expected Results:  
Expect getPropertyValue("overflow") to return "-moz-scrollbars-vertical" and "-moz-scrollbars-horizontal", respectively.
The sample generates a matrix of overflow values and the computed values for "overflow", "overflow-x", and "overflow-y" in each case.
->Core:Style
Assignee: nobody → dbaron
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → Trunk
Attached patch Patch (obsolete) — Splinter Review
Assignee: dbaron → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Attachment #232663 - Flags: superreview?(dbaron)
Attachment #232663 - Flags: review?(dbaron)
Andy, thanks for the excellent testcase!
Flags: in-testsuite?
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Summary: getComputedStyle/getPropertyValue returns wrong overflow values → [FIX]getComputedStyle/getPropertyValue returns wrong overflow values
Target Milestone: --- → mozilla1.9alpha
To be honest, I think I prefer it the way it was.

Calling getComputedStyle and asking for the value of a shorthand property isn't always supposed to work.  (Although I'm skeptical that 0px is the right thing to do in that case; there are potentially a lot of cases where it could happen as properties become shorthands in later levels of CSS.)

I'd rather treat these values as the honest soon-to-be-css3-we-hope shorthand than these funky -moz- values that never should have existed.

I think we need to figure out a compatibility story for the CSS2 properties that are becoming shorthands in higher levels of CSS.  And I think whatever the answer to that general question is should be the fix for this bug -- not reuse of -moz-* values that will fix only this one case and not the general problem.
Our current general solution for shorthands is to simply not support them (since we implement getPropertyValue in terms of getPropertyCSSValue, and the latter is supposed to return nothing for shorthands)....   That really seems suboptimal to me too, for what it's worth.

All that aside, what possible value _could_ we report here?  The corresponding CSS3 value (that we don't actually support yet)?
In many cases there is no possible shorthand value to return.  For example, given:

  border-top: thick dashed blue;
  border-right: medium dotted green;
  border-bottom: none;
  border-left: thin solid red;

there's no possible computed value for the shorthand "border" property.  That's also the case for the "font" shorthand if "font-size-adjust" or "font-stretch" is specified.  CSS3 drafts also introduce this problem for existing non-shorthand properties "white-space" (for good reason) and "vertical-align" (where I hope it will go away), "display" (if we ever sort out the problems with the split) and probably a bunch of others I'm forgetting.
(In reply to comment #6)
> The corresponding
> CSS3 value (that we don't actually support yet)?

comment 7 was an attempt to answer this question -- or rather, point out that such value does not now and need not ever exist.
Attachment #232663 - Attachment is obsolete: true
Attachment #232755 - Flags: superreview?(dbaron)
Attachment #232755 - Flags: review?(dbaron)
Attachment #232663 - Flags: superreview?(dbaron)
Attachment #232663 - Flags: review?(dbaron)
Comment on attachment 232755 [details] [diff] [review]
Just output the empty string

So what I don't like about this is that it looks like the only place where we return a *null* value rather than an uninitialized one.  Do you think that's really what we want to do here?
Looking at GetPropertyCSSValue(), we return null for shorthands in general, no?
Comment on attachment 232755 [details] [diff] [review]
Just output the empty string

ah, ok

r+sr=dbaron
Attachment #232755 - Flags: superreview?(dbaron)
Attachment #232755 - Flags: superreview+
Attachment #232755 - Flags: review?(dbaron)
Attachment #232755 - Flags: review+
Fix checked in; still need to write a regression test.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
FWIW, if there's more issues like this with the "CSSOM" feel free to raise them on www-style@w3.org. http://dev.w3.org/cvsweb/csswg/cssom/ has my ongoing draft to sort out the "CSSOM" and related functionality.
Checked in mochitest testcase.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.