Bug 174149 – CSSFontStyleValue::isItalic seems a bit bogus.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Emilio Cobos Álvarez]

bool isItalic() const
    {
        if (!obliqueValue) {
            auto valueID = fontStyleValue->valueID();
            return valueID == CSSValueItalic || CSSValueOblique;
        }
        return obliqueValue->value<float>(CSSPrimitiveValue::CSS_DEG) >= static_cast<float>(italicValue());
    }

Note how "|| CSSValueOblique" makes the first return always return true.

I'm not familiar with this code, so I'm not sure how to best test it.

Happy to fix it with indications on how to test it. I've CCd the original patch author and reviewer so you can take a look (and fix) if you want to.

[Myles C. Maxfield]

Good catch!

[Myles C. Maxfield]

Created attachment 314662 [details]
Patch

[Tim Horton]

Comment on attachment 314662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314662&action=review

> Source/WebCore/ChangeLog:12
> +        * css/CSSFontStyleValue.h:

Write some words!

> Source/WebCore/css/CSSFontStyleValue.h:50
>      {

Should this be called isItalicOrOblique?

> LayoutTests/editing/execCommand/italicizeByCharacter-normal.html:11
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

This is the ugliest <script> I've seen in a long time :)

[WebKit Commit Bot]

Comment on attachment 314662 [details]
Patch

Clearing flags on attachment: 314662

Committed r219173: <http://trac.webkit.org/changeset/219173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

(In reply to Tim Horton from comment #3)
> Comment on attachment 314662 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314662&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        * css/CSSFontStyleValue.h:
> 
> Write some words!
> 
> > Source/WebCore/css/CSSFontStyleValue.h:50
> >      {
> 
> Should this be called isItalicOrOblique?
> 
> > LayoutTests/editing/execCommand/italicizeByCharacter-normal.html:11
> > +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> 
> This is the ugliest <script> I've seen in a long time :)

Oh no, I didn't actually do anything you said!!! :(
I'll do it in a follow-up patch.

[Myles C. Maxfield]

Committed r220531: <http://trac.webkit.org/changeset/220531>

[Radar WebKit Bug Importer]

<rdar://problem/33829710>

[Simon Fraser (smfr)]

Comment on attachment 314662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314662&action=review

> Source/WebCore/ChangeLog:3
> +        CSSFontStyleValue::isItalic seems a bit bogus.

Vaguest bug title ever.

[Antti Koivisto]

> > +        CSSFontStyleValue::isItalic seems a bit bogus.
> 
> Vaguest bug title ever.

"CSSFontStyleValue::isItalic has high bogosity."

[Brent Fulgham]

*** Bug 173970 has been marked as a duplicate of this bug. ***