Summary: | Paint the highlight behind selected list markers | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | VERIFIED FIXED | ||||||||
Severity: | Normal | CC: | darin | ||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
mitz
2006-04-15 07:09:49 PDT
Created attachment 7727 [details]
Patch
Mostly lifted from RenderReplaced.
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(). 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.
|