335394 - We should remove support for the 'invert' value ifdef MOZ_CAIRO_GFX

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The cairo landing and bug 326550 broke support for 'outline-color: invert'.  Given that we don't support it correctly, we should remove support for the keyword -- and most of the patch to bug 326550 should be backed out.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

O.K. I'll attach the patch.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0

removing 'invert'.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

And this patch fixes bug 335474 too.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1

Sorry, fix the typo.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Shouldn't css continue to use "invert" when it can (i.e. #ifdef ENABLE_INVERT)?  Otherwise we're regressing accessibility in non-cairo builds too, not just cairo ones (see bug 326550 comment 19).

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to comment #5)
> Shouldn't css continue to use "invert" when it can (i.e. #ifdef ENABLE_INVERT)?
>  Otherwise we're regressing accessibility in non-cairo builds too, not just
> cairo ones (see bug 326550 comment 19).

non-cairo build continues to support 'invert' value.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.2

Sorry, previous patch doesn't have layout/generic.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> non-cairo build continues to support 'invert' value.

Yes, but you're removing "invert" from all our CSS, so you're changing the behavior of non-cairo builds (imo in a bad way).

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

no, I don't remove on non-cairo build. See nsStyleConsts.h in my patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm talking about your changes to forms.css, html.css, etc.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

the initial value of outline-color is 'invert'.
http://www.w3.org/TR/CSS21/ui.html#propdef-outline-color
I think that there are no problem.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  OK, that works.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 219992 [details] [diff] [review]
Patch rv1.2

bug 335474 was fixed, this patch cannot apply current trunk.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.3

Sorry for the delay.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should get a bug filed on making cairo able to invert.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 223775 [details] [diff] [review]
Patch rv1.3

(Set|Get)OutlineSpecialColor should be called 
(Set|Get)OutlineInitialColor.  ("Special" doesn't provide any useful
information.)  And in both methods the (void) should just be () (since
they're the same in C++, unlike C where () is more permissive than
(void)).

The OUTLINE_COLOR_SPECIAL stuff really doesn't make sense to me --
BORDER_COLOR_SPECIAL is a mask covering all the bits used to represent
special things.  Why not just define OUTLINE_COLOR_INITIAL to one of the 
constants?

Could you rename ENABLE_INVERT to GFX_HAS_INVERT or GFX_CAN_INVERT?

With those changes, r+sr=dbaron.

Sorry for the long delay in getting to this.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.4

Updating to latest trunk.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

there are two problems:

1. the test suite checks the invert.
http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline.css
http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-width.css
http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-style.css
http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-color.css

we need to change the tests, but I don't know which test is better. (Should I only removes the 'invert'?)

dbaron: what should I do before checking-in?

2. the findbar.xml has CRLF?
When I diffed findbar.xml, some lines are detected as they were changed.

Asaf Romano: You should know this problem, I'll fix it by this checking-in.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> 1. the test suite checks the invert.
> http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline.css
> http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-width.css
> http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-style.css
> http://lxr.mozilla.org/mozilla/source/layout/html/tests/css/outline-color.css
> 
> we need to change the tests, but I don't know which test is better. (Should I
> only removes the 'invert'?)
> 
> dbaron: what should I do before checking-in?

Nothing.  We don't use those tests much anymore.  And this loss of a feature really is a regression, and hopefully it's not permanent.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

thank you, dbaron.

checked in the patch.