Bug 8408 - Paint the highlight behind selected list markers
Summary: Paint the highlight behind selected list markers
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-04-15 07:09 PDT by mitz
Modified: 2006-04-19 02:26 PDT (History)
1 user (show)

See Also:


Attachments
Patch (12.41 KB, patch)
2006-04-15 07:27 PDT, mitz
no flags Details | Formatted Diff | Diff
Revised patch (14.11 KB, patch)
2006-04-16 08:15 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-04-15 07:09:49 PDT
Make non-image list markers inside the selection look like other highlighted text, with the selection color painted behind them, not blended in front.
Comment 1 mitz 2006-04-15 07:27:01 PDT
Created attachment 7727 [details]
Patch

Mostly lifted from RenderReplaced.
Comment 2 Darin Adler 2006-04-15 18:15:30 PDT
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?
Comment 3 mitz 2006-04-16 08:15:11 PDT
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().
Comment 4 Darin Adler 2006-04-16 19:49:30 PDT
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.