Bug 8408 – Paint the highlight behind selected list markers

• Component: Layout and Rendering
• Status: VERIFIED FIXED

Make non-image list markers inside the selection look like other highlighted text, with the selection color painted behind them, not blended in front.

Created attachment 7727 [details]
Patch

Mostly lifted from RenderReplaced.

[Darin Adler]

Comment on attachment 7727 [details]
Patch

Why isn't there a check of selectionState() inside the "isErrorImage" code path?

The 153 alpha thing from RenderReplaced is really ugly. I'd like it to be something more like this to make the 60% clear:

    const int selectionColorImageOverlayAlpha = 60 * 255 / 100;

If it was in a header file it could be shared with RenderReplaced.

Is it really correct to set the selection state of the item to match the state of the list marker?

Created attachment 7744 [details]
Revised patch

(In reply to comment #2)
> Why isn't there a check of selectionState() inside the "isErrorImage" code
> path?

My mistake, corrected in this version.

> The 153 alpha thing from RenderReplaced is really ugly. I'd like it to be
> something more like this to make the 60% clear:
> 
>     const int selectionColorImageOverlayAlpha = 60 * 255 / 100;
> 
> If it was in a header file it could be shared with RenderReplaced.

Added in RenderObject.h.

> Is it really correct to set the selection state of the item to match the state
> of the list marker?

Yet another mistake. In this version I replaced the list item with the containing block both in setSelecitonState() and in selectionRect().

[Darin Adler]

Comment on attachment 7744 [details]
Revised patch

In RenderListMarker::selectionRect, I think it's a little strange to compute selectionRight by adding width() to selectionLeft, then later just subtract selectionLeft out. Instead we should either just use width() directly or have a selectionWidth local variable.

Otherwise this looks perfect to me.

r=me with or without that change.