Closed
Bug 455105
Opened 16 years ago
Closed 16 years ago
IsSolidBorderEdge in nsCSSRendering.cpp ignores foreground borders
Categories
(Core :: Web Painting, defect, P2)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: dbaron, Assigned: zwol)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
I noticed this while looking at bug 455093:
IsSolidBorderEdge in nsCSSRendering.cpp ignores foreground borders. This means it reads from the alpha component of an uninitialized color variable. If that happens to be 255, and the alpha component of the 'color' property is not 255, then we'll misrender.
Reporter | ||
Comment 1•16 years ago
|
||
It also ignores border-image, which could also be transparent.
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.1?
OS: Linux → All
Hardware: PC → All
Assignee: nobody → roc
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Assignee: roc → zweinberg
Assignee | ||
Comment 2•16 years ago
|
||
This is a conservative fix - if we're using a border image at all, or if we are using the foreground color at all, we assume the border might be transparent. For border image this really is the best we can do; the image might not be available yet and we'd have to duplicate a whole lot of the code from DrawBorderImage. For foreground color we could give a precise answer but only by changing all callers of PaintBackgroundWithSC to provide the relevant nsStyleColor object, which I thought would be too invasive.
Attachment #345179 -
Flags: superreview?(roc)
Attachment #345179 -
Flags: review?(vladimir)
Attachment #345179 -
Flags: review?(vladimir) → review+
Attachment #345179 -
Flags: superreview?(roc) → superreview+
Comment on attachment 345179 [details] [diff] [review]
(rev 1) patch
+ // opaque would be too much work. We can't limit this to when the
+ // border image has been loaded, because it might become loaded
+ // between now and when PaintBorder is called.
That last sentence seems inaccurate. If it's loaded now I think it must be loaded for the PaintBorder (and it's only CSS z-ordering which guarantees that the PaintBorder is called after this is called, which is a weak thing to rely on). But it's definitely not worth doing this optimization for border images, so just delete this part of the comment.
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3)
>
> That last sentence seems inaccurate. If it's loaded now I think it must be
> loaded for the PaintBorder (and it's only CSS z-ordering which guarantees that
> the PaintBorder is called after this is called, which is a weak thing to rely
> on).
I'm not sure I understand your objection to the sentence. As long as there's any condition at all where this function is called before PaintBorder for the same element, and the image is not-loaded when this function is called but subsequently loaded when PaintBorder is called, it is a bug to use IsBorderImageLoaded() instead of GetBorderImage() in the predicate, and that's all the comment is trying to say...
Reporter | ||
Comment 5•16 years ago
|
||
IsBorderImageLoaded is testing main-thread-only state that's only going to be manipulated by a separate event on the main thread event loop; this code and PaintBorder will be called within the same event.
Right, the image can't change state between painting the background and painting the border.
Assignee | ||
Comment 7•16 years ago
|
||
Ok, here's a revision with the incorrect sentence removed.
Attachment #345179 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Comment 8•16 years ago
|
||
Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/7e85c5796676>
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Comment 9•16 years ago
|
||
Oops, I missed the tests on the previous check-in, here they are: <http://hg.mozilla.org/mozilla-central/rev/d61da77977df>
Flags: in-testsuite+
Updated•16 years ago
|
Keywords: fixed1.9.1
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•