73884 - Need to support color 'invert' for -moz-outline

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Aaron Leventhal]

When tabbing through links, you normally get a focus ring.
If you tab to an img that is a link, there is no focus ring, and thus no visual
indication where the keyboard focus is.
The focus ring should not simply be coded as black. If the image has a black
background, the focus ring would not be visible. Perhaps each pixel should be
inversed, the way IE does it.

Comment 1

[Stephen Walker]

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

Comment 2

[Marc Attinasi]

The problem is that we don't support 'invert' as a color in the outline drawing 
code. If this was supported, then the :focus rules could just use invert as the 
color instead of black. Updating summary and assigning to correct owner. Don, 
-moz-outline accepts the value 'invert' to indicate that we want the outline 
color to be a pixel-color-inversion, but the drawing code does not support it.

Comment 3

[Judson Valeski]

-> topembed

Comment 4

[dcone (gone)]

I can do the invert if that color can get to the PaintOutline call in the 
nsCSSRendering module.  Marc can you do that part, once I know how invert gets 
to the PaintOutline I can put the code in to paint the outline inverted.  

Comment 5

[Marc Attinasi]

Accepting - I'll find exactly where the 'invert' color is getting dropped...

Comment 6

[Marc Attinasi]

I added a method on the nsOutlineColor called GetOutlineInvert that returns
PR_TRUE if the color is set to 'invert'. In the CSSRendering method PaintOutline
I check for the inverted outline using that method and, for now, set it to a
conspicuous red color. Don, let's chat about how to get the actual inverted
color and finish it off.

Comment 7

[Marc Attinasi]

Attachment: Work In Progress: make 'invert' outline color available and use in PaintOutline

Comment 8

[Hixie (not reading bugmail)]

The 'invert' outline colour means that the outline should be drawn using an XOR 
mask instead of a particular colour. So the answer is not to pick a particular 
colour, but to change the painting style. Hope that helps...

Comment 9

[Marc Attinasi]

Thanks Ian, Don is working on putting the device context (or whatever the output
thingy is called) into an invert-mode to handle that XOR bit. My WIP code was
just to show him how to tell when Invert was the 'color'. BTW: Don says he has
95% of the draw-in-invert-mode work done - yahoo!

Comment 10

[Hixie (not reading bugmail)]

Cool.

Comment 11

[dcone (gone)]

Attachment: Patch which puts invert mode into the RenderingContext and implments the windows platform.

Comment 12

[dcone (gone)]

I have the fix.. so I will get this in.

Comment 13

[dcone (gone)]

Attachment: better patch. Tested

Comment 14

[Marc Attinasi]

Looks great! I'd like to see the assertion you alluded to in this line:

+  // can use the ::GetROP2(mDC); for debugging, see if windows is in the
correct mode


[s]r=attinasi

Comment 15

[Peter Lubczynski]

r=peterl

Comment 16

[Judson Valeski]

if nsIRenderingContext does not need to be considered for JS usage, we're ok
here. Otherwise, we need to get rid of the enum in favor of constants.

Comment 17

[Judson Valeski]

r=valeski if we're ok from the JS standpoint.

Comment 18

[dcone (gone)]

checked in on the branch 9.2 and 9.4 trunk.

Comment 19

[Pierre Saslawsky]

Reopening: 'invert' does not work.
CCd Aaron who added 'fcc508' to bug 55676
CCd Valeski who reviewed.

1) If you set 'invert' on a 'dotted' outline, the outline is displayed in random 
colors.  Verified on Mac and Windows.

2) If you set 'invert' on a 'solid' outline, the outline is not displayed at all 
(Mac) or in a color that seems wrong (Windows)

To reproduce #1:
- change the focus declaration in html.css line #413 to the following:
   *|*:-moz-any-link:focus {
      -moz-outline: 4px dotted invert;
   }
- go to http://mozilla.org/ 
- tab through the links
===> Bug: the outline is displayed in random colors.  On the Mac, you get 
different colors for almost every link.  If you click the URL field and tab 
again, you get other colors.  On Windows, you get one set of colors the first 
time you tab and another set of colors after reloading the page.

To reproduce #2, do the same thing as above with: 
   *|*:-moz-any-link:focus {
      -moz-outline: 4px solid invert;
   }

Comment 20

[Pierre Saslawsky]

Reassigned to attinasi who originally worked on it and s/reviewed.

Comment 21

[Marc Attinasi]

Hmm. Worked back in September. What happened?

Comment 22

[dcone (gone)]

This was not implemented on the Mac.  Can you be more specific on.. seems 
wrong.. for the color when this is on windows.

Comment 23

[Marc Attinasi]

Attachment: TESTCASE: showing invert focus outline over black, white and red background, solid and dotted

Comment 24

[Marc Attinasi]

The solid outline inverts fine on Windows. The dotted outline gets a funny color
when set to invert, some kind of brownish color for me.

Comment 25

[[e-mail]]

I don't see a solid outline at all.
The dotted outline appears in a random colour.
Reloading the page changes the outline colour.

Comment 26

[Marc Attinasi]

neil, what platform?

Comment 27

[[e-mail]]

With Build ID: 2001091903 on Windows 95, the solid outline now works sort of.
However I am still seeing random dotted outline colours.

Comment 28

[[e-mail]]

Attachment: No outlines show for some reason

Comment 29

[Marc Attinasi]

Comment on attachment 50509 [details]
No outlines show for some reason

No outlines show even with normal color specified. Must be a bug in image-links not getting the :moz-any-link applied or something. Anyway, this has nothing to do with 'invert'

Comment 30

[Marc Attinasi]

Also, I confirm what Neil a dotted invert outline to show will in different
colors if I reload the page. On the same page it is always the same color, however.

So, how important is this for: 
A. non-solid outlines
B. Mac
C. Linux

This was originally an accessability issue I believe - please advise.

Comment 31

[Aaron Leventhal]

The accessibility issue exists on all platforms - the main purpose is for a
dotted inverted outline, to show focus on a variety of backgrounds and images.

I'm not sure whethere anyone would want inverted outlines for anything other
than dotted, but it seems to me that completeness might be a good thing, no?

Comment 32

[Marc Attinasi]

nsCSSRendering::PaintOutline only does the invert logic for solid outlines. We
need to 'educate' the DrawDashedSides and PaintRoundedBorder to know about the
invert color too.

Comment 33

[Marc Attinasi]

It is a little bit more involved... The current invert on solid lines works
because it uses DrawLine calls, which use the Pen in the DC and that pen is
setup as the XOR pen. The dashed and doted borders use the FillRect call, and
that is using the bruch in the DC, and the pen mode has no effect.  I'm looking
for information on how to make a brush do XOR logic now, but if there is no way,
then I could jsut revert to doing a sequence of DrawLine calls for FillRect when
in the Invert mode (seems a bit retrograde - there must be a better way).

Comment 34

[Marc Attinasi]

Fortunately, RenderingContext has a nice little pair of calls 'InvertRect' that
mirror the 'FillRect' calls - and they fix the dotted invert outline problem.
Patch coming after I clean it up.

Comment 35

[Marc Attinasi]

Attachment: Enhanced testcase showing solid, dashed and dotted outlines over various backgrounds

Comment 36

[Marc Attinasi]

Attachment: PATCH to make invert work for dotted and dashed outlines. Tested on Windows only so far.

Comment 37

[Marc Attinasi]

I have an XP fix ready, tested on windows only so far (my Mac battery just
expired). I still need to make sure that we do something sensible, if not
totally correct, on Mac and Linux. I'm not sure if InvertRect is implemented on
those platforms or not, but I'll test it tomorrow.

Comment 38

[Marc Attinasi]

Aaron, do you want to try the attached patch? I tested it extensively on Windows
and it seems to work fine. I'll be testing on Mac and Linux soon, but any
testing doen by others is always good.

Also, please make sure that this has the keywords you need...

Comment 39

[Aaron Leventhal]

Looks good on my Win2k system.

So will the following changes need to be made to html.css?
-  *|*:-moz-any-link:focus {
-    -moz-outline: 1px dotted WindowText;
-  }
+  *|*:-moz-any-link:focus {
+    -moz-outline: 1px dotted invert;
+  }

Comment 40

[Marc Attinasi]

Yes, we should change the link outline rule as you show it - I'll add those when
I commit the patch. 

I just want to make sure that Mac and Linux behave 'reasonably' first - LXR
shows that everybody implements InvertRect so it should be fine.

Aaron, can you give me a review on the patch?

Comment 41

[Marc Attinasi]

Sorry, that patch has problems on the Mac. Basically, there is no notion of a
PenMode in the rendering context for the Mac, so the code that Don wrote
previously (that handled solid outlines) and the new code that I added (for
dotted and dashed outlines) was failing. I changed both cases (solid and
dotted/dashed) to fallback to black if the invert cannot be done, but was also
able to make the dotted and dashed outlines draw inverted on the Mac. Solid
outlines on the Mac are now always black (need help getting the PenMode set to
Invert on the Mac, but this can wait a little while I think). New patch coming...

Comment 42

[Marc Attinasi]

Comment on attachment 51189 [details] [diff] [review]
PATCH to make invert work for dotted and dashed outlines. Tested on Windows only so far.

Problems on Mac - resolved in another patch

Comment 43

[Marc Attinasi]

Attachment: Updated patch: correctly handles some invert for platforms that do not support the PenMode methods

Comment 44

[Marc Attinasi]

Attachment: Same patch with nicer newlines

Comment 45

[Marc Attinasi]

Latest patch works fine on Mac, Linux and Windows. On Mac and Linux, the solid
outlines with invert will render in black. I'll open another bug to get their
SetPenMode implemented so they invert with solid outlines too.

Seeking reviews so I can get this in today.

Comment 46

[kinmoz]

Comment on attachment 51700 [details] [diff] [review]
Same patch with nicer newlines

Looks ok to me. sr=[e-mail]

Comment 47

[Kevin McCluskey (gone)]

Comment on attachment 51700 [details] [diff] [review]
Same patch with nicer newlines

r=[e-mail].

Comment 48

[Marc Attinasi]

Checked into trunk:

/cvsroot/mozilla/layout/html/style/src/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.146; previous revision: 3.145
/cvsroot/mozilla/layout/html/document/src/html.css,v  <--  html.css
new revision: 3.133; previous revision: 3.132

Comment 49

[Marc Attinasi]

TOPEMBED --> PDT

Comment 50

[Jaime Rodriguez, Jr.]

adding nsbranch+ to nominate for PDT+.

Comment 51

[Jaime Rodriguez, Jr.]

pls check it into the branch - PDT+

Comment 52

[Marc Attinasi]

bug 103762 opened for Mac and Linux PenModes supporting invert.

Comment 53

[Marc Attinasi]

Landed on branch now too.

Comment 54

[lchiang]

ian - will you be able to verify this fix on both trunk and branch?  If not,
please reassign the qa contact to gerardo and we can verify.

Comment 55

[Jesse Ruderman]

Continuation:
bug 111526, "link focus outline (invert) invisible on gray backgrounds".

Comment 56

[Madhur Bhatia]

verfied fixed on:-
win2000 : 2002-05-22-1.0.0 branch build
macOS 9.1 : 2002-05-21-1.0.0 branch build
macOS 9.1 : 2002-04-12-trunk build 
Linux 7.3 : 2002-04-29-trunk build

Note:- on macOS and Linux, the solid lines with inverts are rendered in black.

marking the bug verified.

Comment 57

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381