Closed Bug 335394 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: masayuki)

References

Details

(Keywords: css2)

Attachments

(1 file, 4 obsolete files)

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.
Flags: blocking1.9a1?
O.K. I'll attach the patch.
Assignee: dbaron → masayuki
Attached patch Patch rv1.0 (obsolete) — Splinter Review
removing 'invert'.
Attachment #219885 - Flags: superreview?(dbaron)
Attachment #219885 - Flags: review?(dbaron)
And this patch fixes bug 335474 too.
Blocks: 335474
Status: NEW → ASSIGNED
Attached patch Patch rv1.1 (obsolete) — Splinter Review
Sorry, fix the typo.
Attachment #219885 - Attachment is obsolete: true
Attachment #219886 - Flags: superreview?(dbaron)
Attachment #219886 - Flags: review?(dbaron)
Attachment #219885 - Flags: superreview?(dbaron)
Attachment #219885 - Flags: review?(dbaron)
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).
(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.
Attached patch Patch rv1.2 (obsolete) — Splinter Review
Sorry, previous patch doesn't have layout/generic.
Attachment #219886 - Attachment is obsolete: true
Attachment #219992 - Flags: superreview?(dbaron)
Attachment #219992 - Flags: review?(dbaron)
Attachment #219886 - Flags: superreview?(dbaron)
Attachment #219886 - Flags: review?(dbaron)
> 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).
no, I don't remove on non-cairo build. See nsStyleConsts.h in my patch.
I'm talking about your changes to forms.css, html.css, etc.
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.
Ah, I see.  OK, that works.
Comment on attachment 219992 [details] [diff] [review]
Patch rv1.2

bug 335474 was fixed, this patch cannot apply current trunk.
Attachment #219992 - Attachment is obsolete: true
Attachment #219992 - Flags: superreview?(dbaron)
Attachment #219992 - Flags: review?(dbaron)
No longer blocks: 335474
Flags: blocking1.9a1? → blocking1.9a1+
Attached patch Patch rv1.3 (obsolete) — Splinter Review
Sorry for the delay.
Attachment #223775 - Flags: superreview?(dbaron)
Attachment #223775 - Flags: review?(dbaron)
Flags: blocking1.9+
We should get a bug filed on making cairo able to invert.
Flags: blocking1.9a1+
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.
Attachment #223775 - Flags: superreview?(dbaron)
Attachment #223775 - Flags: superreview+
Attachment #223775 - Flags: review?(dbaron)
Attachment #223775 - Flags: review+
Attached patch Patch rv1.4Splinter Review
Updating to latest trunk.
Attachment #223775 - Attachment is obsolete: true
Attachment #258670 - Flags: superreview+
Attachment #258670 - Flags: review+
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.
(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.
thank you, dbaron.

checked in the patch.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
See Also: → 586863
You need to log in before you can comment on or make changes to this bug.