CSS warnings: "Error in parsing value for 'height'/'width'. Declaration dropped."

RESOLVED FIXED in Firefox 4.0b11

Status

defect
P4
normal
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: mitcho, Assigned: ttaubert)

Tracking

unspecified
Firefox 4.0b11
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-][good first bug][cleanup])

Attachments

(1 attachment, 2 obsolete attachments)

From time to time in Panorama, we get CSS warnings like this:

> Warning: Error in parsing value for 'height'.  Declaration dropped.
> Source File: chrome://browser/content/tabview.html
> Line: 0

> Warning: Error in parsing value for 'width'.  Declaration dropped.
> Source File: chrome://browser/content/tabview.html
> Line: 0

We need to clean these up. They're messy and unprofessional, and may actually
indicate underlying problems.

See also: bug 622285
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Posted patch patch v1 (obsolete) — Splinter Review
Just in case to save some time:

-      height: orig.height * zoomWidth / orig.width
+      height: orig.height * zoomWidth / orig.width || 0
--> sometimes orig.width == 0, so divide by zero and get NaN, fallback to 0

-  background-image-opacity: .1;
--> this property does not exist

-  -moz-padding-end: -20px;
--> this property does not accept negative values
Attachment #503985 - Flags: review?(ian)
Comment on attachment 503985 [details] [diff] [review]
patch v1

>-      height: orig.height * zoomWidth / orig.width
>+      height: orig.height * zoomWidth / orig.width || 0

Might be worth a comment.

>diff --git a/browser/themes/gnomestripe/browser/tabview/tabview.css b/browser/themes/gnomestripe/browser/tabview/tabview.css
>--- a/browser/themes/gnomestripe/browser/tabview/tabview.css
>+++ b/browser/themes/gnomestripe/browser/tabview/tabview.css
>@@ -429,10 +429,8 @@
>   font-style: italic !important;
>   color: transparent;
>   background-image: url(chrome://browser/skin/tabview/edit-light.png);
>-  background-image-opacity: .1;
>   background-repeat: no-repeat;
>   -moz-padding-start: 20px;
>-  -moz-padding-end: -20px;
> }
> 
> .title-container:hover input.name:-moz-placeholder {
>diff --git a/browser/themes/pinstripe/browser/tabview/tabview.css b/browser/themes/pinstripe/browser/tabview/tabview.css
>--- a/browser/themes/pinstripe/browser/tabview/tabview.css
>+++ b/browser/themes/pinstripe/browser/tabview/tabview.css
>@@ -421,7 +421,6 @@
>   font-style: italic !important;
>   color: transparent;	
>   background-image: url(chrome://browser/skin/tabview/edit-light.png);
>-  background-image-opacity: .1;
>   background-repeat: no-repeat;
>   -moz-padding-start: 20px;
>   -moz-margin-end: -20px;
>diff --git a/browser/themes/winstripe/browser/tabview/tabview.css b/browser/themes/winstripe/browser/tabview/tabview.css
>--- a/browser/themes/winstripe/browser/tabview/tabview.css
>+++ b/browser/themes/winstripe/browser/tabview/tabview.css
>@@ -448,10 +448,8 @@
>   font-style: italic !important;
>   color: transparent;
>   background-image: url(chrome://browser/skin/tabview/edit-light.png);
>-  background-image-opacity: .1;
>   background-repeat: no-repeat;
>   -moz-padding-start: 20px;
>-  -moz-padding-end: -20px;
> }

Good clean up, and doesn't seem to have any negative impact.

I wonder if the -moz-margin-end in pinstripe... maybe we should get rid of that as well? Or convert the -moz-padding-ends to margin?
Attachment #503985 - Flags: review?(ian) → review-
(In reply to comment #2)
> Comment on attachment 503985 [details] [diff] [review]
> patch v1
> 
> >-      height: orig.height * zoomWidth / orig.width
> >+      height: orig.height * zoomWidth / orig.width || 0
> 
> Might be worth a comment.

Or write it so that it's self-explanatory.
E.g. height: (orig.width ? orig.height * zoomWidth / orig.width : 0)
(In reply to comment #3)
> Or write it so that it's self-explanatory.
> E.g. height: (orig.width ? orig.height * zoomWidth / orig.width : 0)

Looks good, I'll take that.

(In reply to comment #2)
> I wonder if the -moz-margin-end in pinstripe... maybe we should get rid of that
> as well? Or convert the -moz-padding-ends to margin?

Well, negative margin values are allowed and _could_ make sense. I don't know if they're really needed for the layout because gnomestripe doesn't use them :/
Comment on attachment 503985 [details] [diff] [review]
patch v1

(In reply to comment #4)
> (In reply to comment #3)
> > Or write it so that it's self-explanatory.
> > E.g. height: (orig.width ? orig.height * zoomWidth / orig.width : 0)
> 
> Looks good, I'll take that.
> 
> (In reply to comment #2)
> > I wonder if the -moz-margin-end in pinstripe... maybe we should get rid of that
> > as well? Or convert the -moz-padding-ends to margin?
> 
> Well, negative margin values are allowed and _could_ make sense. I don't know
> if they're really needed for the layout because gnomestripe doesn't use them :/

I've just tried it on my mac; doesn't seem to make any difference. Please kill it.

R+ with those two changes.
Attachment #503985 - Flags: review- → review+
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #503985 - Attachment is obsolete: true
Attachment #505208 - Flags: approval2.0?
Blocks: 627096
Priority: -- → P4
Blocks: 608502
Comment on attachment 505208 [details] [diff] [review]
patch v2

a=beltzner
Attachment #505208 - Flags: approval2.0? → approval2.0+
Attachment #505208 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/f68e1a3a3705
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out: http://hg.mozilla.org/mozilla-central/rev/84431e829386

This one was backed out together with bug 625424 but surely is not the cause for tests failing. So please feel free to push again :)
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Comment 11

8 years ago
http://hg.mozilla.org/mozilla-central/rev/bb6aa94958c4
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
See Also: → 837136
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.