Closed Bug 335394 Opened 14 years ago Closed 13 years ago
We should remove support for the 'invert' value ifdef MOZ
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.
O.K. I'll attach the patch.
Assignee: dbaron → masayuki
Sorry, fix the typo.
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.
Sorry, previous patch doesn't have layout/generic.
> 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.
No longer blocks: 335474
Sorry for the delay.
We should get a bug filed on making cairo able to invert.
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.
Updating to latest trunk.
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
You need to log in before you can comment on or make changes to this bug.