896503 - Rule view shows doesn't distinguish an invalid rgb color in a text-shadow rule

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[pqwoerituytrueiwoq]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130722 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130722030226

Steps to reproduce:

64bit Firefox Nightly for linux: 25.0a1 (2013-07-22)

Right Click -> Inspect Element (one with a text-shadow)


Actual results:

Text shadow was reported as:
text-shadow: 0px 0px 1px, 0px 0px 2px, 0px 0px 3px, 0px 0px 4px;


Expected results:

Text shadow really is:
text-shadow: 0px 0px 1px rgb(125,125,125), 0px 0px 2px rgb(125,125,125, 0px 0px 3px, 0px 0px 4px rgb(125,125,125);

Comment 1

[pqwoerituytrueiwoq]

* Note there was not a typo on the 3ed value under really is

Comment 2

[pqwoerituytrueiwoq]

not a 100% reproducible it seems, the style was in effect and working

Comment 3

[pqwoerituytrueiwoq]

Ok, floating point values in rgb break the dev tool
rgb(127.5,127.5,127.5)

Comment 4

[sjw]

Floating point values are not allowed in rgb(), except the alpha value in rgba().

Comment 5

[Girish Sharma [:Optimizer]]

These values actually work, so we should show them too. Although, I believe that its the rule view that is being referenced here, not the style editor.

Comment 6

[Brian Grinstead [:bgrins]]

Basic test case showing the reported issue: http://fiddle.jshell.net/bgrins/mYMKB/show/.  pbrosset was working on a patch that allows authored styles in the rule view, which would likely solve this problem - I can't find a bug # off hand though.  Patrick, is there a bug opened for this?

Comment 7

[Patrick Brosset <:pbro>]

Indeed as bgrins noted, this is a problem of our rule-view not showing authored styles, but rather actually applied styles.
I haven't yet started working on my authored-style support demo. I created bug 984880 for this.

Comment 8

[Girish Sharma [:Optimizer]]

Wha tI am wondering is that the invalid rgb values actually do apply, I see blue color if I do something like rgb(1.1, 1.2, 20) . So rule view should show at least some values !

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> Wha tI am wondering is that the invalid rgb values actually do apply, I see
> blue color if I do something like rgb(1.1, 1.2, 20) . So rule view should
> show at least some values !
Really? I my tests, if a RGB color has floating point values, it just doesn't get applied.
So, I think the rule-view works "as expected" considering it doesn't deal with authored styles as this stage.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> Wha tI am wondering is that the invalid rgb values actually do apply, I see
> blue color if I do something like rgb(1.1, 1.2, 20) . So rule view should
> show at least some values !

RGB colors by spec are integers or percentages: http://www.w3.org/TR/css3-color/#rgb-color.  "The format of an RGB value in the functional notation is ‘rgb(’ followed by a comma-separated list of three numerical values (either three integer values or three percentage values) followed by ‘)’"

What do you see if you do window.getComputedStyle for text-shadow on this element?

Comment 11

[Girish Sharma [:Optimizer]]

See the screenshot, I added decimal values, they did not appear, but got applied : http://i.snag.gy/wF9nX.jpg

Comment 12

[Patrick Brosset <:pbro>]

I think the shadow got applied, but not the color. Try choosing a color that's easy to recognize and using the offset to see it better. Something like:

10px 10px 0px rgb(1.1, 1.1, 200);

you will see that the shadow has the default color, and is not blue.
But this:

10px 10px 0px rgb(1, 1, 200);

will produce a blue shadow.

Comment 13

[pqwoerituytrueiwoq]

on Brians's jsfiddle, window.getComputedStyle(document.getElementById('cat'),null).getPropertyValue('text-shadow') returned:
rgb(0, 0, 0) 0px 0px 1px

**The ID of cat was added via firebug

Comment 14

[pqwoerituytrueiwoq]

I just confired what Patrick thought, the decimal values make it fall back to 0,0,0
this was on firefox 27.0.1

Comment 15

[Girish Sharma [:Optimizer]]

So we should've at least shown that in the rule view as per the current logic of getting the computed values from gecko. :)

Comment 16

[Tom Tromey :tromey]

With the as-authored series applied, the invalid value does
show up in the rule view.  E.g., for the link in comment #6
I see:

text-shadow: 0px 0px 1px rgb(127.5,127.5,127.5);

It is a bit misleading here because the rgb() is not applied.
There is one subtle difference here, though, which is that the
invalid rgb doesn't have a color swatch -- but I wouldn't expect
most users to notice this.

It's also clear that the color isn't applied by consulting the
computed view.

One way we could perhaps do better would be to change output-parser
to strike-through invalid colors (not names of course, but rgb,
hsl, or hex).

Comment 17

[Tom Tromey :tromey]

Attachment: Screenshot from 2015-10-08 15-18-09.png

Here's what the line-through idea looks like.

Comment 18

[Tom Tromey :tromey]

Attachment: strike through text that looks like a color but is not quite valid

Patch I used to make that screenshot.

Comment 19

[Tom Tromey :tromey]

Brian, do you think this approach is worth pursuing?

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Tom Tromey :tromey from comment #19)
> Brian, do you think this approach is worth pursuing?

Yeah, that looks great

Comment 21

[Tom Tromey :tromey]

Attachment: strike through text that looks like a color but is not quite valid

Comment 22

[Tom Tromey :tromey]

Attachment: strike through text that looks like a color but is not quite valid

Now with debugging removed.

Comment 23

[Tom Tromey :tromey]

Comment on attachment 8672012 [details] [diff] [review]
strike through text that looks like a color but is not quite valid

Here's the patch plus tests.

I considered having this look for invalid hex colors as well but I didn't think that would be as useful.

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8672012 [details] [diff] [review]
strike through text that looks like a color but is not quite valid

Review of attachment 8672012 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me

Comment 25

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7af6b3b1121

Comment 26

[Tom Tromey :tromey]

Something has changed here since I wrote the patch, because now
DOMUtils.cssPropertyIsValid returns false for the text-shadow in
the example.  The property isn't applied and appears with a line
through it (and a warning sign) in the rule view.

I don't know of another way to reproduce this problem, but I'm looking.

Comment 27

[Tom Tromey :tromey]

I think this was fixed by bug 1199610 and that we don't actually need this patch.