Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by trchen Modified:
4 years, 10 months ago CC:
blink-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, blink-reviews-rendering, jchaffraix+rendering Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionCorrect box shadow overflow computation for InlineFlowBox
InlineFlowBox::addBoxShadowVisualOverflow() has an early exit condition that
doesn't match the logic in InlineFlowBoxPainter::paintBoxDecorationBackground(),
resulting in under-reported visual overflow. This CL makes correction to that.
Similar adjustment is made to InlineFlowBox::addBorderOutsetVisualOverflow()
in this CL too.
BUG=498848
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197412
Patch Set 1 #
Total comments: 1
Patch Set 2 : do the same to addBorderOutsetVisualOverflow #Messages
Total messages: 18 (7 generated)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182053009/1
trchen@chromium.org changed reviewers: + schenney@chromium.org
One-liner. PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fs@opera.com changed reviewers: + jchaffraix@chromium.org, leviw@chromium.org
https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... File Source/core/layout/line/InlineFlowBox.cpp (right): https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... Source/core/layout/line/InlineFlowBox.cpp:751: if (!parent() && (!isFirstLineStyle() || &style == layoutObject().style())) This is also true for border-image outsets and outline (below), so seems reasonable to extend those predicates in the same way.
On 2015/06/18 08:47:33, fs wrote: > https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... > File Source/core/layout/line/InlineFlowBox.cpp (right): > > https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... > Source/core/layout/line/InlineFlowBox.cpp:751: if (!parent() && > (!isFirstLineStyle() || &style == layoutObject().style())) > This is also true for border-image outsets and outline (below), so seems > reasonable to extend those predicates in the same way. Thanks for pointing out! I applied the change to InlineFlowBox::addBorderOutsetVisualOverflow() too. For the outline, I examined InlineFlowBoxPainter::paint() it doesn't seem to respect :first-line, thus unchanged. (I don't know which way is more standard compliant.) Note: No new tests are needed because they will be caught by strict cull rect checking.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182053009/20001
On 2015/06/18 21:44:12, trchen wrote: > On 2015/06/18 08:47:33, fs wrote: > > > https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... > > File Source/core/layout/line/InlineFlowBox.cpp (right): > > > > > https://codereview.chromium.org/1182053009/diff/1/Source/core/layout/line/Inl... > > Source/core/layout/line/InlineFlowBox.cpp:751: if (!parent() && > > (!isFirstLineStyle() || &style == layoutObject().style())) > > This is also true for border-image outsets and outline (below), so seems > > reasonable to extend those predicates in the same way. > > Thanks for pointing out! I applied the change to > InlineFlowBox::addBorderOutsetVisualOverflow() too. > > For the outline, I examined InlineFlowBoxPainter::paint() it doesn't seem to > respect :first-line, thus unchanged. (I don't know which way is more standard > compliant.) Right, 'outline' isn't in the ::first-line whitelist. LGTM, but you may want second opinions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/06/18 23:24:09, fs wrote: > LGTM, but you may want second opinions. I feel this is pretty safe. I'll just land it gahahahaha.
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182053009/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197412 |
